From f628cc695c680469c441394511a72b3b3912dd7f Mon Sep 17 00:00:00 2001 From: Mark Rowe Date: Mon, 7 Jul 2025 22:25:22 -0700 Subject: [ObjC] Avoid leaking SymbolQueue `SymbolQueue`'s lifetime was being managed via `new` / `delete`. This was error-prone in the face of code that can a) throw exceptions, or b) return early. Typically the solution would be to move to `std::unique_ptr` and call it a day, but nothing is ever easy. The `SymbolQueue`s created by `ObjCProcessor` are intended to live for the duration of `ProcessObjCData` and `ProcessCFStrings` methods. Since they are used multiple levels down the call tree, the active `SymbolQueue` is stored as a member variable on `ObjCProcessor`. Switching to `std::unique_ptr` would ensure that the `SymbolQueue` is destroyed, but would leave a dangling pointer in the member variable. To address this I'm introducing a `ScopedSingleton` class that provides a thread-local singleton whose lifetime is controlled by a guard object. `ProcessObjCData` / `ProcessCFStrings` call `ScopedSymbolQueue::Make` and store the returned guard object in a local. Code that accesses the symbol queue uses `ScopedSymbolQueue::Get` to retrieve the current instance. The guard object ensures the `SymbolQueue` is deleted and the `current` pointer is cleared no matter how the scope is exited. A better longer-term design is to introduce a class for processing the Objective-C runtime metadata and a class for processing constants such as `CFString`s. These could directly own the symbol queue so it would be accessible to any member functions that define symbols. This is a more involved refactoring than I have time for right now. --- objectivec/objc.cpp | 67 ++++++++++++++++++++++++++++++++++++++++++++++------- objectivec/objc.h | 1 - 2 files changed, 59 insertions(+), 9 deletions(-) (limited to 'objectivec') diff --git a/objectivec/objc.cpp b/objectivec/objc.cpp index b45cb3ed..6316c11f 100644 --- a/objectivec/objc.cpp +++ b/objectivec/objc.cpp @@ -2,10 +2,60 @@ #include "inttypes.h" #include +#define RELEASE_ASSERT(condition) ((condition) ? (void)0 : (std::abort(), (void)0)) + using namespace BinaryNinja; namespace { + // ScopedSingleton is a thread-local singleton that allows for scoped + // instantiation and destruction of an object. It is useful for managing + // resources that should only exist during a specific scope, but where it + // would be inconvenient to pass the object around explicitly. + // + // Calling `Make` initializes the thread-local singleton and returns a `Guard` + // object. When the `Guard` object goes out of scope, the singleton is destroyed. + template + class ScopedSingleton + { + static thread_local T* current; + + public: + class Guard + { + friend class ScopedSingleton; + Guard() = default; + + public: + ~Guard() + { + delete current; + current = nullptr; + } + Guard(Guard&&) = default; + Guard(const Guard&) = delete; + Guard& operator=(const Guard&) = delete; + }; + + static T& Get() + { + RELEASE_ASSERT(current); + return *current; + } + + static Guard Make() + { + RELEASE_ASSERT(!current); + current = new T(); + return Guard {}; + } + }; + + template + thread_local T* ScopedSingleton::current = nullptr; + + using ScopedSymbolQueue = ScopedSingleton; + // Attempt to recover an Objective-C class name from the symbol's name. // Note: classes defined in the current image should be looked up in m_classes // rather than using this function. @@ -56,7 +106,7 @@ namespace { return component; } -} +} // namespace Ref ObjCProcessor::SerializeMethod(uint64_t loc, const Method& method) { @@ -369,7 +419,7 @@ void ObjCProcessor::DefineObjCSymbol( if (!deferred) { - m_symbolQueue->Append(process, defineSymbol); + ScopedSymbolQueue::Get().Append(process, defineSymbol); } else { @@ -1331,7 +1381,8 @@ Ref ObjCProcessor::GetSymbol(uint64_t address) void ObjCProcessor::ProcessObjCData() { - m_symbolQueue = new SymbolQueue(); + auto guard = ScopedSymbolQueue::Make(); + auto addrSize = m_data->GetAddressSize(); m_typeNames.id = defineTypedef(m_data, {"id"}, Type::PointerType(addrSize, Type::VoidType())); @@ -1525,9 +1576,8 @@ void ObjCProcessor::ProcessObjCData() PostProcessObjCSections(reader.get()); - m_symbolQueue->Process(); + ScopedSymbolQueue::Get().Process(); m_data->EndBulkModifySymbols(); - delete m_symbolQueue; auto meta = SerializeMetadata(); m_data->StoreMetadata("Objective-C", meta, true); @@ -1547,7 +1597,8 @@ void ObjCProcessor::ProcessObjCLiterals() void ObjCProcessor::ProcessCFStrings() { - m_symbolQueue = new SymbolQueue(); + auto guard = ScopedSymbolQueue::Make(); + uint64_t ptrSize = m_data->GetAddressSize(); // https://github.com/apple/llvm-project/blob/next/clang/lib/CodeGen/CodeGenModule.cpp#L6129 // See also ASTContext.cpp ctrl+f __NSConstantString_tag @@ -1654,9 +1705,9 @@ void ObjCProcessor::ProcessCFStrings() DefineObjCSymbol(DataSymbol, Type::NamedType(m_data, m_typeNames.cfString), "cfstr_" + str, i, true); } } - m_symbolQueue->Process(); + + ScopedSymbolQueue::Get().Process(); m_data->EndBulkModifySymbols(); - delete m_symbolQueue; } } diff --git a/objectivec/objc.h b/objectivec/objc.h index 61df2b0d..6d194971 100644 --- a/objectivec/objc.h +++ b/objectivec/objc.h @@ -286,7 +286,6 @@ namespace BinaryNinja { // TODO(WeiN76LQh): this is to avoid a bug with defining a classes protocol list in the DSC plugin. Remove once fixed bool m_skipClassBaseProtocols; - SymbolQueue* m_symbolQueue; std::map m_classes; std::map m_categories; std::map m_protocols; -- cgit v1.3.1