| Age | Commit message (Collapse) | Author |
|---|
|
MMappedFileAccessor::Open
Nothing guarantees that the `SharedCache` lives long enough.
|
|
There was a second BeginBulkModifySymbols after rebasing the new processing symbols commit
|
|
This is _probably_ the fix? I am not actually sure I am pushing so the CI can figure it out for me :)
|
|
|
|
|
|
Split out from https://github.com/WeiN76LQh/binaryninja-api/tree/process-local-symbols
|
|
Split out from https://github.com/WeiN76LQh/binaryninja-api/tree/process-local-symbols
|
|
|
|
|
|
The initial state is initialized during `PerformInitialLoad` and is
immutable after that point. This required some slight restructuring of
how information about memory regions is tracked as that was previously
modified as regions were loaded. Memory regions are now stored in a map
from their address range to the `MemoryRegion` object. This makes it
cheap to look them up by address which is a common operation.
The modified state consists of changes since the last save to the
`DSCView` / `ViewSpecificState`. This means it is no longer necessary to
copy any state when mutating a `SharedCache` instance for the first
time. Instead, its data structures start off empty and are populated as
images, sections, or symbol information is loaded.
The loaded state consists of all modified state that has since been
saved. It lives on the `ViewSpecificState`. Saving modified state
merges it into the the existing loaded state.
This pattern is carried over to the `Metadata` stored on the `DSCView`.
The initial state is stored under its own metadata key, and each
modified state is stored under a key with an incrementing number. This
means each save of the state only needs to serialize the state that
changed, rather than reserializing all of the state all of the time.
There are two huge benefits from these changes:
1. At no point does `SharedCache` have to copy its in memory state.
The basic copy-on-write approach introduced in #6129 reduced how
often these copies are made, but they're still frequent and very
expensive.
1. At no point does `SharedCache` have to re-serialize state to JSON
that it has already serialized. JSON serialization previously added
hundreds of milliseconds to any mutating operation on `SharedCache`.
As a result, this speeds up the initial load of the shared cache by
around 2x and loading of subsequent images improves by about the same.
One trade-off is that the serialization / deserialization logic is more
complicated. There are two reasons for this:
1. The state is now split across multiple metadata keys and needs to be
merged when it is loaded.
2. The in-memory representation uses pointers to identify memory regions.
These relationships have to be re-established after the JSON is
deserialized.
As a future direction it is worth considering whether the logic owned by
`SharedCache` could be split in a similar manner to the data. The
initial loading of the cache header, loading of images, and handling of
symbol information are all mostly independent and work on separate data.
If the logic were split into separate classes it would be easier to
reason about which data is valid when, and would easily permit
concurrent loading of multiple images from the shared library in a
thread-safe manner.
|
|
|
|
Fixes MSVC C++17 failing to compile, hopefully.
|
|
|
|
which had not been serialized.
We will just return early, this is fine as the previous code would effectively do the same thing.
|
|
These are from clang so there are still a bunch of warnings on GCC and MSVC
|
|
This apparently is not an issue on LLVM, but GCC and MSVC both seem to dislike this.
|
|
Potential nullptr deref on invalid view type (see the error log right after the logger init)
|
|
This is a meaningless commit, i'm throwing this in for when someone inevitably calls out the inconsistent "formatting" (what style is this again?)
|
|
A logic error in `FindSymbolAtAddrAndApplyToAddr` meant that a code path
that would attempt to re-use existing symbol definitions in the view
would still fall back to searching the exported symbol list.
`FindSymbolAtAddrAndApplyToAddr` is updated to return early if a symbol
already exists at the target location and the target location is the
same as the symbol location. Additionally, in the event a symbol was found
in the view at the symbol location, it is applied to the target location
and then the function returns.
Additionally, `WillMutateState` is moved after these initial checks so
it is only called in the codepaths that attempt to locate the symbol
amongst the exported symbols.
After these changes it is now possible to load QuartzCore from the iOS
14 shared cache in around 70 seconds. Previously it would run for hours
without the load completing.
|
|
Also add some other misc error reporting and nullptr check
|
|
This allows a consumer to bypass the alloc + free of the API NameSpace, removing the need for the consumer to manually construct a symbol for said behavior.
|
|
Previously, `MMappedFileAccessor::Open` attempted to impose a fixed limit
on the number of file accessors that were live at one time. This was
done because the default file descriptor limit on some platforms is
relatively low (256 on macOS). Given that recent iOS shared caches can
contain 60+ files it is easy to tie up a large percentage of this limit
by opening one or two shared caches.
This is problematic as if the limit imposed by `MMappedFileAccessor::Open`
is reached, the attempt to access the file will block waiting for
another file accessor to be closed. This can lead to a deadlock.
This commit makes three changes to this strategy:
1. It attempts to raise the file descriptor limit to 1024. Unix systems
support both soft and hard file descriptor limits. The soft file
descriptor limit is what is enforced, but a process can explicitly
raise the limit to any value below the hard limit if it wishes.
2. The fixed limit on open files accessors is changed to a soft limit.
`FileAccessorCache` is introduced to manage the caching of file
accessors. It provides a basic LRU cache. Whenever a new file is
opened, the cache of open accessors is pruned to stay below the
target limit (50% of the soft file descriptor limit). This limiting
is primarily done to allow files containing dirty pages to be
unmapped if they're no longer being used.
LRU isn't the optimal strategy for this, but it is simple to
implement and understand. Ideally there'd be some access time
component to the cache so files that haven't been accessed can be
released.
3. File accessors are cached even for sessions corresponding to views
that have been closed. The "Open Selection with Options..." context
menu hits this case as a view is created and destroyed as part of
populating the options dialog, and the real view is then created in
the same session.
The most significant benefit of this change is that the logic around
opening / closing file accessors is simpler and can no longer deadlock.
While working on this I noticed that `SharedCache` opened some files via
`MMappedFileAccessor::Open` without specifying a post-open operation to
apply slide information. Instead, it would explicitly apply the slide
information after opening the file. This works fine so long as the file
accessor limit is never hit. If it is hit and the accessor is closed,
the next time the file is opened it will not have any slide information
applied. This would lead to very confusing bugs.
|
|
This change is mostly motivated by simplifying the code, but it also
brings minor correctness and performance benefits.
1. The pointer returned by mmap is stored as a uint8_t* rather than
void* as that is how it is used. This reduces how often it needs to
be cast to a different type before it is used.
2. Read methods for primitives delegate to a new Read template function
that in turn delegates to the general-purpose `Read(void* dest, size_t
address, size_t length)`. This improves the consistency of bounds
checking and simplifies the code. The compiler is more than willing
to inline this so we get less repetition with no overhead.
3. ReadNullTermString now uses std::find to find the nul byte and
directly constructs the string from that range of bytes. This removes
an unnecessary allocation that was previously being forced by the use
reserve followed by shrink_to_fit. It also avoids repeated
reallocation for longer strings as they grew past the the reserved
size as they were being built up a character at a time.
|
|
`MetadataSerializable` is updated to allow a subclass to optionally
specify the return type of `Load`. If not specified it defaults to the
subclass itself. `SharedCache` specifies `std::optional` to represent
the case where the serialized metadata is in a format or version it does
not recognize.
By making `Load` a static member function and having it return a new
object it becomes easier to reason about the state of objects when a
deserialization failure occurs. It can return `std::optional` to
indicate failure and there is no object left in an unknown state.
Prior to this, `Load` worked on an existing instance of an object. It
was unclear what state the object would be in if a deserialization
failure occurred (its original state prior to `Load`? some
partially-loaded state? a null state?). The failure itself also had to
be communicated out of band.
|
|
|
|
|
|
This avoids expensive copying when returning a value from the map in `SharedCache::GetExportListForHeader`. Additionally it ensures that the value stays alive and at the same location in memory if `m_exportInfos` is modified and requires its storage to be re-allocated.
I was unable to use a `unique_ptr` instead of a `shared_ptr` because of copy semantics with `m_exportInfos` in `ViewStateCacheStore`. I don't see things being any worse using `shared_ptr` instead of `unique_ptr` anyway and it means less code changes.
|
|
`SharedCache::FindSymbolAtAddrAndApplyToAddr` if a symbol is found
|
|
This commit changes 2 things;
1. `m_exportInfos` is now a map where its values are also a map rather than a vector of pairs. The reason for this is that `SharedCache::FindSymbolAtAddrAndApplyToAddr` is a hot path which does by far the most accesses to `m_exportInfos`. In that function it must find the correct symbol for a given address so a map lookup will be much quicker than iterating a vector. The other use cases of `m_exportInfos` would prefer a vector but they are executed very infrequently.
2. The symbols are stored in `m_exportInfos` as references to the `Symbol` type. This makes more sense because otherwise there is a lot of time spent converting to and from a `Symbol` type and a pair of `BNSymbolType` + a `std::string`.
|
|
`m_exportInfos` was modified
This probably makes more sense than the current solution of using execution of the callback parameter to determine if `m_exportInfos` was modified.
|
|
`SharedCache::ParseExportTrie` is getting called a lot during DSC library loading and analysis. In large part due to the hot path `SharedCache::FindSymbolAtAddrAndApplyToAddr`. Its unnecessary for it to be being called more than once per DSC header as the export list symbol information is stored in `SharedCache::m_exportInfos`.
This commit adds the function `SharedCache::GetExportListForHeader`, which will either return the header's list of symbol information cached in `SharedCache::m_exportInfos` or call `SharedCache::ParseExportTrie` and cache the results in `SharedCache::m_exportInfos`.
This should also improve the execution time of `SharedCache::LoadAllSymbolsAndWait`.
Further improvement here would be to add locking to `SharedCache::GetExportListForHeader` so that races don't result in redundant parsing of the export trie for the same header if multiple threads call `SharedCache::GetExportListForHeader` at the same time for the same header. This only really matters during initial loading because from what I can tell that parses all the export trie's anyway.
|
|
|
|
|
|
`ReadExportNode` is called a lot during the initial load of the shared
cache and thus impacts how long it takes for the UI to become
responsive. This is a collection of optimizations that cut the time
spent within `ReadExportNode` by 50%:
1. Pass iterators to `ReadExportNode` rather than a `DataBuffer` + offset.
The lack of inlining in `DataBuffer`'s `operator[]` kills performance.
Ideally this would have used `std::span`, but that would require
bumping the minimum C++ version to C++20.
2. Add `MMappedFileAccessor::ReadSpan` so that `ReadExportNode` can
operate directly on the mapped data without first copying it.
3. Removes a call to `GetAnalysisFunctionsForAddress` whose result was
unused.
4. Use `std::find` to find the nul at the end of strings rather than
assembling the string a character at a time. This avoids repeatedly
growing the string.
5. Avoid the usual `Symbol` constructor in favor of `BNCreateSymbol`.
The `Symbol` constructor has over head in two forms:
1. It has a `NameSpace` as an argument. Creating / destroying this
allocates and deallocates memory We could create a single
instance and reuse it for all calls, but...
2. The constructor copies all fields of the `NameSpace` to the heap
before calling `BNCreateSymbol` and then deallocates them
afterwards. This seems unnecessary, and adds a non-trivial amount
of overhead.
This can go back to directly constructing the `Symbol` once the
constructors are improved.
|
|
view workaround
|
|
|
|
|
|
`PageMapping` was storing the path to the file it points within. This
was causing unnecessary work within `VM::MappingAtAddress` as the
`PageMapping`, and thus the path, is copied into the return value. This
copying was more expensive than the map lookup.
The file path is now stored within a `LazyMappedFileAccessor` class that
wraps the `SelfAllocatingWeakPtr`. This means the path is still
available via the `PageMapping`, but it does not need to be copied as
often.
This includes two additional improvements / optimizations while I was
touching the code in question:
1. `MMappedFileAccessor::Open` no longer performs two hash lookups when
a file accessor already exists.
2. `VM::MapPages` takes the path by const reference to avoid an
unnecessary copy.
|
|
fix compilation issue on linux
|
|
Find the relative selector base address in the Objective-C optimization
data pointed to by the shared cache header, rather than via
`__objc_scoffs`. This is only present on iOS, and not for every iOS
version that encodes selectors via direct offsets.
This also includes some related improvements:
1. Direct selectors get their own pointer type so they're rendered
correctly in the view.
2. Method lists encoded as lists of lists are now handled.
3. The `dyld_cache_header` type added to the view is truncated to the
length in the loaded cache. This ensures it is applied to the view.
4. A couple of methods that process method IMPs and selectors are
updated to check whether the address is valid before attempting to
process them. They would otherwise fail by throwing an exception if
they proceed, but checking for validity is quicker and makes
exception breakpoints usable.
|
|
`BackingCache` now tracks the `dyld_cache_mapping_info` for its mappings
so it has access to the memory protections for the region. This means it
can avoid marking some regions as containing code when they don't,
reducing the amount of analysis work that has to be done.
Using `dyld_cache_mapping_info` also makes references to mappings easier
to understand due to its named fields vs the nested `std::pair`s that
were previously in use.
|
|
They're surprisingly expensive to look up.
|
|
The existing view-specific state was stored in several global unordered
maps. Many of these were accessed without locking, including
`viewSpecificMutexes`, which is racy in the face of multiple threads.
View-specific state is stored in a new heap-allocated
`ViewSpecificState` struct that is reference counted via
`std::shared_ptr`. A static map holds a `std::weak_ptr` to each
view-specific state, keyed by session id. `SharedCache` retrieves its
view-specific state during its constructor.
Since `ViewSpecificState` is reference counted it will naturally be
deallocated when the last `SharedCache` instance that references it goes
away. Its corresponding entry will remain in the static map, though
since it only holds a `std::weak_ptr` rather than any state it will not
use much memory. The next time view-specific state is retrieved any
expired entries will be removed from the map.
|
|
`BNDSCViewGetAllImages`
|
|
A problem with processing Objective-C sections at the time a library is loaded is that some of the references within those sections may refer to unload sections. This results in things like selectors not being correctly typed and named.
This commit provides a way for users to manually trigger Objective-C parsing against sections for a specific library or all libraries, via the API. Combined with the previous commit a user can use the API to batch load a number of libraries and skip Objective-C processing for each one and then run it across the entire of the DSC once they are all loaded.
Further improvement would be to provide a way to trigger Objective-C processing through the UI.
|
|
library
This is useful when batch loading libraries to avoid extra processing (once the next commit has landed).
|
|
Prior to this commit the function `DSCObjCProcessor::PostProcessObjCSections` never does anything because it doesn't use the correct names to get the Objective-C sections of the recently loaded library. In fact it never does anything because the DSC never has sections with the names its searching for.
This commit passes the `baseName` (the name of the library that was loaded), which is what other Objective-C section processing code does. Combining the base name with the section names it will now find them and process them as intended. This was resulting in alot of Objective-C related stuff being missed.
There is however still an issue of the fact that the way this DSC plugin works means it only analyzes Objective-C sections once. This catches alot of things but there are a number of cases where other libraries need to be loaded first due to information being referenced in another library. For instance errors like `Failed to determine base classname for category` can be caused by the class reference in the category being to a class outside of the loaded library. Once the library containing the class has been loaded, the section containing the category should be re-proccessed.
|
|
From what I can tell the lock being taken in `SharedCache::~SharedCache` was purely for the decrement of `sharedCacheReferences`, however its an atomic so a lock isn't necessary. The lock being taken is extremely contentious and therefore often slow to be acquired. This resulted in a surprising amount of execution time spent in the `SharedCache` destructor. Nothing hugely significant but a quick and easy win to remove this single line of code.
|
|
`SharedCache::m_symbolInfos` isn't being serialized but there is an attempt to deserialize it. This commit adds in the code to serialize it.
I slightly modified the format because I didn't really understand how it was expected to be serialized based on the deserialization code. The deserialization code looked wrong to me but its likely a misunderstanding on my part. I kept it similar to how `m_exportInfos` is serialized.
|
|
There seem to be a number of issues with slide info parsing. I decided the simplest fix was to largely mimick what `dyld` is doing.
Part of this process was to remove creating a vector of page starts and processing them in another loop below the one where they were read out. It wasn't clear to me the original design decision to separate it into 2 loops like that. I think this was part of the problem that was causing issues.
By adding rewrites in the same loop where page starts are being read out, it was much easier to mimick the code in `dyld` which I assume has to be correct. So as long as my copying was correct then I believe this should work as intended.
Not everything has been thoroughly tested but I'm pretty confident v3 and v5 are now working as intended. v2 should be but less testing of it has been done.
|
