BNBeginUndoActions can cause a deadlock
#6289
Labels
Component: DSC
Issue needs changes to the DyldSharedCacheView
File Format: SharedCache
Issue with the dyld_shared_cache plugin
Impact: Critical
Issue blocks CRITICAL functionality
State: Awaiting Triage
Issue is waiting for more in-depth triage from a developer
Type: Crash
Issue is a crash or deadlock
Comments
|
Removing calls to This issue is a continuation of the undo action API being problematic for me; its not very performant, causes crashes in headless and there's also this deadlock issue. |
|
Potentially related to #6097 |
WeiN76LQh
pushed a commit
to WeiN76LQh/binaryninja-api
that referenced
this issue
Jan 2, 2025
This is a massive and messy commit that started out as trying to add bulk loading of images (the ability to load them in parallel via a single API call) but then required a number of changes that unearthed some fundamental bugs with the current design of `SharedCache`. Untangle it all to create nice clean commits is more work than its worth. The main bug fix is the fact that `State()` was not actually thread safe because `m_state` which it was returning a copy of could be modified concurrently during the copying. Now a lock is held when accessing `m_state` via functions like `State()` and a `shared_ptr` is returned. This is a thread safe operation and the returned `shared_ptr` guarantees the `State` struct remains allocated whilst that `shared_ptr` exists. This has also required changes to some of the code to create local variables to ensure things remain allocated after they are read from `State()`. The other big change is introducing an API call to load a number of images concurrently. This is done using worker threads and required some changes to `SharedCache::LoadImageWithInstallName` and `SharedCache::LoadSectionAtAddress` so that they could work in parallel. Previously they just took locks for extended periods, this is now reduced as much as possible. Other work had to be done to make parts of the loading process thread safe. Something else this commit adds is the commenting out of calls to the undo actions API; `BeginUndoActions`, `ForgetUndoActions` and `CommitUndoActions`. Tbh I don't know if this actually works at all when being used in parallel as, according to Vector35, due to the design of the undo actions system, it can't distinguish undo actions across threads. So when you've got loads of threads making calls to this API and then calling `ForgetUndoActions` or `CommitUndoActions`, is what you expect actually getting forgotten or committed? The main reason for commenting these out though is issues like Vector35#6289. There might be some other minor changes in this commit I've forgotten about but I think this is the majority of it.
emesare
added
Type: Crash
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Version and Platform (required):
Bug Description:
BNBeginUndoActionscan cause a deadlock by what appears to be, from the stack trace below, due to waiting on a condition variable:There are other threads stuck in
BNForgetUndoActions:Another thread (with the name
"Worker T module:core.module.loadDebugInfo module:core.module...") is stuck waiting here:2 threads called
"Thread (pooled)"are stuck here:The deadlocking is permanent, Binary Ninja becomes not responding and its CPU usage drops to basically 0.
There is a chance this is user error but the deadlocking is occurring inside Binary Ninja core so I'm not sure how I could be the cause of it.
Steps To Reproduce:
I'm not sure how to reproduce with a vanilla copy of the specified version of Binary Ninja. I'm using a customised version of the DSC plugin that does allow more parallelization so it could be user error, but ultimately the hanging is due to something going on in Binary Ninja core.
The way I cause this deadlock is by the following steps:
File -> New Window.The important parts here seem to be that analysis is going on for both at the same time and that both files that are opened are for DYLD Shared Cache.
I know thats not particularly helpful, I'm hoping from the stack traces it might be clear where multiple threads are either contending for the same lock or where a condition variable is not being notified.
Expected Behavior:
To be honest I don't really know.
BeginUndoActionsis a bit of an odd one because my understanding is it doesn't really work in multi-threaded cases due to the way undo actions have been architected. Obviously it shouldn't deadlock but does the use ofBeginUndoActionsin a multi-threaded situation make any sense? Given Binary Ninja is designed to be multi-threaded as much as possible doesBeginUndoActionsmake sense in most situations because it won't necessarily batch the expected undo actions? The DSC plugin uses it in a number of places to either forget or batch undo actions, but given its multi-threaded nature would this even work as expected?The text was updated successfully, but these errors were encountered: