Skip to content

Commit 6cb6714

Browse files
bdashbdash
committed
[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.
1 parent f628cc6 commit 6cb6714

File tree

3 files changed

+33
-36
lines changed

3 files changed

+33
-36
lines changed

‎objectivec/objc.cpp‎

Lines changed: 11 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1713,7 +1713,7 @@ void ObjCProcessor::ProcessCFStrings()
17131713

17141714
void ObjCProcessor::ProcessNSConstantArrays()
17151715
{
1716-
m_symbolQueue = new SymbolQueue();
1716+
auto guard = ScopedSymbolQueue::Make();
17171717
uint64_t ptrSize = m_data->GetAddressSize();
17181718

17191719
auto idType = Type::NamedType(m_data, m_typeNames.id);
@@ -1742,16 +1742,16 @@ void ObjCProcessor::ProcessNSConstantArrays()
17421742
fmt::format("nsarray_{:x}", i), i, true);
17431743
}
17441744
auto id = m_data->BeginUndoActions();
1745-
m_symbolQueue->Process();
1745+
ScopedSymbolQueue::Get().Process();
17461746
m_data->EndBulkModifySymbols();
17471747
m_data->ForgetUndoActions(id);
17481748
}
1749-
delete m_symbolQueue;
1749+
17501750
}
17511751

17521752
void ObjCProcessor::ProcessNSConstantDictionaries()
17531753
{
1754-
m_symbolQueue = new SymbolQueue();
1754+
auto guard = ScopedSymbolQueue::Make();
17551755
uint64_t ptrSize = m_data->GetAddressSize();
17561756

17571757
auto idType = Type::NamedType(m_data, m_typeNames.id);
@@ -1786,16 +1786,15 @@ void ObjCProcessor::ProcessNSConstantDictionaries()
17861786
fmt::format("nsdict_{:x}", i), i, true);
17871787
}
17881788
auto id = m_data->BeginUndoActions();
1789-
m_symbolQueue->Process();
1789+
ScopedSymbolQueue::Get().Process();
17901790
m_data->EndBulkModifySymbols();
17911791
m_data->ForgetUndoActions(id);
17921792
}
1793-
delete m_symbolQueue;
17941793
}
17951794

17961795
void ObjCProcessor::ProcessNSConstantIntegerNumbers()
17971796
{
1798-
m_symbolQueue = new SymbolQueue();
1797+
auto guard = ScopedSymbolQueue::Make();
17991798
uint64_t ptrSize = m_data->GetAddressSize();
18001799

18011800
StructureBuilder nsConstantIntegerNumberBuilder;
@@ -1844,11 +1843,10 @@ void ObjCProcessor::ProcessNSConstantIntegerNumbers()
18441843
}
18451844
}
18461845
auto id = m_data->BeginUndoActions();
1847-
m_symbolQueue->Process();
1846+
ScopedSymbolQueue::Get().Process();
18481847
m_data->EndBulkModifySymbols();
18491848
m_data->ForgetUndoActions(id);
18501849
}
1851-
delete m_symbolQueue;
18521850
}
18531851

18541852
void ObjCProcessor::ProcessNSConstantFloatingPointNumbers()
@@ -1893,7 +1891,7 @@ void ObjCProcessor::ProcessNSConstantFloatingPointNumbers()
18931891
if (!numbers)
18941892
continue;
18951893

1896-
m_symbolQueue = new SymbolQueue();
1894+
auto guard = ScopedSymbolQueue::Make();
18971895
auto start = numbers->GetStart();
18981896
auto end = numbers->GetEnd();
18991897
auto typeWidth = Type::NamedType(m_data, m_typeNames.nsConstantDoubleNumber)->GetWidth();
@@ -1935,16 +1933,15 @@ void ObjCProcessor::ProcessNSConstantFloatingPointNumbers()
19351933
DefineObjCSymbol(DataSymbol, Type::NamedType(m_data, *typeName), name, i, true);
19361934
}
19371935
auto id = m_data->BeginUndoActions();
1938-
m_symbolQueue->Process();
1936+
ScopedSymbolQueue::Get().Process();
19391937
m_data->EndBulkModifySymbols();
19401938
m_data->ForgetUndoActions(id);
1941-
delete m_symbolQueue;
19421939
}
19431940
}
19441941

19451942
void ObjCProcessor::ProcessNSConstantDatas()
19461943
{
1947-
m_symbolQueue = new SymbolQueue();
1944+
auto guard = ScopedSymbolQueue::Make();
19481945
uint64_t ptrSize = m_data->GetAddressSize();
19491946

19501947
StructureBuilder nsConstantDataBuilder;
@@ -1972,11 +1969,10 @@ void ObjCProcessor::ProcessNSConstantDatas()
19721969
DataSymbol, Type::NamedType(m_data, m_typeNames.nsConstantData), fmt::format("nsdata_{:x}", i), i, true);
19731970
}
19741971
auto id = m_data->BeginUndoActions();
1975-
m_symbolQueue->Process();
1972+
ScopedSymbolQueue::Get().Process();
19761973
m_data->EndBulkModifySymbols();
19771974
m_data->ForgetUndoActions(id);
19781975
}
1979-
delete m_symbolQueue;
19801976
}
19811977

19821978
void ObjCProcessor::AddRelocatedPointer(uint64_t location, uint64_t rewrite)

‎view/macho/machoview.cpp‎

Lines changed: 19 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1856,9 +1856,11 @@ bool MachoView::InitializeHeader(MachOHeader& header, bool isMainHeader, uint64_
18561856
parseObjCStructs = false;
18571857
if (!GetSectionByName("__cfstring"))
18581858
parseCFStrings = false;
1859+
1860+
std::unique_ptr<MachoObjCProcessor> objcProcessor;
18591861
if (parseObjCStructs || parseCFStrings)
18601862
{
1861-
m_objcProcessor = new MachoObjCProcessor(this);
1863+
objcProcessor = std::make_unique<MachoObjCProcessor>(this);
18621864
}
18631865
if (parseObjCStructs)
18641866
{
@@ -2067,7 +2069,7 @@ bool MachoView::InitializeHeader(MachOHeader& header, bool isMainHeader, uint64_
20672069
{
20682070
// Add functions for all function symbols
20692071
m_logger->LogDebug("Parsing symbol table\n");
2070-
ParseSymbolTable(reader, header, header.symtab, indirectSymbols);
2072+
ParseSymbolTable(reader, header, header.symtab, indirectSymbols, objcProcessor.get());
20712073
}
20722074
catch (std::exception&)
20732075
{
@@ -2088,8 +2090,8 @@ bool MachoView::InitializeHeader(MachOHeader& header, bool isMainHeader, uint64_
20882090
uint64_t slidTarget = target + m_imageBaseAdjustment;
20892091
relocation.address = slidTarget;
20902092
DefineRelocation(m_arch, relocation, slidTarget, relocationLocation);
2091-
if (m_objcProcessor)
2092-
m_objcProcessor->AddRelocatedPointer(relocationLocation, slidTarget);
2093+
if (objcProcessor)
2094+
objcProcessor->AddRelocatedPointer(relocationLocation, slidTarget);
20932095
}
20942096
for (auto& [relocation, name] : header.externalRelocations)
20952097
{
@@ -2369,7 +2371,7 @@ bool MachoView::InitializeHeader(MachOHeader& header, bool isMainHeader, uint64_
23692371
if (parseCFStrings)
23702372
{
23712373
try {
2372-
m_objcProcessor->ProcessObjCLiterals();
2374+
objcProcessor->ProcessObjCLiterals();
23732375
}
23742376
catch (std::exception& ex)
23752377
{
@@ -2381,14 +2383,13 @@ bool MachoView::InitializeHeader(MachOHeader& header, bool isMainHeader, uint64_
23812383
if (parseObjCStructs)
23822384
{
23832385
try {
2384-
m_objcProcessor->ProcessObjCData();
2386+
objcProcessor->ProcessObjCData();
23852387
}
23862388
catch (std::exception& ex)
23872389
{
23882390
m_logger->LogError("Failed to process Objective-C Metadata. Binary may be malformed");
23892391
m_logger->LogError("Error: %s", ex.what());
23902392
}
2391-
delete m_objcProcessor;
23922393
}
23932394

23942395

@@ -2976,7 +2977,8 @@ void MachoView::ParseDynamicTable(BinaryReader& reader, MachOHeader& header, BNS
29762977
}
29772978

29782979

2979-
void MachoView::ParseSymbolTable(BinaryReader& reader, MachOHeader& header, const symtab_command& symtab, const vector<uint32_t>& indirectSymbols)
2980+
void MachoView::ParseSymbolTable(BinaryReader& reader, MachOHeader& header, const symtab_command& symtab,
2981+
const vector<uint32_t>& indirectSymbols, MachoObjCProcessor* objcProcessor)
29802982
{
29812983
if (header.ident.filetype == MH_DSYM)
29822984
{
@@ -3004,12 +3006,12 @@ void MachoView::ParseSymbolTable(BinaryReader& reader, MachOHeader& header, cons
30043006
if (header.chainedFixupsPresent)
30053007
{
30063008
m_logger->LogDebug("Chained Fixups");
3007-
ParseChainedFixups(header, header.chainedFixups);
3009+
ParseChainedFixups(header, header.chainedFixups, objcProcessor);
30083010
}
30093011
else if (header.chainStartsPresent)
30103012
{
30113013
m_logger->LogDebug("Chained Starts");
3012-
ParseChainedStarts(header, header.chainStarts);
3014+
ParseChainedStarts(header, header.chainStarts, objcProcessor);
30133015
}
30143016
if (header.exportTriePresent && header.isMainHeader)
30153017
ParseExportTrie(reader, header.exportTrie);
@@ -3197,7 +3199,8 @@ void MachoView::ParseSymbolTable(BinaryReader& reader, MachOHeader& header, cons
31973199
}
31983200

31993201

3200-
void MachoView::ParseChainedFixups(MachOHeader& header, linkedit_data_command chainedFixups)
3202+
void MachoView::ParseChainedFixups(
3203+
MachOHeader& header, linkedit_data_command chainedFixups, MachoObjCProcessor* objcProcessor)
32013204
{
32023205
if (!chainedFixups.dataoff)
32033206
return;
@@ -3596,9 +3599,9 @@ void MachoView::ParseChainedFixups(MachOHeader& header, linkedit_data_command ch
35963599
reloc.address = GetStart() + (chainEntryAddress - m_universalImageOffset);
35973600
DefineRelocation(m_arch, reloc, entryOffset, reloc.address);
35983601

3599-
if (m_objcProcessor)
3602+
if (objcProcessor)
36003603
{
3601-
m_objcProcessor->AddRelocatedPointer(reloc.address, entryOffset);
3604+
objcProcessor->AddRelocatedPointer(reloc.address, entryOffset);
36023605
}
36033606
}
36043607

@@ -3627,7 +3630,7 @@ void MachoView::ParseChainedFixups(MachOHeader& header, linkedit_data_command ch
36273630
}
36283631

36293632

3630-
void MachoView::ParseChainedStarts(MachOHeader& header, section_64 chainedStarts)
3633+
void MachoView::ParseChainedStarts(MachOHeader& header, section_64 chainedStarts, MachoObjCProcessor* objcProcessor)
36313634
{
36323635
if (!chainedStarts.offset)
36333636
return;
@@ -3799,9 +3802,9 @@ void MachoView::ParseChainedStarts(MachOHeader& header, section_64 chainedStarts
37993802
DefineRelocation(m_arch, reloc, entryOffset, reloc.address);
38003803
m_logger->LogDebug("Chained Starts: Adding relocated pointer %llx -> %llx", reloc.address, entryOffset);
38013804

3802-
if (m_objcProcessor)
3805+
if (objcProcessor)
38033806
{
3804-
m_objcProcessor->AddRelocatedPointer(reloc.address, entryOffset);
3807+
objcProcessor->AddRelocatedPointer(reloc.address, entryOffset);
38053808
}
38063809
}
38073810

‎view/macho/machoview.h‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1456,8 +1456,6 @@ namespace BinaryNinja
14561456
QualifiedName filesetEntryCommandQualName;
14571457
} m_typeNames;
14581458

1459-
MachoObjCProcessor* m_objcProcessor = nullptr;
1460-
14611459
uint64_t m_universalImageOffset;
14621460
bool m_parseOnly, m_backedByDatabase;
14631461
int64_t m_imageBaseAdjustment;
@@ -1488,7 +1486,7 @@ namespace BinaryNinja
14881486
void RebaseThreadStarts(BinaryReader& virtualReader, std::vector<uint32_t>& threadStarts, uint64_t stepMultiplier);
14891487
Ref<Symbol> DefineMachoSymbol(
14901488
BNSymbolType type, const std::string& name, uint64_t addr, BNSymbolBinding binding, bool deferred);
1491-
void ParseSymbolTable(BinaryReader& reader, MachOHeader& header, const symtab_command& symtab, const std::vector<uint32_t>& symbolStubsList);
1489+
void ParseSymbolTable(BinaryReader& reader, MachOHeader& header, const symtab_command& symtab, const std::vector<uint32_t>& symbolStubsList, MachoObjCProcessor*);
14921490
bool IsValidFunctionStart(uint64_t addr);
14931491
void ParseFunctionStarts(Platform* platform, uint64_t textBase, function_starts_command functionStarts);
14941492
bool ParseRelocationEntry(const relocation_info& info, uint64_t start, BNRelocationInfo& result);
@@ -1503,8 +1501,8 @@ namespace BinaryNinja
15031501
BNSymbolBinding binding);
15041502
bool GetSectionPermissions(MachOHeader& header, uint64_t address, uint32_t &flags);
15051503
bool GetSegmentPermissions(MachOHeader& header, uint64_t address, uint32_t &flags);
1506-
void ParseChainedFixups(MachOHeader& header, linkedit_data_command chainedFixups);
1507-
void ParseChainedStarts(MachOHeader& header, section_64 chainedStarts);
1504+
void ParseChainedFixups(MachOHeader& header, linkedit_data_command chainedFixups, MachoObjCProcessor*);
1505+
void ParseChainedStarts(MachOHeader& header, section_64 chainedStarts, MachoObjCProcessor*);
15081506

15091507
virtual uint64_t PerformGetEntryPoint() const override;
15101508

0 commit comments

Comments
 (0)