From 2b1fc96ebb746ee95c12b5a35cd4ffbe9a83d73e Mon Sep 17 00:00:00 2001 From: Mark Rowe Date: Fri, 27 Feb 2026 17:47:37 -0800 Subject: [MachO] Ensure that weak bound symbols are not resolved to their import address Fixes https://github.com/Vector35/binaryninja-api/issues/7989. Also corrects an oversight from d92b3684 in handling of library ordinals >= 128. --- view/macho/chained_fixups.cpp | 6 +- view/macho/machoview.cpp | 130 ++++++++++++++++++------------------------ 2 files changed, 57 insertions(+), 79 deletions(-) (limited to 'view') diff --git a/view/macho/chained_fixups.cpp b/view/macho/chained_fixups.cpp index 9a8e8617..e71b34b8 100644 --- a/view/macho/chained_fixups.cpp +++ b/view/macho/chained_fixups.cpp @@ -247,7 +247,7 @@ ImportEntry ReadChainedImport32(BinaryReader& reader, std::span symb return { std::string_view(&symbolData[import.name_offset]), 0, - static_cast(import.lib_ordinal), + import.lib_ordinal > 0xF0 ? static_cast(import.lib_ordinal) : static_cast(import.lib_ordinal), (bool)import.weak_import, }; } @@ -259,7 +259,7 @@ ImportEntry ReadChainedImportAddend32(BinaryReader& reader, std::span(import.addend), - static_cast(import.lib_ordinal), + import.lib_ordinal > 0xF0 ? static_cast(import.lib_ordinal) : static_cast(import.lib_ordinal), (bool)import.weak_import, }; } @@ -271,7 +271,7 @@ ImportEntry ReadChainedImportAddend64(BinaryReader& reader, std::span(import.lib_ordinal), + import.lib_ordinal > 0xFFF0 ? static_cast(import.lib_ordinal) : static_cast(import.lib_ordinal), (bool)import.weak_import, }; } diff --git a/view/macho/machoview.cpp b/view/macho/machoview.cpp index f742a1c5..b1a5bdd9 100644 --- a/view/macho/machoview.cpp +++ b/view/macho/machoview.cpp @@ -1710,6 +1710,51 @@ bool MachoView::Init() } +static Ref FindInternalSymbol(BinaryView* view, const std::string& name) +{ + // When multiple symbols are defined with the same name, which can happen when a symbol is both + // in the symbol table and self-bound, `GetSymbolByRawName` prefers the symbol with the lowest + // type value. Since `ImportAddressSymbol` is a lower value than `DataSymbol`/`FunctionSymbol`, + // it would return the import stub rather than the actual symbol definition. Filter it out. + auto symbols = view->GetSymbolsByRawName(name, view->GetInternalNameSpace()); + auto it = std::ranges::find_if(symbols, [](const Ref& sym) { + return sym->GetType() != ImportAddressSymbol; + }); + return it != symbols.end() ? *it : nullptr; +} + + +static Ref ResolveBindSymbol(BinaryView* view, const MachOHeader& header, const std::string& name, int32_t ordinal) +{ + switch (ordinal) + { + case BindSpecialDylibSelf: + return FindInternalSymbol(view, name); + + case BindSpecialDylibMainExecutable: + case BindSpecialDylibFlatLookup: + case BindSpecialDylibWeakLookup: + { + Ref symbol; + + // Prefer internal symbols for executables, and external symbols for everything else. + if (header.ident.filetype == MH_EXECUTE) + symbol = FindInternalSymbol(view, name); + if (!symbol) + symbol = view->GetSymbolByRawName(name, view->GetExternalNameSpace()); + if (!symbol && header.ident.filetype != MH_EXECUTE) + symbol = FindInternalSymbol(view, name); + return symbol; + } + + default: + if (ordinal > 0) + return view->GetSymbolByRawName(name, view->GetExternalNameSpace()); + return nullptr; + } +} + + bool MachoView::InitializeHeader(MachOHeader& header, bool isMainHeader, uint64_t preferredImageBase, std::string preferredImageBaseDesc, bool platformSetByUser) { @@ -2272,86 +2317,19 @@ bool MachoView::InitializeHeader(MachOHeader& header, bool isMainHeader, uint64_ Ref symbolToLibraryMapping = new Metadata(KeyValueDataType); for (auto& [relocation, name, ordinal] : header.bindingRelocations) { - bool handled = false; - - switch (ordinal) + if (auto symbol = ResolveBindSymbol(this, header, name, ordinal); symbol) { - case BindSpecialDylibSelf: - { - // When multiple symbols are defined with the same name, which can happen for a symbol is both in the - // symbol table and self-bound, `GetSymbolByRawName` prefers the symbol with the lowest type value. - // Since `ImportAddressSymbol` is a lower value than `DataSymbol`, using `GetSymbolByRawName` would - // return the symbol representing the import we're binding to rather than the actual symbol definition. - auto symbols = GetSymbolsByRawName(name, GetInternalNameSpace()); - auto it = std::ranges::find_if(symbols, [](const Ref& sym) { - return sym->GetType() != ImportAddressSymbol; - }); - - if (it != symbols.end()) - { - auto symbol = *it; - DefineRelocation(m_arch, relocation, symbol, relocation.address); - if (objcProcessor) - objcProcessor->AddRelocatedPointer(relocation.address, symbol->GetAddress()); - handled = true; - } - break; + DefineRelocation(m_arch, relocation, symbol, relocation.address); + if (objcProcessor && symbol->GetNameSpace() == GetInternalNameSpace()) + objcProcessor->AddRelocatedPointer(relocation.address, symbol->GetAddress()); } - - case BindSpecialDylibMainExecutable: - case BindSpecialDylibFlatLookup: - case BindSpecialDylibWeakLookup: - // In cases where we are the primary executable, flat lookup should find us first, - // it seems like our best course of action is to try and find internally first on - // executables, and externally on libraries. - if (header.ident.filetype == MH_EXECUTE) - { - if (auto symbol = GetSymbolByRawName(name, GetInternalNameSpace()); symbol) - { - DefineRelocation(m_arch, relocation, symbol, relocation.address); - if (objcProcessor) - objcProcessor->AddRelocatedPointer(relocation.address, symbol->GetAddress()); - handled = true; - } - else if (auto symbol = GetSymbolByRawName(name, GetExternalNameSpace()); symbol) - { - DefineRelocation(m_arch, relocation, symbol, relocation.address); - handled = true; - } - } - else - { - if (auto symbol = GetSymbolByRawName(name, GetExternalNameSpace()); symbol) - { - DefineRelocation(m_arch, relocation, symbol, relocation.address); - handled = true; - } - else if (auto symbol = GetSymbolByRawName(name, GetInternalNameSpace()); symbol) - { - DefineRelocation(m_arch, relocation, symbol, relocation.address); - if (objcProcessor) - objcProcessor->AddRelocatedPointer(relocation.address, symbol->GetAddress()); - handled = true; - } - } - break; - - default: - if (ordinal > 0) - { - if (auto symbol = GetSymbolByRawName(name, GetExternalNameSpace())) - { - DefineRelocation(m_arch, relocation, symbol, relocation.address); - handled = true; - } - if (ordinal - 1 < header.dylibs.size()) - symbolToLibraryMapping->SetValueForKey(name, new Metadata(header.dylibs[ordinal - 1].first)); - } - break; + else + { + m_logger->LogErrorF("Failed to find symbol {:?} for bind at {:#x} (ordinal: {})", name, relocation.address, ordinal); } - if (!handled) - m_logger->LogErrorF("Failed to find external symbol {:?}, couldn't bind symbol at {:#x}", name, relocation.address); + if (ordinal > 0 && ordinal - 1 < header.dylibs.size()) + symbolToLibraryMapping->SetValueForKey(name, new Metadata(header.dylibs[ordinal - 1].first)); } StoreMetadata("SymbolExternalLibraryMapping", std::move(symbolToLibraryMapping), true); -- cgit v1.3.1