From 6cb671483fe02f0e9ab20bab189101d1cc8210a1 Mon Sep 17 00:00:00 2001 From: Mark Rowe Date: Tue, 8 Jul 2025 10:30:26 -0700 Subject: [MachO] Avoid leaking MachoObjCProcessor This would leak if parsing of CFStrings was enabled while parsing of Objective-C metadata was disabled. It would also leak if exceptions were thrown or early returns were taken in the ~500 lines between where the object was allocated and it was deleted. --- objectivec/objc.cpp | 26 +++++++++++--------------- view/macho/machoview.cpp | 35 +++++++++++++++++++---------------- view/macho/machoview.h | 8 +++----- 3 files changed, 33 insertions(+), 36 deletions(-) diff --git a/objectivec/objc.cpp b/objectivec/objc.cpp index 6316c11f..f0a6a8f3 100644 --- a/objectivec/objc.cpp +++ b/objectivec/objc.cpp @@ -1713,7 +1713,7 @@ void ObjCProcessor::ProcessCFStrings() void ObjCProcessor::ProcessNSConstantArrays() { - m_symbolQueue = new SymbolQueue(); + auto guard = ScopedSymbolQueue::Make(); uint64_t ptrSize = m_data->GetAddressSize(); auto idType = Type::NamedType(m_data, m_typeNames.id); @@ -1742,16 +1742,16 @@ void ObjCProcessor::ProcessNSConstantArrays() fmt::format("nsarray_{:x}", i), i, true); } auto id = m_data->BeginUndoActions(); - m_symbolQueue->Process(); + ScopedSymbolQueue::Get().Process(); m_data->EndBulkModifySymbols(); m_data->ForgetUndoActions(id); } - delete m_symbolQueue; + } void ObjCProcessor::ProcessNSConstantDictionaries() { - m_symbolQueue = new SymbolQueue(); + auto guard = ScopedSymbolQueue::Make(); uint64_t ptrSize = m_data->GetAddressSize(); auto idType = Type::NamedType(m_data, m_typeNames.id); @@ -1786,16 +1786,15 @@ void ObjCProcessor::ProcessNSConstantDictionaries() fmt::format("nsdict_{:x}", i), i, true); } auto id = m_data->BeginUndoActions(); - m_symbolQueue->Process(); + ScopedSymbolQueue::Get().Process(); m_data->EndBulkModifySymbols(); m_data->ForgetUndoActions(id); } - delete m_symbolQueue; } void ObjCProcessor::ProcessNSConstantIntegerNumbers() { - m_symbolQueue = new SymbolQueue(); + auto guard = ScopedSymbolQueue::Make(); uint64_t ptrSize = m_data->GetAddressSize(); StructureBuilder nsConstantIntegerNumberBuilder; @@ -1844,11 +1843,10 @@ void ObjCProcessor::ProcessNSConstantIntegerNumbers() } } auto id = m_data->BeginUndoActions(); - m_symbolQueue->Process(); + ScopedSymbolQueue::Get().Process(); m_data->EndBulkModifySymbols(); m_data->ForgetUndoActions(id); } - delete m_symbolQueue; } void ObjCProcessor::ProcessNSConstantFloatingPointNumbers() @@ -1893,7 +1891,7 @@ void ObjCProcessor::ProcessNSConstantFloatingPointNumbers() if (!numbers) continue; - m_symbolQueue = new SymbolQueue(); + auto guard = ScopedSymbolQueue::Make(); auto start = numbers->GetStart(); auto end = numbers->GetEnd(); auto typeWidth = Type::NamedType(m_data, m_typeNames.nsConstantDoubleNumber)->GetWidth(); @@ -1935,16 +1933,15 @@ void ObjCProcessor::ProcessNSConstantFloatingPointNumbers() DefineObjCSymbol(DataSymbol, Type::NamedType(m_data, *typeName), name, i, true); } auto id = m_data->BeginUndoActions(); - m_symbolQueue->Process(); + ScopedSymbolQueue::Get().Process(); m_data->EndBulkModifySymbols(); m_data->ForgetUndoActions(id); - delete m_symbolQueue; } } void ObjCProcessor::ProcessNSConstantDatas() { - m_symbolQueue = new SymbolQueue(); + auto guard = ScopedSymbolQueue::Make(); uint64_t ptrSize = m_data->GetAddressSize(); StructureBuilder nsConstantDataBuilder; @@ -1972,11 +1969,10 @@ void ObjCProcessor::ProcessNSConstantDatas() DataSymbol, Type::NamedType(m_data, m_typeNames.nsConstantData), fmt::format("nsdata_{:x}", i), i, true); } auto id = m_data->BeginUndoActions(); - m_symbolQueue->Process(); + ScopedSymbolQueue::Get().Process(); m_data->EndBulkModifySymbols(); m_data->ForgetUndoActions(id); } - delete m_symbolQueue; } void ObjCProcessor::AddRelocatedPointer(uint64_t location, uint64_t rewrite) diff --git a/view/macho/machoview.cpp b/view/macho/machoview.cpp index 9244a9c9..77d8d8fe 100644 --- a/view/macho/machoview.cpp +++ b/view/macho/machoview.cpp @@ -1856,9 +1856,11 @@ bool MachoView::InitializeHeader(MachOHeader& header, bool isMainHeader, uint64_ parseObjCStructs = false; if (!GetSectionByName("__cfstring")) parseCFStrings = false; + + std::unique_ptr objcProcessor; if (parseObjCStructs || parseCFStrings) { - m_objcProcessor = new MachoObjCProcessor(this); + objcProcessor = std::make_unique(this); } if (parseObjCStructs) { @@ -2067,7 +2069,7 @@ bool MachoView::InitializeHeader(MachOHeader& header, bool isMainHeader, uint64_ { // Add functions for all function symbols m_logger->LogDebug("Parsing symbol table\n"); - ParseSymbolTable(reader, header, header.symtab, indirectSymbols); + ParseSymbolTable(reader, header, header.symtab, indirectSymbols, objcProcessor.get()); } catch (std::exception&) { @@ -2088,8 +2090,8 @@ bool MachoView::InitializeHeader(MachOHeader& header, bool isMainHeader, uint64_ uint64_t slidTarget = target + m_imageBaseAdjustment; relocation.address = slidTarget; DefineRelocation(m_arch, relocation, slidTarget, relocationLocation); - if (m_objcProcessor) - m_objcProcessor->AddRelocatedPointer(relocationLocation, slidTarget); + if (objcProcessor) + objcProcessor->AddRelocatedPointer(relocationLocation, slidTarget); } for (auto& [relocation, name] : header.externalRelocations) { @@ -2369,7 +2371,7 @@ bool MachoView::InitializeHeader(MachOHeader& header, bool isMainHeader, uint64_ if (parseCFStrings) { try { - m_objcProcessor->ProcessObjCLiterals(); + objcProcessor->ProcessObjCLiterals(); } catch (std::exception& ex) { @@ -2381,14 +2383,13 @@ bool MachoView::InitializeHeader(MachOHeader& header, bool isMainHeader, uint64_ if (parseObjCStructs) { try { - m_objcProcessor->ProcessObjCData(); + objcProcessor->ProcessObjCData(); } catch (std::exception& ex) { m_logger->LogError("Failed to process Objective-C Metadata. Binary may be malformed"); m_logger->LogError("Error: %s", ex.what()); } - delete m_objcProcessor; } @@ -2976,7 +2977,8 @@ void MachoView::ParseDynamicTable(BinaryReader& reader, MachOHeader& header, BNS } -void MachoView::ParseSymbolTable(BinaryReader& reader, MachOHeader& header, const symtab_command& symtab, const vector& indirectSymbols) +void MachoView::ParseSymbolTable(BinaryReader& reader, MachOHeader& header, const symtab_command& symtab, + const vector& indirectSymbols, MachoObjCProcessor* objcProcessor) { if (header.ident.filetype == MH_DSYM) { @@ -3004,12 +3006,12 @@ void MachoView::ParseSymbolTable(BinaryReader& reader, MachOHeader& header, cons if (header.chainedFixupsPresent) { m_logger->LogDebug("Chained Fixups"); - ParseChainedFixups(header, header.chainedFixups); + ParseChainedFixups(header, header.chainedFixups, objcProcessor); } else if (header.chainStartsPresent) { m_logger->LogDebug("Chained Starts"); - ParseChainedStarts(header, header.chainStarts); + ParseChainedStarts(header, header.chainStarts, objcProcessor); } if (header.exportTriePresent && header.isMainHeader) ParseExportTrie(reader, header.exportTrie); @@ -3197,7 +3199,8 @@ void MachoView::ParseSymbolTable(BinaryReader& reader, MachOHeader& header, cons } -void MachoView::ParseChainedFixups(MachOHeader& header, linkedit_data_command chainedFixups) +void MachoView::ParseChainedFixups( + MachOHeader& header, linkedit_data_command chainedFixups, MachoObjCProcessor* objcProcessor) { if (!chainedFixups.dataoff) return; @@ -3596,9 +3599,9 @@ void MachoView::ParseChainedFixups(MachOHeader& header, linkedit_data_command ch reloc.address = GetStart() + (chainEntryAddress - m_universalImageOffset); DefineRelocation(m_arch, reloc, entryOffset, reloc.address); - if (m_objcProcessor) + if (objcProcessor) { - m_objcProcessor->AddRelocatedPointer(reloc.address, entryOffset); + objcProcessor->AddRelocatedPointer(reloc.address, entryOffset); } } @@ -3627,7 +3630,7 @@ void MachoView::ParseChainedFixups(MachOHeader& header, linkedit_data_command ch } -void MachoView::ParseChainedStarts(MachOHeader& header, section_64 chainedStarts) +void MachoView::ParseChainedStarts(MachOHeader& header, section_64 chainedStarts, MachoObjCProcessor* objcProcessor) { if (!chainedStarts.offset) return; @@ -3799,9 +3802,9 @@ void MachoView::ParseChainedStarts(MachOHeader& header, section_64 chainedStarts DefineRelocation(m_arch, reloc, entryOffset, reloc.address); m_logger->LogDebug("Chained Starts: Adding relocated pointer %llx -> %llx", reloc.address, entryOffset); - if (m_objcProcessor) + if (objcProcessor) { - m_objcProcessor->AddRelocatedPointer(reloc.address, entryOffset); + objcProcessor->AddRelocatedPointer(reloc.address, entryOffset); } } diff --git a/view/macho/machoview.h b/view/macho/machoview.h index dfbb24fe..253af9d1 100644 --- a/view/macho/machoview.h +++ b/view/macho/machoview.h @@ -1456,8 +1456,6 @@ namespace BinaryNinja QualifiedName filesetEntryCommandQualName; } m_typeNames; - MachoObjCProcessor* m_objcProcessor = nullptr; - uint64_t m_universalImageOffset; bool m_parseOnly, m_backedByDatabase; int64_t m_imageBaseAdjustment; @@ -1488,7 +1486,7 @@ namespace BinaryNinja void RebaseThreadStarts(BinaryReader& virtualReader, std::vector& threadStarts, uint64_t stepMultiplier); Ref DefineMachoSymbol( BNSymbolType type, const std::string& name, uint64_t addr, BNSymbolBinding binding, bool deferred); - void ParseSymbolTable(BinaryReader& reader, MachOHeader& header, const symtab_command& symtab, const std::vector& symbolStubsList); + void ParseSymbolTable(BinaryReader& reader, MachOHeader& header, const symtab_command& symtab, const std::vector& symbolStubsList, MachoObjCProcessor*); bool IsValidFunctionStart(uint64_t addr); void ParseFunctionStarts(Platform* platform, uint64_t textBase, function_starts_command functionStarts); bool ParseRelocationEntry(const relocation_info& info, uint64_t start, BNRelocationInfo& result); @@ -1503,8 +1501,8 @@ namespace BinaryNinja BNSymbolBinding binding); bool GetSectionPermissions(MachOHeader& header, uint64_t address, uint32_t &flags); bool GetSegmentPermissions(MachOHeader& header, uint64_t address, uint32_t &flags); - void ParseChainedFixups(MachOHeader& header, linkedit_data_command chainedFixups); - void ParseChainedStarts(MachOHeader& header, section_64 chainedStarts); + void ParseChainedFixups(MachOHeader& header, linkedit_data_command chainedFixups, MachoObjCProcessor*); + void ParseChainedStarts(MachOHeader& header, section_64 chainedStarts, MachoObjCProcessor*); virtual uint64_t PerformGetEntryPoint() const override; -- cgit v1.3.1