From 8953660bbbd0fc8f15be004f3302886f34f620df Mon Sep 17 00:00:00 2001 From: WeiN76LQh Date: Mon, 25 Nov 2024 19:34:36 +0000 Subject: [PATCH 1/5] [SharedCache] Use `m_exportInfos` as an export list cache `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. --- view/sharedcache/core/SharedCache.cpp | 124 ++++++++++++++++---------- view/sharedcache/core/SharedCache.h | 3 + 2 files changed, 78 insertions(+), 49 deletions(-) diff --git a/view/sharedcache/core/SharedCache.cpp b/view/sharedcache/core/SharedCache.cpp index 0ba4f184d4..d593303907 100644 --- a/view/sharedcache/core/SharedCache.cpp +++ b/view/sharedcache/core/SharedCache.cpp @@ -2665,33 +2665,34 @@ void SharedCache::InitializeHeader( if (header.exportTriePresent && header.linkeditPresent && vm->AddressIsMapped(header.linkeditSegment.vmaddr)) { - auto symbols = SharedCache::ParseExportTrie(vm->MappingAtAddress(header.linkeditSegment.vmaddr).first.fileAccessor->lock(), header); - std::vector>> exportMapping; + auto symbols = GetExportListForHeader(header, [&]() { + return vm->MappingAtAddress(header.linkeditSegment.vmaddr).first.fileAccessor->lock(); + }); for (const auto& symbol : symbols) { - exportMapping.push_back({symbol->GetAddress(), {symbol->GetType(), symbol->GetRawName()}}); + auto bnSymbol = new Symbol(symbol.second.first, symbol.second.second, symbol.first); if (typeLib) { - auto type = m_dscView->ImportTypeLibraryObject(typeLib, {symbol->GetFullName()}); + auto type = m_dscView->ImportTypeLibraryObject(typeLib, {symbol.second.second}); if (type) { - view->DefineAutoSymbolAndVariableOrFunction(view->GetDefaultPlatform(), symbol, type); + view->DefineAutoSymbolAndVariableOrFunction(view->GetDefaultPlatform(), bnSymbol, type); } else - view->DefineAutoSymbol(symbol); + view->DefineAutoSymbol(bnSymbol); - if (view->GetAnalysisFunction(view->GetDefaultPlatform(), symbol->GetAddress())) + if (view->GetAnalysisFunction(view->GetDefaultPlatform(), symbol.first)) { - auto func = view->GetAnalysisFunction(view->GetDefaultPlatform(), symbol->GetAddress()); - if (symbol->GetFullName() == "_objc_msgSend") + auto func = view->GetAnalysisFunction(view->GetDefaultPlatform(), symbol.first); + if (symbol.second.second == "_objc_msgSend") { func->SetHasVariableArguments(false); } - else if (symbol->GetFullName().find("_objc_retain_x") != std::string::npos || symbol->GetFullName().find("_objc_release_x") != std::string::npos) + else if (symbol.second.second.find("_objc_retain_x") != std::string::npos || symbol.second.second.find("_objc_release_x") != std::string::npos) { - auto x = symbol->GetFullName().rfind("x"); - auto num = symbol->GetFullName().substr(x + 1); + auto x = symbol.second.second.rfind("x"); + auto num = symbol.second.second.substr(x + 1); std::vector callTypeParams; auto cc = m_dscView->GetDefaultArchitecture()->GetCallingConventionByName("apple-arm64-objc-fast-arc-" + num); @@ -2704,9 +2705,8 @@ void SharedCache::InitializeHeader( } } else - view->DefineAutoSymbol(symbol); + view->DefineAutoSymbol(bnSymbol); } - MutableState().exportInfos[header.textBase] = std::move(exportMapping); } view->EndBulkModifySymbols(); @@ -2794,6 +2794,7 @@ std::vector> SharedCache::ParseExportTrie(std::shared_ptr> symbols; auto [begin, end] = linkeditFile->ReadSpan(header.exportTrie.dataoff, header.exportTrie.datasize); ReadExportNode(symbols, header, begin, end, begin, header.textBase, ""); @@ -2806,6 +2807,32 @@ std::vector> SharedCache::ParseExportTrie(std::shared_ptr>> SharedCache::GetExportListForHeader(SharedCacheMachOHeader header, std::function()> provideLinkeditFile) +{ + if (auto it = m_state->exportInfos.find(header.textBase); it != m_state->exportInfos.end()) + { + return it->second; + } + else + { + // TODO does this have to be a functor? can't we just pass the accessor? if not, why? + std::shared_ptr linkeditFile = provideLinkeditFile(); + if (!linkeditFile) + return std::vector>>(); + + auto exportList = SharedCache::ParseExportTrie(linkeditFile, header); + std::vector>> exportMapping(exportList.size()); + for (const auto& sym : exportList) + { + exportMapping.push_back({sym->GetAddress(), {sym->GetType(), sym->GetRawName()}}); + } + m_state->exportInfos[header.textBase] = exportMapping; + return exportMapping; + } +} + + std::vector SharedCache::GetAvailableImages() { std::vector installNames; @@ -2823,30 +2850,32 @@ std::vector>> SharedCache::LoadAllSymbolsAndW std::lock_guard initialLoadBlock(m_viewSpecificState->viewOperationsThatInfluenceMetadataMutex); + bool doSave = false; std::vector>> symbols; for (const auto& img : State().images) { auto header = HeaderForAddress(img.headerLocation); - std::shared_ptr mapping; - try { - mapping = MMappedFileAccessor::Open(m_dscView, m_dscView->GetFile()->GetSessionId(), header->exportTriePath)->lock(); - } - catch (...) - { - m_logger->LogWarn("Serious Error: Failed to open export trie %s for %s", header->exportTriePath.c_str(), header->installName.c_str()); - continue; - } - auto exportList = SharedCache::ParseExportTrie(mapping, *header); - std::vector>> exportMapping; + auto exportList = GetExportListForHeader(*header, [&]() { + try { + auto mapping = MMappedFileAccessor::Open(m_dscView, m_dscView->GetFile()->GetSessionId(), header->exportTriePath)->lock(); + doSave = true; + return mapping; + } + catch (...) + { + m_logger->LogWarn("Serious Error: Failed to open export trie %s for %s", header->exportTriePath.c_str(), header->installName.c_str()); + return std::shared_ptr(nullptr); + } + }); for (const auto& sym : exportList) { - exportMapping.push_back({sym->GetAddress(), {sym->GetType(), sym->GetRawName()}}); - symbols.push_back({img.installName, sym}); + symbols.push_back({img.installName, new Symbol(sym.second.first, sym.second.second, sym.first)}); } - MutableState().exportInfos[header->textBase] = std::move(exportMapping); } - SaveToDSCView(); + // Only save to DSC view if a header was actually loaded + if (doSave) + SaveToDSCView(); return symbols; } @@ -2926,41 +2955,42 @@ void SharedCache::FindSymbolAtAddrAndApplyToAddr( auto header = HeaderForAddress(symbolLocation); if (header) { - std::shared_ptr mapping; - try { - mapping = MMappedFileAccessor::Open(m_dscView, m_dscView->GetFile()->GetSessionId(), header->exportTriePath)->lock(); - } - catch (...) - { - m_logger->LogWarn("Serious Error: Failed to open export trie for %s", header->installName.c_str()); - return; - } - auto exportList = SharedCache::ParseExportTrie(mapping, *header); + + auto exportList = GetExportListForHeader(*header, [&]() { + try { + return MMappedFileAccessor::Open(m_dscView, m_dscView->GetFile()->GetSessionId(), header->exportTriePath)->lock(); + } + catch (...) + { + m_logger->LogWarn("Serious Error: Failed to open export trie %s for %s", header->exportTriePath.c_str(), header->installName.c_str()); + return std::shared_ptr(nullptr); + } + }); + std::vector>> exportMapping; auto typeLib = TypeLibraryForImage(header->installName); id = m_dscView->BeginUndoActions(); m_dscView->BeginBulkModifySymbols(); for (const auto& sym : exportList) { - exportMapping.push_back({sym->GetAddress(), {sym->GetType(), sym->GetRawName()}}); - if (sym->GetAddress() == symbolLocation) + if (sym.first == symbolLocation) { if (auto func = m_dscView->GetAnalysisFunction(m_dscView->GetDefaultPlatform(), targetLocation)) { m_dscView->DefineUserSymbol( - new Symbol(FunctionSymbol, prefix + sym->GetFullName(), targetLocation)); + new Symbol(FunctionSymbol, prefix + sym.second.second, targetLocation)); if (typeLib) - if (auto type = m_dscView->ImportTypeLibraryObject(typeLib, {sym->GetFullName()})) + if (auto type = m_dscView->ImportTypeLibraryObject(typeLib, {sym.second.second})) func->SetUserType(type); } else { m_dscView->DefineUserSymbol( - new Symbol(sym->GetType(), prefix + sym->GetFullName(), targetLocation)); + new Symbol(sym.second.first, prefix + sym.second.second, targetLocation)); if (typeLib) - if (auto type = m_dscView->ImportTypeLibraryObject(typeLib, {sym->GetFullName()})) + if (auto type = m_dscView->ImportTypeLibraryObject(typeLib, {sym.second.second})) m_dscView->DefineUserDataVariable(targetLocation, type); } if (triggerReanalysis) @@ -2972,10 +3002,6 @@ void SharedCache::FindSymbolAtAddrAndApplyToAddr( break; } } - { - std::lock_guard lock(m_viewSpecificState->viewOperationsThatInfluenceMetadataMutex); - MutableState().exportInfos[header->textBase] = std::move(exportMapping); - } m_dscView->EndBulkModifySymbols(); m_dscView->ForgetUndoActions(id); } diff --git a/view/sharedcache/core/SharedCache.h b/view/sharedcache/core/SharedCache.h index 8c06ff6e40..0b268e3d6b 100644 --- a/view/sharedcache/core/SharedCache.h +++ b/view/sharedcache/core/SharedCache.h @@ -642,6 +642,9 @@ namespace SharedCacheCore { std::vector> ParseExportTrie( std::shared_ptr linkeditFile, const SharedCacheMachOHeader& header); + std::vector>> GetExportListForHeader( + SharedCacheMachOHeader header, std::function()> provideLinkeditFile); + Ref TypeLibraryForImage(const std::string& installName); size_t GetBaseAddress() const; From e90a08f5a6237472a3d0caf77e11dfcc987f1169 Mon Sep 17 00:00:00 2001 From: WeiN76LQh Date: Tue, 26 Nov 2024 16:06:30 +0000 Subject: [PATCH 2/5] [SharedCache] Add parameter to `SharedCache::InitializeHeader` to check if `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. --- view/sharedcache/core/SharedCache.cpp | 13 ++++++++++--- view/sharedcache/core/SharedCache.h | 3 ++- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/view/sharedcache/core/SharedCache.cpp b/view/sharedcache/core/SharedCache.cpp index d593303907..64afeee73d 100644 --- a/view/sharedcache/core/SharedCache.cpp +++ b/view/sharedcache/core/SharedCache.cpp @@ -2808,10 +2808,12 @@ std::vector> SharedCache::ParseExportTrie(std::shared_ptr>> SharedCache::GetExportListForHeader(SharedCacheMachOHeader header, std::function()> provideLinkeditFile) +std::vector>> SharedCache::GetExportListForHeader(SharedCacheMachOHeader header, std::function()> provideLinkeditFile, bool* didModifyExportList) { if (auto it = m_state->exportInfos.find(header.textBase); it != m_state->exportInfos.end()) { + if (didModifyExportList) + *didModifyExportList = false; return it->second; } else @@ -2819,7 +2821,11 @@ std::vector>> SharedCac // TODO does this have to be a functor? can't we just pass the accessor? if not, why? std::shared_ptr linkeditFile = provideLinkeditFile(); if (!linkeditFile) + { + if (didModifyExportList) + *didModifyExportList = false; return std::vector>>(); + } auto exportList = SharedCache::ParseExportTrie(linkeditFile, header); std::vector>> exportMapping(exportList.size()); @@ -2828,6 +2834,8 @@ std::vector>> SharedCac exportMapping.push_back({sym->GetAddress(), {sym->GetType(), sym->GetRawName()}}); } m_state->exportInfos[header.textBase] = exportMapping; + if (didModifyExportList) + *didModifyExportList = true; return exportMapping; } } @@ -2858,7 +2866,6 @@ std::vector>> SharedCache::LoadAllSymbolsAndW auto exportList = GetExportListForHeader(*header, [&]() { try { auto mapping = MMappedFileAccessor::Open(m_dscView, m_dscView->GetFile()->GetSessionId(), header->exportTriePath)->lock(); - doSave = true; return mapping; } catch (...) @@ -2866,7 +2873,7 @@ std::vector>> SharedCache::LoadAllSymbolsAndW m_logger->LogWarn("Serious Error: Failed to open export trie %s for %s", header->exportTriePath.c_str(), header->installName.c_str()); return std::shared_ptr(nullptr); } - }); + }, &doSave); for (const auto& sym : exportList) { symbols.push_back({img.installName, new Symbol(sym.second.first, sym.second.second, sym.first)}); diff --git a/view/sharedcache/core/SharedCache.h b/view/sharedcache/core/SharedCache.h index 0b268e3d6b..bb1181b699 100644 --- a/view/sharedcache/core/SharedCache.h +++ b/view/sharedcache/core/SharedCache.h @@ -643,7 +643,8 @@ namespace SharedCacheCore { std::shared_ptr linkeditFile, const SharedCacheMachOHeader& header); std::vector>> GetExportListForHeader( - SharedCacheMachOHeader header, std::function()> provideLinkeditFile); + SharedCacheMachOHeader header, std::function()> provideLinkeditFile, + bool* didModifyExportList = nullptr); Ref TypeLibraryForImage(const std::string& installName); From 28aef35c76c4b6c9215aeb82f974032031154bd1 Mon Sep 17 00:00:00 2001 From: WeiN76LQh Date: Tue, 26 Nov 2024 16:15:58 +0000 Subject: [PATCH 3/5] [SharedCache] Improve the types for `m_exportInfos` 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`. --- view/sharedcache/core/SharedCache.cpp | 97 +++++++++++++-------------- view/sharedcache/core/SharedCache.h | 6 +- 2 files changed, 49 insertions(+), 54 deletions(-) diff --git a/view/sharedcache/core/SharedCache.cpp b/view/sharedcache/core/SharedCache.cpp index 64afeee73d..61b1822369 100644 --- a/view/sharedcache/core/SharedCache.cpp +++ b/view/sharedcache/core/SharedCache.cpp @@ -57,7 +57,7 @@ int count_trailing_zeros(uint64_t value) { struct SharedCache::State { - std::unordered_map>>> + std::unordered_map>> exportInfos; std::unordered_map>>> symbolInfos; @@ -2668,31 +2668,31 @@ void SharedCache::InitializeHeader( auto symbols = GetExportListForHeader(header, [&]() { return vm->MappingAtAddress(header.linkeditSegment.vmaddr).first.fileAccessor->lock(); }); - for (const auto& symbol : symbols) + for (const auto& symPair : symbols) { - auto bnSymbol = new Symbol(symbol.second.first, symbol.second.second, symbol.first); if (typeLib) { - auto type = m_dscView->ImportTypeLibraryObject(typeLib, {symbol.second.second}); + auto type = m_dscView->ImportTypeLibraryObject(typeLib, symPair.second->GetRawName()); if (type) { - view->DefineAutoSymbolAndVariableOrFunction(view->GetDefaultPlatform(), bnSymbol, type); + view->DefineAutoSymbolAndVariableOrFunction(view->GetDefaultPlatform(), symPair.second, type); } else - view->DefineAutoSymbol(bnSymbol); + view->DefineAutoSymbol(symPair.second); - if (view->GetAnalysisFunction(view->GetDefaultPlatform(), symbol.first)) + if (view->GetAnalysisFunction(view->GetDefaultPlatform(), symPair.first)) { - auto func = view->GetAnalysisFunction(view->GetDefaultPlatform(), symbol.first); - if (symbol.second.second == "_objc_msgSend") + auto func = view->GetAnalysisFunction(view->GetDefaultPlatform(), symPair.first); + auto name = symPair.second->GetFullName(); + if (name == "_objc_msgSend") { func->SetHasVariableArguments(false); } - else if (symbol.second.second.find("_objc_retain_x") != std::string::npos || symbol.second.second.find("_objc_release_x") != std::string::npos) + else if (name.find("_objc_retain_x") != std::string::npos || name.find("_objc_release_x") != std::string::npos) { - auto x = symbol.second.second.rfind("x"); - auto num = symbol.second.second.substr(x + 1); + auto x = name.rfind("x"); + auto num = name.substr(x + 1); std::vector callTypeParams; auto cc = m_dscView->GetDefaultArchitecture()->GetCallingConventionByName("apple-arm64-objc-fast-arc-" + num); @@ -2705,7 +2705,7 @@ void SharedCache::InitializeHeader( } } else - view->DefineAutoSymbol(bnSymbol); + view->DefineAutoSymbol(symPair.second); } } view->EndBulkModifySymbols(); @@ -2808,7 +2808,7 @@ std::vector> SharedCache::ParseExportTrie(std::shared_ptr>> SharedCache::GetExportListForHeader(SharedCacheMachOHeader header, std::function()> provideLinkeditFile, bool* didModifyExportList) +std::unordered_map> SharedCache::GetExportListForHeader(SharedCacheMachOHeader header, std::function()> provideLinkeditFile, bool* didModifyExportList) { if (auto it = m_state->exportInfos.find(header.textBase); it != m_state->exportInfos.end()) { @@ -2824,14 +2824,14 @@ std::vector>> SharedCac { if (didModifyExportList) *didModifyExportList = false; - return std::vector>>(); + return std::unordered_map>(); } auto exportList = SharedCache::ParseExportTrie(linkeditFile, header); - std::vector>> exportMapping(exportList.size()); + std::unordered_map> exportMapping(exportList.size()); for (const auto& sym : exportList) { - exportMapping.push_back({sym->GetAddress(), {sym->GetType(), sym->GetRawName()}}); + exportMapping[sym->GetAddress()] = sym; } m_state->exportInfos[header.textBase] = exportMapping; if (didModifyExportList) @@ -2874,9 +2874,9 @@ std::vector>> SharedCache::LoadAllSymbolsAndW return std::shared_ptr(nullptr); } }, &doSave); - for (const auto& sym : exportList) + for (const auto& symPair : exportList) { - symbols.push_back({img.installName, new Symbol(sym.second.first, sym.second.second, sym.first)}); + symbols.push_back({img.installName, symPair.second}); } } @@ -2978,35 +2978,31 @@ void SharedCache::FindSymbolAtAddrAndApplyToAddr( auto typeLib = TypeLibraryForImage(header->installName); id = m_dscView->BeginUndoActions(); m_dscView->BeginBulkModifySymbols(); - for (const auto& sym : exportList) + if (auto it = exportList.find(symbolLocation); it != exportList.end()) { - if (sym.first == symbolLocation) + if (auto func = m_dscView->GetAnalysisFunction(m_dscView->GetDefaultPlatform(), targetLocation)) { - if (auto func = m_dscView->GetAnalysisFunction(m_dscView->GetDefaultPlatform(), targetLocation)) - { - m_dscView->DefineUserSymbol( - new Symbol(FunctionSymbol, prefix + sym.second.second, targetLocation)); + m_dscView->DefineUserSymbol( + new Symbol(FunctionSymbol, prefix + it->second->GetFullName(), targetLocation)); - if (typeLib) - if (auto type = m_dscView->ImportTypeLibraryObject(typeLib, {sym.second.second})) - func->SetUserType(type); - } - else - { - m_dscView->DefineUserSymbol( - new Symbol(sym.second.first, prefix + sym.second.second, targetLocation)); + if (typeLib) + if (auto type = m_dscView->ImportTypeLibraryObject(typeLib, {it->second->GetFullName()})) + func->SetUserType(type); + } + else + { + m_dscView->DefineUserSymbol( + new Symbol(it->second->GetType(), prefix + it->second->GetFullName(), targetLocation)); - if (typeLib) - if (auto type = m_dscView->ImportTypeLibraryObject(typeLib, {sym.second.second})) - m_dscView->DefineUserDataVariable(targetLocation, type); - } - if (triggerReanalysis) - { - auto func = m_dscView->GetAnalysisFunction(m_dscView->GetDefaultPlatform(), targetLocation); - if (func) - func->Reanalyze(); - } - break; + if (typeLib) + if (auto type = m_dscView->ImportTypeLibraryObject(typeLib, {it->second->GetFullName()})) + m_dscView->DefineUserDataVariable(targetLocation, type); + } + if (triggerReanalysis) + { + auto func = m_dscView->GetAnalysisFunction(m_dscView->GetDefaultPlatform(), targetLocation); + if (func) + func->Reanalyze(); } } m_dscView->EndBulkModifySymbols(); @@ -3474,8 +3470,8 @@ void SharedCache::Store(SerializationContext& context) const { context.writer.StartObject(); Serialize(context, "key", pair2.first); - Serialize(context, "val1", pair2.second.first); - Serialize(context, "val2", pair2.second.second); + Serialize(context, "val1", pair2.second->GetType()); + Serialize(context, "val2", pair2.second->GetRawName()); context.writer.EndObject(); } context.writer.EndArray(); @@ -3546,12 +3542,13 @@ void SharedCache::Load(DeserializationContext& context) for (const auto& obj1 : context.doc["exportInfos"].GetArray()) { - std::vector>> innerVec; + std::unordered_map> innerVec; for (const auto& obj2 : obj1["value"].GetArray()) { - std::pair innerPair = { - (BNSymbolType)obj2["val1"].GetUint64(), obj2["val2"].GetString()}; - innerVec.push_back({obj2["key"].GetUint64(), innerPair}); + std::string raw = obj2["val2"].GetString(); + uint64_t addr = obj2["key"].GetUint64(); + innerVec[addr] = new Symbol(BNCreateSymbol((BNSymbolType)obj2["val1"].GetUint64(), raw.c_str(), + raw.c_str(), raw.c_str(), addr, NoBinding, nullptr, 0)); } MutableState().exportInfos[obj1["key"].GetUint64()] = std::move(innerVec); diff --git a/view/sharedcache/core/SharedCache.h b/view/sharedcache/core/SharedCache.h index bb1181b699..fd976ed745 100644 --- a/view/sharedcache/core/SharedCache.h +++ b/view/sharedcache/core/SharedCache.h @@ -567,6 +567,7 @@ namespace SharedCacheCore { struct ViewSpecificState; + private: Ref m_logger; /* VIEW STATE BEGIN -- SERIALIZE ALL OF THIS AND STORE IT IN RAW VIEW */ @@ -641,10 +642,7 @@ namespace SharedCacheCore { const uint8_t *end, const uint8_t* current, uint64_t textBase, const std::string& currentText); std::vector> ParseExportTrie( std::shared_ptr linkeditFile, const SharedCacheMachOHeader& header); - - std::vector>> GetExportListForHeader( - SharedCacheMachOHeader header, std::function()> provideLinkeditFile, - bool* didModifyExportList = nullptr); + std::unordered_map> GetExportListForHeader(SharedCacheMachOHeader header, std::function()> provideLinkeditFile, bool* didModifyExportList = nullptr); Ref TypeLibraryForImage(const std::string& installName); From 876a5e2f8eeea13dd4aed5bbf7abc853727dcc85 Mon Sep 17 00:00:00 2001 From: WeiN76LQh Date: Tue, 26 Nov 2024 16:35:55 +0000 Subject: [PATCH 4/5] [SharedCache] Only setup undo actions and bulk modify in `SharedCache::FindSymbolAtAddrAndApplyToAddr` if a symbol is found --- view/sharedcache/core/SharedCache.cpp | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/view/sharedcache/core/SharedCache.cpp b/view/sharedcache/core/SharedCache.cpp index 61b1822369..c7591c3a19 100644 --- a/view/sharedcache/core/SharedCache.cpp +++ b/view/sharedcache/core/SharedCache.cpp @@ -2974,13 +2974,14 @@ void SharedCache::FindSymbolAtAddrAndApplyToAddr( } }); - std::vector>> exportMapping; - auto typeLib = TypeLibraryForImage(header->installName); - id = m_dscView->BeginUndoActions(); - m_dscView->BeginBulkModifySymbols(); if (auto it = exportList.find(symbolLocation); it != exportList.end()) { - if (auto func = m_dscView->GetAnalysisFunction(m_dscView->GetDefaultPlatform(), targetLocation)) + auto typeLib = TypeLibraryForImage(header->installName); + id = m_dscView->BeginUndoActions(); + m_dscView->BeginBulkModifySymbols(); + + auto func = m_dscView->GetAnalysisFunction(m_dscView->GetDefaultPlatform(), targetLocation); + if (func) { m_dscView->DefineUserSymbol( new Symbol(FunctionSymbol, prefix + it->second->GetFullName(), targetLocation)); @@ -3000,13 +3001,13 @@ void SharedCache::FindSymbolAtAddrAndApplyToAddr( } if (triggerReanalysis) { - auto func = m_dscView->GetAnalysisFunction(m_dscView->GetDefaultPlatform(), targetLocation); if (func) func->Reanalyze(); } + + m_dscView->EndBulkModifySymbols(); + m_dscView->ForgetUndoActions(id); } - m_dscView->EndBulkModifySymbols(); - m_dscView->ForgetUndoActions(id); } } From 0f0801ac9fc2196b568bd6c4a5fb3f16a928c2d3 Mon Sep 17 00:00:00 2001 From: WeiN76LQh Date: Tue, 26 Nov 2024 18:21:14 +0000 Subject: [PATCH 5/5] [SharedCache] Make `m_exportInfos` map's values a `shared_ptr` 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. --- view/sharedcache/core/SharedCache.cpp | 136 ++++++++++++++------------ view/sharedcache/core/SharedCache.h | 4 +- 2 files changed, 75 insertions(+), 65 deletions(-) diff --git a/view/sharedcache/core/SharedCache.cpp b/view/sharedcache/core/SharedCache.cpp index c7591c3a19..6eebc40901 100644 --- a/view/sharedcache/core/SharedCache.cpp +++ b/view/sharedcache/core/SharedCache.cpp @@ -57,7 +57,7 @@ int count_trailing_zeros(uint64_t value) { struct SharedCache::State { - std::unordered_map>> + std::unordered_map>>> exportInfos; std::unordered_map>>> symbolInfos; @@ -2668,44 +2668,47 @@ void SharedCache::InitializeHeader( auto symbols = GetExportListForHeader(header, [&]() { return vm->MappingAtAddress(header.linkeditSegment.vmaddr).first.fileAccessor->lock(); }); - for (const auto& symPair : symbols) + if (symbols) { - if (typeLib) + for (const auto& [symbolAddress, symbol] : *symbols) { - auto type = m_dscView->ImportTypeLibraryObject(typeLib, symPair.second->GetRawName()); - - if (type) + if (typeLib) { - view->DefineAutoSymbolAndVariableOrFunction(view->GetDefaultPlatform(), symPair.second, type); - } - else - view->DefineAutoSymbol(symPair.second); + auto type = m_dscView->ImportTypeLibraryObject(typeLib, symbol->GetRawName()); - if (view->GetAnalysisFunction(view->GetDefaultPlatform(), symPair.first)) - { - auto func = view->GetAnalysisFunction(view->GetDefaultPlatform(), symPair.first); - auto name = symPair.second->GetFullName(); - if (name == "_objc_msgSend") + if (type) { - func->SetHasVariableArguments(false); + view->DefineAutoSymbolAndVariableOrFunction(view->GetDefaultPlatform(), symbol, type); } - else if (name.find("_objc_retain_x") != std::string::npos || name.find("_objc_release_x") != std::string::npos) + else + view->DefineAutoSymbol(symbol); + + if (view->GetAnalysisFunction(view->GetDefaultPlatform(), symbolAddress)) { - auto x = name.rfind("x"); - auto num = name.substr(x + 1); + auto func = view->GetAnalysisFunction(view->GetDefaultPlatform(), symbolAddress); + auto name = symbol->GetFullName(); + if (name == "_objc_msgSend") + { + func->SetHasVariableArguments(false); + } + else if (name.find("_objc_retain_x") != std::string::npos || name.find("_objc_release_x") != std::string::npos) + { + auto x = name.rfind("x"); + auto num = name.substr(x + 1); - std::vector callTypeParams; - auto cc = m_dscView->GetDefaultArchitecture()->GetCallingConventionByName("apple-arm64-objc-fast-arc-" + num); + std::vector callTypeParams; + auto cc = m_dscView->GetDefaultArchitecture()->GetCallingConventionByName("apple-arm64-objc-fast-arc-" + num); - callTypeParams.push_back({"obj", m_dscView->GetTypeByName({ "id" }), true, BinaryNinja::Variable()}); + callTypeParams.push_back({"obj", m_dscView->GetTypeByName({ "id" }), true, BinaryNinja::Variable()}); - auto funcType = BinaryNinja::Type::FunctionType(m_dscView->GetTypeByName({ "id" }), cc, callTypeParams); - func->SetUserType(funcType); + auto funcType = BinaryNinja::Type::FunctionType(m_dscView->GetTypeByName({ "id" }), cc, callTypeParams); + func->SetUserType(funcType); + } } } + else + view->DefineAutoSymbol(symbol); } - else - view->DefineAutoSymbol(symPair.second); } } view->EndBulkModifySymbols(); @@ -2808,7 +2811,7 @@ std::vector> SharedCache::ParseExportTrie(std::shared_ptr> SharedCache::GetExportListForHeader(SharedCacheMachOHeader header, std::function()> provideLinkeditFile, bool* didModifyExportList) +std::shared_ptr>> SharedCache::GetExportListForHeader(SharedCacheMachOHeader header, std::function()> provideLinkeditFile, bool* didModifyExportList) { if (auto it = m_state->exportInfos.find(header.textBase); it != m_state->exportInfos.end()) { @@ -2824,19 +2827,19 @@ std::unordered_map> SharedCache::GetExportListForHeader(Sh { if (didModifyExportList) *didModifyExportList = false; - return std::unordered_map>(); + return nullptr; } auto exportList = SharedCache::ParseExportTrie(linkeditFile, header); - std::unordered_map> exportMapping(exportList.size()); + auto exportMapping = std::make_shared>>(exportList.size()); for (const auto& sym : exportList) { - exportMapping[sym->GetAddress()] = sym; + exportMapping->insert_or_assign(sym->GetAddress(), sym); } - m_state->exportInfos[header.textBase] = exportMapping; + MutableState().exportInfos.emplace(header.textBase, exportMapping); if (didModifyExportList) *didModifyExportList = true; - return exportMapping; + return m_state->exportInfos[header.textBase]; } } @@ -2874,9 +2877,11 @@ std::vector>> SharedCache::LoadAllSymbolsAndW return std::shared_ptr(nullptr); } }, &doSave); - for (const auto& symPair : exportList) + if (!exportList) + continue; + for (const auto& [_, symbol] : *exportList) { - symbols.push_back({img.installName, symPair.second}); + symbols.push_back({img.installName, symbol}); } } @@ -2974,39 +2979,42 @@ void SharedCache::FindSymbolAtAddrAndApplyToAddr( } }); - if (auto it = exportList.find(symbolLocation); it != exportList.end()) + if (exportList) { - auto typeLib = TypeLibraryForImage(header->installName); - id = m_dscView->BeginUndoActions(); - m_dscView->BeginBulkModifySymbols(); - - auto func = m_dscView->GetAnalysisFunction(m_dscView->GetDefaultPlatform(), targetLocation); - if (func) - { - m_dscView->DefineUserSymbol( - new Symbol(FunctionSymbol, prefix + it->second->GetFullName(), targetLocation)); - - if (typeLib) - if (auto type = m_dscView->ImportTypeLibraryObject(typeLib, {it->second->GetFullName()})) - func->SetUserType(type); - } - else + if (auto it = exportList->find(symbolLocation); it != exportList->end()) { - m_dscView->DefineUserSymbol( - new Symbol(it->second->GetType(), prefix + it->second->GetFullName(), targetLocation)); + auto typeLib = TypeLibraryForImage(header->installName); + id = m_dscView->BeginUndoActions(); + m_dscView->BeginBulkModifySymbols(); - if (typeLib) - if (auto type = m_dscView->ImportTypeLibraryObject(typeLib, {it->second->GetFullName()})) - m_dscView->DefineUserDataVariable(targetLocation, type); - } - if (triggerReanalysis) - { + auto func = m_dscView->GetAnalysisFunction(m_dscView->GetDefaultPlatform(), targetLocation); if (func) - func->Reanalyze(); - } + { + m_dscView->DefineUserSymbol( + new Symbol(FunctionSymbol, prefix + it->second->GetFullName(), targetLocation)); + + if (typeLib) + if (auto type = m_dscView->ImportTypeLibraryObject(typeLib, {it->second->GetFullName()})) + func->SetUserType(type); + } + else + { + m_dscView->DefineUserSymbol( + new Symbol(it->second->GetType(), prefix + it->second->GetFullName(), targetLocation)); - m_dscView->EndBulkModifySymbols(); - m_dscView->ForgetUndoActions(id); + if (typeLib) + if (auto type = m_dscView->ImportTypeLibraryObject(typeLib, {it->second->GetFullName()})) + m_dscView->DefineUserDataVariable(targetLocation, type); + } + if (triggerReanalysis) + { + if (func) + func->Reanalyze(); + } + + m_dscView->EndBulkModifySymbols(); + m_dscView->ForgetUndoActions(id); + } } } } @@ -3467,7 +3475,7 @@ void SharedCache::Store(SerializationContext& context) const Serialize(context, "key", pair1.first); Serialize(context, "value"); context.writer.StartArray(); - for (const auto& pair2 : pair1.second) + for (const auto& pair2 : *pair1.second) { context.writer.StartObject(); Serialize(context, "key", pair2.first); @@ -3552,7 +3560,7 @@ void SharedCache::Load(DeserializationContext& context) raw.c_str(), raw.c_str(), addr, NoBinding, nullptr, 0)); } - MutableState().exportInfos[obj1["key"].GetUint64()] = std::move(innerVec); + MutableState().exportInfos[obj1["key"].GetUint64()] = std::make_shared>>(innerVec); } for (auto& symbolInfo : context.doc["symbolInfos"].GetArray()) diff --git a/view/sharedcache/core/SharedCache.h b/view/sharedcache/core/SharedCache.h index fd976ed745..2f6f2ae45e 100644 --- a/view/sharedcache/core/SharedCache.h +++ b/view/sharedcache/core/SharedCache.h @@ -642,7 +642,9 @@ namespace SharedCacheCore { const uint8_t *end, const uint8_t* current, uint64_t textBase, const std::string& currentText); std::vector> ParseExportTrie( std::shared_ptr linkeditFile, const SharedCacheMachOHeader& header); - std::unordered_map> GetExportListForHeader(SharedCacheMachOHeader header, std::function()> provideLinkeditFile, bool* didModifyExportList = nullptr); + std::shared_ptr>> GetExportListForHeader(SharedCacheMachOHeader header, + std::function()> provideLinkeditFile, bool* didModifyExportList = nullptr); + Ref TypeLibraryForImage(const std::string& installName);