diff --git a/view/sharedcache/core/VM.cpp b/view/sharedcache/core/VM.cpp index 5aca51d908..76046998d5 100644 --- a/view/sharedcache/core/VM.cpp +++ b/view/sharedcache/core/VM.cpp @@ -52,13 +52,36 @@ void VMShutdown() { - std::unique_lock lock2(fileAccessorsMutex); - std::unique_lock lock(fileAccessorDequeMutex); - // This will trigger the deallocation logic for these. // It is background threaded to avoid a deadlock on exit. - fileAccessorReferenceHolder.clear(); - fileAccessors.clear(); + + // Do not grab both locks at the same time otherwise a deadlock will occur + // due to `fileAccessorReferenceHolder.clear()` triggering + // `SelfAllocatingWeakPtr` deallocation routines that + // need to acquire `fileAccessorsMutex`. + { + // Some additional complexity with this lock is that dropping all these references likely + // will cause the final reference to a DSC binary view to be dropped. In doing so + // `~DSCView` will call `MMappedFileAccessor::CloseAll` which requires the same lock. This + // occurs on the same thread, resulting in it trying to acquire a lock it already has. + // This solution copies all the deques from `fileAccessorReferenceHolder`, therefore + // keeping all the references alive temporarily. Then `fileAccessorReferenceHolder` can be + // cleared and the `fileAccessorDequeMutex` lock can be dropped. When exiting this scope + // the desctructor for `accessorDequesToDrop` will drop all the `MMappedFileAccessor` + // references whilst holding no locks. + std::unique_lock lock(fileAccessorDequeMutex); + std::vector>> accessorDequesToDrop; + for (auto& [_, fileAccessorDeque] : fileAccessorReferenceHolder) + { + accessorDequesToDrop.push_back(fileAccessorDeque); + } + fileAccessorReferenceHolder.clear(); + lock.unlock(); + } + { + std::unique_lock lock(fileAccessorsMutex); + fileAccessors.clear(); + } } @@ -205,61 +228,141 @@ void MMAP::Unmap() std::shared_ptr> MMappedFileAccessor::Open(BinaryNinja::Ref dscView, const uint64_t sessionID, const std::string &path, std::function)> postAllocationRoutine) { - std::scoped_lock lock(fileAccessorsMutex); - if (fileAccessors.count(path) == 0) - { - auto fileAcccessor = std::shared_ptr>(new SelfAllocatingWeakPtr( - // Allocator logic for the SelfAllocatingWeakPtr - [path=path, sessionID=sessionID, dscView](){ - std::unique_lock _lock(fileAccessorDequeMutex); - - // Iterate through held references and start removing them until we can get a file pointer - // FIXME: This could clear all currently used file pointers and still not get one. FIX! - // We should probably use a condition variable here to wait for a file pointer to be released!!! + std::unique_lock lock(fileAccessorsMutex); + if (auto it = fileAccessors.find(path); it != fileAccessors.end()) { + return it->second; + } + + auto fileAccessor = std::shared_ptr>(new SelfAllocatingWeakPtr( + // Allocator logic for the SelfAllocatingWeakPtr. This has been written to respect + // `maxFPLimit` as much as possible. However if, for whatever reason, a deadlock occurs, + // requiring more than `maxFPLimit` of files to be opened to continue past, there is a + // timeout added to do this. It should almost never occur in real world conditions (I + // believe) but a timeout safe guard has been added to choose to exceed `maxFPLimit` + // temporarily to avoid a permanent deadlock. The absolute worst case is Binary Ninja + // throws a bunch of errors because it tried to open a file and couldn't due to actually + // hitting the OS enforced open file limit. + [path=path, sessionID=sessionID, dscView](){ + // This lock needs to be held for the entire duration of this function, in part to + // synchronize access to `fileAccessorReferenceHolder` and `blockedSessionIDs`, but + // also to ensure that, if a thread needs to drop a file accessor reference to get a + // `fileAccessorSemaphore` ref, then there's actually a reference to drop and its the + // one to acquire it after triggering for it to get dropped. This is due to 2 + // complexities here: + // 1. If thread 1 tries to acquire a ref on `fileAccessorSemaphore` with `try_acquire` + // and fails then it needs to drop a strong reference to a file accessor by + // removing an entry from one of the values in `fileAccessorReferenceHolder`. + // Without synchronizing this function its possible that thread 2 could come in + // after thread 1 drops an entry but before it can block in + // `fileAccessorSemaphore.acquire`, potentially sniping away the reference it has + // just freed up by thread 2 reaching `try_acquire` first. Its then possible that + // nothing else causes a reference to be dropped because + // `fileAccessorReferenceHolder` keeps file accessors alive until this function + // drops one. That would then lead a thread to deadlock in + // `fileAccessorSemaphore.acquire`. So a lock must be held the entire time that a + // ref is trying to be acquired on `fileAccessorSemaphore` to prevent sniping. + // 2. In a very contrived scenario where `fileAccessorSemaphore` has a count of 1 (as + // in there's only 1 reference to give out) then its possible that, thread 1 could + // take that only reference but before it reaches the end of the function and adds + // the newly created `accessor` to `fileAccessorReferenceHolder`. Thread 2 comes + // in, observes there's no reference, tries to drop one from + // `fileAccessorReferenceHolder` but there aren't any currently and then deadlocks + // in `fileAccessorSemaphore.acquire`. + std::unique_lock lock(fileAccessorDequeMutex); + + bool refAcquired = fileAccessorSemaphore.try_acquire(); + if (!refAcquired) + { + // The permitted allowance of open files has been used up. Drop a reference to one + // and wait for it to close its file. Assume there's always at least one reference + // to drop but if not then the code below can handle the situation. for (auto& [_, fileAccessorDeque] : fileAccessorReferenceHolder) { - if (fileAccessorSemaphore.try_acquire()) + if (!fileAccessorDeque.empty()) + { + fileAccessorDeque.pop_front(); break; - fileAccessorDeque.pop_front(); + } } - mmapCount++; - _lock.unlock(); - auto accessor = std::shared_ptr(new MMappedFileAccessor(ResolveFilePath(dscView, path)), [](MMappedFileAccessor* accessor){ - // worker thread or we can deadlock on exit here. - BinaryNinja::WorkerEnqueue([accessor](){ - fileAccessorSemaphore.release(); - mmapCount--; - if (fileAccessors.count(accessor->m_path)) - { - std::scoped_lock lock(fileAccessorsMutex); - fileAccessors.erase(accessor->m_path); - } - delete accessor; - }, "MMappedFileAccessor Destructor"); - }); - _lock.lock(); - // If some background thread has managed to try and open a file when the BV was already closed, - // we can still give them the file they want so they dont crash, but as soon as they let go it's gone. + // There should now be a count of 1 on `fileAccessorSemaphore` but its possible + // that its not the case. This happens when the file limit is being heavily + // consumed by long living references to `MMappedFileAccessor`. + // `fileAccessorReferenceHolder` isn't the only holder of strong references to + // `MMappedFileAccessor`. Any call to this function followed by calling + // `SelfAllocatingWeakPtr::lock` on the return value will create a strong + // reference. Dropping that `MMappedFileAccessor` from the map + // `fileAccessorReferenceHolder` won't cause deallocation until those other strong + // references returned via `SelfAllocatingWeakPtr::lock` have also been destructed. + // Therefore its possible to block here for a bit whilst another thread completes + // up the work it is doing with a mapped file. It is also possible to deadlock + // here if any of the coding in the rest of the plugin allows for a single thread + // to lock more than 1 `MMappedFileAccessor` at a time. At the time of writing + // this comment it is true that all paths of execution will only need to hold at + // max 1 `MMappedFileAccessor` lock at a time. Future changes should try to + // maintain this as much as possible to ensure a deadlock is impossible. However + // even if this is not the case, as long as the maximum number of + // `MMappedFileAccessor` locks that may need to be held by a single thread, is a + // small number, then the actual probability of a deadlock should be + // extraordinarily low and would likely require the perfect storm of conditions + // for it to occur. + // `acquire` has been modified to have deadlock protection by adding a timeout. In + // the case that a deadlock occurs and the timeout is reached, the following code + // can go above `maxFPLimit` temporarily. + // The previous implementation of the code here tried its best to make a file + // available but never checked that it had, instead it could go over the + // `maxFPLimit`. + refAcquired = fileAccessorSemaphore.acquire(std::chrono::seconds(10)); + if (!refAcquired) + BinaryNinja::LogWarn("Potential deadlock occurred in MMappedFileAccessor::Open"); + } + + mmapCount++; + auto accessor = std::shared_ptr(new MMappedFileAccessor(ResolveFilePath(dscView, path)), [refAcquired](MMappedFileAccessor* accessor){ + { + std::unique_lock lock(fileAccessorsMutex); + fileAccessors.erase(accessor->m_path); + } + delete accessor; + mmapCount--; + // Release the reference once the `MMappedFileAccessor` destructor has completed + // and the file is actually closed. Only drop the ref if one was actually + // acquired. Read the comment above the line where `fileAccessorSemaphore` is + // acquired, for information on how this can occur. + if (refAcquired) + fileAccessorSemaphore.release(); + }); + + // Only hold a strong reference to a `MMappedFileAccessor` if a + // `fileAccessorSemaphore` ref was acquired, otherwise we're going over the FP limit + // and therefore should drop the reference ASAP to cause the file to close. Also this + // prevents a situation where the above code, when trying to acquire a + // `fileAccessorSemaphore` ref, drops a reference to a `MMappedFileAccessor` that + // won't release a `fileAccessorSemaphore` ref, which can cause a deadlock. + if (refAcquired) + { + // If some background thread has managed to try and open a file when the BV was + // already closed, we can still give them the file they want so they dont crash, + // but as soon as they let go it's gone. if (!blockedSessionIDs.count(sessionID)) fileAccessorReferenceHolder[sessionID].push_back(accessor); - return accessor; - }, - [postAllocationRoutine=postAllocationRoutine](std::shared_ptr accessor){ - if (postAllocationRoutine) - postAllocationRoutine(accessor); - })); - fileAccessors.insert_or_assign(path, fileAcccessor); - } - return fileAccessors.at(path); + } + return accessor; + }, + [postAllocationRoutine=postAllocationRoutine](std::shared_ptr accessor){ + if (postAllocationRoutine) + postAllocationRoutine(accessor); + })); + + fileAccessors.insert_or_assign(path, fileAccessor); + return fileAccessor; } void MMappedFileAccessor::CloseAll(const uint64_t sessionID) { + std::unique_lock lock(fileAccessorDequeMutex); blockedSessionIDs.insert(sessionID); - if (fileAccessorReferenceHolder.count(sessionID) == 0) - return; fileAccessorReferenceHolder.erase(sessionID); } @@ -285,21 +388,19 @@ void MMappedFileAccessor::InitialVMSetup() } else { - if (maxFPLimit < 10) { #ifdef _MSC_VER - // It is not _super_ clear what the max file pointer limit is on windows, - // but to my understanding, we are using the windows API to map files, - // so we should have at least 2^24; - // kind of funny to me that windows would be the most effecient OS to - // parallelize sharedcache processing on in terms of FP usage concerns - maxFPLimit = 0x1000000; + // It is not _super_ clear what the max file pointer limit is on windows, + // but to my understanding, we are using the windows API to map files, + // so we should have at least 2^24; + // kind of funny to me that windows would be the most effecient OS to + // parallelize sharedcache processing on in terms of FP usage concerns + maxFPLimit = 0x1000000; #else - // unix in comparison will likely have a very small limit, especially mac, necessitating all of this consideration - struct rlimit rlim; - getrlimit(RLIMIT_NOFILE, &rlim); - maxFPLimit = rlim.rlim_cur / 2; + // unix in comparison will likely have a very small limit, especially mac, necessitating all of this consideration + struct rlimit rlim; + getrlimit(RLIMIT_NOFILE, &rlim); + maxFPLimit = rlim.rlim_cur / 2; #endif - } } BinaryNinja::LogInfo("Shared Cache processing initialized with a max file pointer limit of 0x%llx", maxFPLimit); fileAccessorSemaphore.set_count(maxFPLimit); diff --git a/view/sharedcache/core/VM.h b/view/sharedcache/core/VM.h index e47cf15ef4..b8b5bf31ff 100644 --- a/view/sharedcache/core/VM.h +++ b/view/sharedcache/core/VM.h @@ -21,10 +21,30 @@ class counting_semaphore { cv_.notify_all(); } - void acquire() { + // Blocks for the specified number of seconds (or indefinitely if its 0) + // or until the semaphore has a reference available. The timeout is to + // prevent deadlocking if the caller is concerned about that. The caller + // can determine if a ref was acquired by checking the return value. If a + // ref was not acquired then a timeout occurred. + bool acquire(std::chrono::seconds timeout) { std::unique_lock lock(mutex_); - cv_.wait(lock, [this]() { return count_ > 0; }); - --count_; + if (count_ > 0) { + --count_; + return true; + } + bool acquired = false; + if (timeout != std::chrono::seconds(0)) + { + acquired = cv_.wait_for(lock, timeout, [this]() { return count_ > 0; }); + } + else + { + cv_.wait(lock, [this]() { return count_ > 0; }); + acquired = true; + } + if (acquired) + --count_; + return acquired; } bool try_acquire() { @@ -106,8 +126,8 @@ class MMAP { static uint64_t maxFPLimit; static std::mutex fileAccessorDequeMutex; -static std::unordered_map>> fileAccessorReferenceHolder; -static std::set blockedSessionIDs; +static std::unordered_map>> fileAccessorReferenceHolder; // access is protected by `fileAccessorDequeMutex` +static std::set blockedSessionIDs; // access is protected by `fileAccessorDequeMutex` static std::mutex fileAccessorsMutex; static std::unordered_map>> fileAccessors; static counting_semaphore fileAccessorSemaphore(0);