[SharedCache] Improvements and fixes for MMappedFileAccessor stuff

[WeiN76LQh]

MMappedFileAccessor::Open wasn't really behaving as I think it was intended. It tried to use fileAccessorSemaphore to limit the number of concurrent files Binary Ninja could open. It didn't really confirm it had acquired a reference on fileAccessorSemaphore but then would always release one back when a MMappedFileAccessor was deallocated. This could actually inflate the count of fileAccessorSemaphore meaning it could end up with a higher file limit than originally set. Also using a worker to deallocate a MMappedFileAccessor on another thread felt like a bit of a gross hack and made synchronizing dropping a ref and acquiring one on fileAccessorSemaphore harder than it needed to be. This resulted in a bit of a rewrite of MMappedFileAccessor::Open to mitigate these issues. It now should respect the file limit set on startup but can go over it temporarily in extreme circumstances that I don't expect to occur in real world experiences.

Additionally there seemed to be some insufficient use of locking to synchronize accesses to certain data structures. I believe this commit has cleaned those up. They were causing very intermittent crashes.

[bdash]

As an FYI, #6316 also changes MMappedFileAccessor::Open and will conflict with this. It shouldn't be too bad to resolve as none of the behavior it changes is relevant to what you're fixing here.

[WeiN76LQh]

Yes I hadn't seen your PRs until after I pushed this one. As you state it doesn't really conflict with any of your changes. If an updated PR is required in the future then I can do that.

[WeiN76LQh]

I've identified another bug in this area of code to do with the blockedSessionIDs variable. If a user opens DSC using Open Selected Files With Options... and then changes a setting (I think any setting) and then clicks Open, in the background a DSCView is destroyed and a new one is created. The destruction of the first one calls MMappedFileAccessor::CloseAll which adds the current session ID to the blockedSessionIDs list. However the newly created DSCView has the same session ID so during initial load it is constantly creating and destroying MMappedFileAccessors due to this check preventing MMappedFileAccessors being added to fileAccessorReferenceHolder. The performance degradation is pretty crazy, causing the initial load to take minutes. During which Binary Ninja is unresponsive so its likely for the average user they will just kill the process after a bit of time of seemingly nothing happening.

I would provide a fix in this PR via an additional commit but I'm unsure how to proceed as I don't fully understand the mechanics of everything in this context and the levers that can be worked with. blockedSessionIDs existence feels like a hack to solve a problem which I don't really understand exactly how it exists. If this check is still required then another solution is going to be needed otherwise I guess the code can just be removed?

[bdash]

There's another issue with how MMappedFileAccessor::Open works / is used. Every call to it takes in a postAllocationRoutine function. This is used by SharedCache to apply the slide when the file is opened. But MMappedFileAccessor::Open caches the first lazy accessor (and its postAllocationRoutine) created for any given path. If the first lazy accessor does not include a postAllocationRoutine, no slide will be applied if the MMappedFileAccessor is closed and subsequently reopened. SharedCache::PerformInitialLoad never specifies a postAllocationRoutine when opening files, and it touches most (all?) of the files in the shared cache.

In practice the file descriptor limit seems high enough that files for a single shared cache tend to stay open indefinitely, avoiding this problem. I guess if you open several iOS shared caches with macOS's default low file descriptor limit you might hit the problem.

I was able to trigger it by hard-coding a file descriptor limit of 8 and opening an iOS shared cache. I can see files being closed then reopened, with no slide being applied.

Perhaps the approach to file accessor caching should be revisited?

A lot of the hoops that the existing code jumps through to try to limit the number of open files could be avoided by using setrlimit(RLIMIT_NOFILE, …) to bump the soft file descriptor limit to a more reasonable value. It may still be desirable to close files that haven't been accessed recently, but that would become a memory optimization rather than being necessary for correctness.

[bdash]

I sent #6392 which removes the complex synchronization in MMappedFileAccessor::Open rather than trying to fix it. It solves the underlying issues by:

1. Using setrlimit to increase the file descriptor limit.
2. Using an LRU cache to impose a soft limit on the number of open MMappedFileAccessor objects. This is primarily a memory optimization since closing the accessors lets the dirty pages corresponding to those files be discarded.
3. Allowing caching of file accessors opened by sessions corresponding to views that have been closed (to avoid the issue mentioned in a comment above: [SharedCache] Improvements and fixes for MMappedFileAccessor stuff #6318 (comment)).

[WeiN76LQh]

Closing because this is no longer relevant after #6392 was merged