From 10fb40648c03bdaf80a758450fba3e3678a9a4f6 Mon Sep 17 00:00:00 2001 From: WeiN76LQh Date: Sun, 23 Mar 2025 19:18:23 +0000 Subject: [PATCH 1/3] [SharedCache] Avoid data memory region name conflicts & reduce section naming ones The problems this commit addresses is that Binary Ninja does not allow for sections with duplicate names; the sections are added but they are unnamed. It also does not allow memory map data regions to be added with the same name and will throw an error when attempting to do so. Prior to this commit the DSC plugin would use the base name of the image path plus the segment name, as a name for a segment region when it is added to the binary view memory map when loading an image. The problem with this is that some images in the DSC share base names, i.e. `TSUtility`. To avoid this conflict the full image path is used followed by the segment name for memory map data region names. Additionally this same problem occurs with sections. Prior to this commit the DSC plugin used the image base name plus the section name, however even within an image there can be multiple sections with the same name. This commit changes it so that the segment name is also included in the section name. Analysis shows that images do not contain duplicate named segments and segments do not contain duplicate named sections. The only problem remaining is the issue of duplicate image base names used in section names. So section names can still be a problem if images with the same base name are loaded. I filed issue #6411 to request sections can have duplicate names. In part this commit actually mitigates half the issue that made me file the feature request but it doesn't seem unreasonable to add support for duplicate section names and it will help fully fix the remaining issue. Arguably there another solution could be provided here for duplicate image base names, like including the image index. Although that information is not particularly useful to the user, but it does differentiate the sections that belong to a given image. I don't think it makes sense to include the full image path in section names because its overly verbose although it does inform the user the most, its just too much text. --- view/sharedcache/core/SharedCache.cpp | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/view/sharedcache/core/SharedCache.cpp b/view/sharedcache/core/SharedCache.cpp index e3c115cd0f..468a17471d 100644 --- a/view/sharedcache/core/SharedCache.cpp +++ b/view/sharedcache/core/SharedCache.cpp @@ -1584,6 +1584,11 @@ bool SharedCache::LoadImageContainingAddress(uint64_t address, bool skipObjC) return false; } +static std::string GetNameForMemoryDataRegion(const SharedCacheMachOHeader& header, const std::string& regionName) +{ + return fmt::format("{}{}", header.installName.substr(0, header.installName.size() - header.identifierPrefix.size()), regionName); +} + bool SharedCache::LoadSectionAtAddress(uint64_t address) { std::lock(m_mutex, m_viewSpecificState->viewOperationsThatInfluenceMetadataMutex); @@ -1656,7 +1661,7 @@ bool SharedCache::LoadSectionAtAddress(uint64_t address) auto targetFile = vm->MappingAtAddress(targetSegment->start).first.fileAccessor->lock(); auto buff = reader.ReadBuffer(targetSegment->start, targetSegment->size); - m_dscView->GetMemoryMap()->AddDataMemoryRegion(targetSegment->prettyName, targetSegment->start, buff, targetSegment->flags); + m_dscView->GetMemoryMap()->AddDataMemoryRegion(GetNameForMemoryDataRegion(targetHeader, targetSegment->prettyName), targetSegment->start, buff, targetSegment->flags); SetMemoryRegionIsLoaded(lock, *targetSegment); @@ -1818,7 +1823,7 @@ bool SharedCache::LoadImageWithInstallName(std::lock_guard& lock, st auto targetFile = vm->MappingAtAddress(region.start).first.fileAccessor->lock(); auto buff = reader.ReadBuffer(region.start, region.size); - m_dscView->GetMemoryMap()->AddDataMemoryRegion(region.prettyName, region.start, buff, region.flags); + m_dscView->GetMemoryMap()->AddDataMemoryRegion(GetNameForMemoryDataRegion(header, region.prettyName), region.start, buff, region.flags); SetMemoryRegionIsLoaded(lock, region); regionsToLoad.push_back(®ion); @@ -2250,13 +2255,18 @@ std::optional SharedCache::LoadHeaderForAddress(std::sha for (auto& section : header.sections) { - char sectionName[17]; + char sectionName[sizeof(section.sectname)+1]; memcpy(sectionName, section.sectname, sizeof(section.sectname)); - sectionName[16] = 0; + sectionName[sizeof(sectionName)-1] = 0; + + char segmentName[sizeof(section.segname)+1]; + memcpy(segmentName, section.segname, sizeof(section.segname)); + segmentName[sizeof(segmentName)-1] = 0; + if (header.identifierPrefix.empty()) - header.sectionNames.push_back(sectionName); + header.sectionNames.push_back(fmt::format("{}.{}", segmentName, sectionName)); else - header.sectionNames.push_back(header.identifierPrefix + "::" + sectionName); + header.sectionNames.push_back(fmt::format("{}::{}.{}", header.identifierPrefix, segmentName, sectionName)); } } catch (ReadException&) From 4026c0e5f0f1de8edb581ac468310a70e9dcad09 Mon Sep 17 00:00:00 2001 From: WeiN76LQh Date: Sun, 23 Mar 2025 19:50:11 +0000 Subject: [PATCH 2/3] [SharedCache] Update the workflow to work with the new section naming scheme --- view/sharedcache/workflow/SharedCacheWorkflow.cpp | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/view/sharedcache/workflow/SharedCacheWorkflow.cpp b/view/sharedcache/workflow/SharedCacheWorkflow.cpp index fa34e2f51d..957071901d 100644 --- a/view/sharedcache/workflow/SharedCacheWorkflow.cpp +++ b/view/sharedcache/workflow/SharedCacheWorkflow.cpp @@ -127,7 +127,8 @@ void SharedCacheWorkflow::FixupStubs(Ref ctx) return; auto section = bv->GetSectionsAt(funcStart)[0]; - auto imageName = section->GetName(); + const auto sectionName = section->GetName(); + auto imageName = sectionName; // remove everything after :: auto pos = imageName.find("::"); if (pos != std::string::npos) @@ -152,7 +153,7 @@ void SharedCacheWorkflow::FixupStubs(Ref ctx) } // Processor that automatically loads the libObjC image when it encounters a stub (so we can do inlining). - if (workflowState->autoLoadObjCStubRequirements && section->GetName().find("__objc_stubs") != std::string::npos) + if (workflowState->autoLoadObjCStubRequirements && sectionName.find("__objc_stubs") != std::string::npos) { auto firstInstruction = mlil->GetInstruction(0); if (firstInstruction.operation == MLIL_TAILCALL) @@ -271,10 +272,10 @@ void SharedCacheWorkflow::FixupStubs(Ref ctx) return; } - if (section->GetName().find("::_stubs") != std::string::npos // Branch Islands (iOS 16) - || section->GetName().find("dyld_shared_cache_branch_islands") != std::string::npos // Branch Islands (iOS 11-?) - || section->GetName().find("::__stubs") != std::string::npos // Stubs (non arm64e) - || section->GetName().find("::__auth_stubs") != std::string::npos // Stubs (arm64e) + if (sectionName.find("::_stubs") != std::string::npos // Branch Islands (iOS 16) + || sectionName.find("dyld_shared_cache_branch_islands") != std::string::npos // Branch Islands (iOS 11-?) + || sectionName.find(".__stubs") != std::string::npos // Stubs (non arm64e) + || sectionName.find(".__auth_stubs") != std::string::npos // Stubs (arm64e) ) { auto firstInstruction = mlil->GetInstruction(0); From 1a946b97fd16f289e072357206b0e1ee9bf1e026 Mon Sep 17 00:00:00 2001 From: WeiN76LQh Date: Sun, 23 Mar 2025 20:58:36 +0000 Subject: [PATCH 3/3] [SharedCache] Update the Objective-C processor to work with the new section naming scheme --- view/sharedcache/core/ObjC.cpp | 24 ++++++------- view/sharedcache/core/SharedCache.cpp | 52 ++++++++++++++++----------- view/sharedcache/core/SharedCache.h | 1 + 3 files changed, 44 insertions(+), 33 deletions(-) diff --git a/view/sharedcache/core/ObjC.cpp b/view/sharedcache/core/ObjC.cpp index fa0e603c8f..359283bd45 100644 --- a/view/sharedcache/core/ObjC.cpp +++ b/view/sharedcache/core/ObjC.cpp @@ -1124,13 +1124,13 @@ void DSCObjCProcessor::ApplyMethodTypes(Class& cls) void DSCObjCProcessor::PostProcessObjCSections(VMReader* reader, std::string baseName) { auto ptrSize = m_data->GetAddressSize(); - if (auto imageInfo = m_data->GetSectionByName(baseName + "::__objc_imageinfo")) + if (auto imageInfo = m_data->GetSectionByName(baseName + ".__objc_imageinfo")) { auto start = imageInfo->GetStart(); auto type = Type::NamedType(m_data, m_typeNames.imageInfo); m_data->DefineDataVariable(start, type); } - if (auto selrefs = m_data->GetSectionByName(baseName + "::__objc_selrefs")) + if (auto selrefs = m_data->GetSectionByName(baseName + ".__objc_selrefs")) { auto start = selrefs->GetStart(); auto end = selrefs->GetEnd(); @@ -1153,7 +1153,7 @@ void DSCObjCProcessor::PostProcessObjCSections(VMReader* reader, std::string bas DefineObjCSymbol(DataSymbol, type, "selRef_" + sel, i, true); } } - if (auto superRefs = m_data->GetSectionByName(baseName + "::__objc_classrefs")) + if (auto superRefs = m_data->GetSectionByName(baseName + ".__objc_classrefs")) { auto start = superRefs->GetStart(); auto end = superRefs->GetEnd(); @@ -1171,7 +1171,7 @@ void DSCObjCProcessor::PostProcessObjCSections(VMReader* reader, std::string bas } } } - if (auto superRefs = m_data->GetSectionByName(baseName + "::__objc_superrefs")) + if (auto superRefs = m_data->GetSectionByName(baseName + ".__objc_superrefs")) { auto start = superRefs->GetStart(); auto end = superRefs->GetEnd(); @@ -1189,7 +1189,7 @@ void DSCObjCProcessor::PostProcessObjCSections(VMReader* reader, std::string bas } } } - if (auto protoRefs = m_data->GetSectionByName(baseName + "::__objc_protorefs")) + if (auto protoRefs = m_data->GetSectionByName(baseName + ".__objc_protorefs")) { auto start = protoRefs->GetStart(); auto end = protoRefs->GetEnd(); @@ -1207,7 +1207,7 @@ void DSCObjCProcessor::PostProcessObjCSections(VMReader* reader, std::string bas } } } - if (auto ivars = m_data->GetSectionByName(baseName + "::__objc_ivar")) + if (auto ivars = m_data->GetSectionByName(baseName + ".__objc_ivar")) { auto start = ivars->GetStart(); auto end = ivars->GetEnd(); @@ -1408,23 +1408,23 @@ void DSCObjCProcessor::ProcessObjCData(std::shared_ptr vm, std::string baseN m_typeNames.protocol = finalizeStructureBuilder(m_data, protocolBuilder, "objc_protocol_t").first; m_data->BeginBulkModifySymbols(); - if (auto classList = m_data->GetSectionByName(baseName + "::__objc_classlist")) + if (auto classList = m_data->GetSectionByName(baseName + ".__objc_classlist")) LoadClasses(&reader, classList); - if (auto nonLazyClassList = m_data->GetSectionByName(baseName + "::__objc_nlclslist")) + if (auto nonLazyClassList = m_data->GetSectionByName(baseName + ".__objc_nlclslist")) LoadClasses(&reader, nonLazyClassList); // See: https://stackoverflow.com/a/15318325 GenerateClassTypes(); for (auto& [_, cls] : m_classes) ApplyMethodTypes(cls); - if (auto catList = m_data->GetSectionByName(baseName + "::__objc_catlist")) // Do this after loading class type data. + if (auto catList = m_data->GetSectionByName(baseName + ".__objc_catlist")) // Do this after loading class type data. LoadCategories(&reader, catList); - if (auto nonLazyCatList = m_data->GetSectionByName(baseName + "::__objc_nlcatlist")) // Do this after loading class type data. + if (auto nonLazyCatList = m_data->GetSectionByName(baseName + ".__objc_nlcatlist")) // Do this after loading class type data. LoadCategories(&reader, nonLazyCatList); for (auto& [_, cat] : m_categories) ApplyMethodTypes(cat); - if (auto protoList = m_data->GetSectionByName(baseName + "::__objc_protolist")) + if (auto protoList = m_data->GetSectionByName(baseName + ".__objc_protolist")) LoadProtocols(&reader, protoList); PostProcessObjCSections(&reader, baseName); @@ -1477,7 +1477,7 @@ void DSCObjCProcessor::ProcessCFStrings(std::shared_ptr vm, std::string base m_typeNames.cfStringUTF16 = type.first; auto reader = VMReader(vm); - if (auto cfstrings = m_data->GetSectionByName(baseName + "::__cfstring")) + if (auto cfstrings = m_data->GetSectionByName(baseName + ".__cfstring")) { auto start = cfstrings->GetStart(); auto end = cfstrings->GetEnd(); diff --git a/view/sharedcache/core/SharedCache.cpp b/view/sharedcache/core/SharedCache.cpp index 468a17471d..0bad444ea5 100644 --- a/view/sharedcache/core/SharedCache.cpp +++ b/view/sharedcache/core/SharedCache.cpp @@ -1586,7 +1586,7 @@ bool SharedCache::LoadImageContainingAddress(uint64_t address, bool skipObjC) static std::string GetNameForMemoryDataRegion(const SharedCacheMachOHeader& header, const std::string& regionName) { - return fmt::format("{}{}", header.installName.substr(0, header.installName.size() - header.identifierPrefix.size()), regionName); + return fmt::format("{}{}", header.installName.substr(0, header.installName.size() - header.identifierPrefix.size()), regionName); } bool SharedCache::LoadSectionAtAddress(uint64_t address) @@ -1661,7 +1661,7 @@ bool SharedCache::LoadSectionAtAddress(uint64_t address) auto targetFile = vm->MappingAtAddress(targetSegment->start).first.fileAccessor->lock(); auto buff = reader.ReadBuffer(targetSegment->start, targetSegment->size); - m_dscView->GetMemoryMap()->AddDataMemoryRegion(GetNameForMemoryDataRegion(targetHeader, targetSegment->prettyName), targetSegment->start, buff, targetSegment->flags); + m_dscView->GetMemoryMap()->AddDataMemoryRegion(GetNameForMemoryDataRegion(targetHeader, targetSegment->prettyName), targetSegment->start, buff, targetSegment->flags); SetMemoryRegionIsLoaded(lock, *targetSegment); @@ -1689,22 +1689,34 @@ static void GetObjCSettings(Ref view, bool* processObjCMetadata, boo *processObjCMetadata = settings->Get("loader.dsc.processObjC", view); } -static void ProcessObjCSectionsForImageWithName(std::string baseName, std::shared_ptr vm, std::shared_ptr objc, bool processCFStrings, bool processObjCMetadata, Ref logger) +static void ProcessObjCSectionsForMemoryRegion(const MemoryRegion& region, std::shared_ptr vm, std::shared_ptr objc, bool processCFStrings, bool processObjCMetadata, Ref logger) { try { if (processObjCMetadata) - objc->ProcessObjCData(vm, baseName); + objc->ProcessObjCData(vm, region.prettyName); if (processCFStrings) - objc->ProcessCFStrings(vm, baseName); + objc->ProcessCFStrings(vm, region.prettyName); } catch (const std::exception& ex) { - logger->LogWarn("Error processing ObjC data for image %s: %s", baseName.c_str(), ex.what()); + logger->LogWarn("Error processing ObjC data for memory region %s: %s", region.prettyName.c_str(), ex.what()); } catch (...) { - logger->LogWarn("Error processing ObjC data for image %s", baseName.c_str()); + logger->LogWarn("Error processing ObjC data for memory region %s", region.prettyName.c_str()); + } +} + +void SharedCache::ProcessObjCSectionsForImage(std::lock_guard& lock, const CacheImage& image, std::shared_ptr vm, bool processCFStrings, bool processObjCMetadata) +{ + const auto objc = std::make_shared(m_dscView, this, false); + for (auto regionStart : image.regionStarts) + { + const auto& region = m_cacheInfo->memoryRegions.find(regionStart)->second; + if (!MemoryRegionIsLoaded(lock, region)) + continue; + ProcessObjCSectionsForMemoryRegion(region, vm, objc, processCFStrings, processObjCMetadata, m_logger); } } @@ -1717,10 +1729,17 @@ void SharedCache::ProcessObjCSectionsForImageWithInstallName(std::string install if (!processObjCMetadata && !processCFStrings) return; - auto objc = std::make_shared(m_dscView, this, false); auto vm = GetVMMap(); - ProcessObjCSectionsForImageWithName(base_name(installName), vm, objc, processCFStrings, processObjCMetadata, m_logger); + for (const auto& image : this->m_cacheInfo->images) + { + if (image.installName != installName) + continue; + + std::lock_guard lock(m_mutex); + ProcessObjCSectionsForImage(lock, image, vm, processCFStrings, processObjCMetadata); + break; + } } void SharedCache::ProcessAllObjCSections() @@ -1741,18 +1760,9 @@ void SharedCache::ProcessAllObjCSections(std::lock_guard& lock) auto objc = std::make_shared(m_dscView, this, false); auto vm = GetVMMap(); - std::set processedImageHeaders; for (auto region : GetMappedRegions()) { - // Don't repeat the same images multiple times - auto header = HeaderForAddress(region->start); - if (!header) - continue; - if (processedImageHeaders.find(header->textBase) != processedImageHeaders.end()) - continue; - processedImageHeaders.insert(header->textBase); - - ProcessObjCSectionsForImageWithName(header->identifierPrefix, vm, objc, processCFStrings, processObjCMetadata, m_logger); + ProcessObjCSectionsForMemoryRegion(*region, vm, objc, processCFStrings, processObjCMetadata, m_logger); } } @@ -1853,7 +1863,7 @@ bool SharedCache::LoadImageWithInstallName(std::lock_guard& lock, st bool processObjCMetadata; GetObjCSettings(m_dscView, &processCFStrings, &processObjCMetadata); - ProcessObjCSectionsForImageWithName(h->identifierPrefix, vm, std::make_shared(m_dscView, this, false), processCFStrings, processObjCMetadata, m_logger); + ProcessObjCSectionsForImage(lock, *targetImage, vm, processCFStrings, processObjCMetadata); } m_dscView->AddAnalysisOption("linearsweep"); @@ -2258,7 +2268,7 @@ std::optional SharedCache::LoadHeaderForAddress(std::sha char sectionName[sizeof(section.sectname)+1]; memcpy(sectionName, section.sectname, sizeof(section.sectname)); sectionName[sizeof(sectionName)-1] = 0; - + char segmentName[sizeof(section.segname)+1]; memcpy(segmentName, section.segname, sizeof(section.segname)); segmentName[sizeof(segmentName)-1] = 0; diff --git a/view/sharedcache/core/SharedCache.h b/view/sharedcache/core/SharedCache.h index f6a1e7a8bb..55bf3b454d 100644 --- a/view/sharedcache/core/SharedCache.h +++ b/view/sharedcache/core/SharedCache.h @@ -647,6 +647,7 @@ namespace SharedCacheCore { uint64_t stringsOffset, size_t stringsSize, uint64_t nlistEntriesOffset, uint32_t nlistCount, uint32_t nlistStartIndex = 0); void ApplySymbol(Ref view, Ref typeLib, Ref symbol); + void ProcessObjCSectionsForImage(std::lock_guard& lock, const CacheImage& image, std::shared_ptr vm, bool processCFStrings, bool processObjCMetadata); void ProcessAllObjCSections(std::lock_guard&); bool LoadImageWithInstallName(std::lock_guard&, std::string installName, bool skipObjC);