[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.

2 files changed, 59 insertions, 9 deletions

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 <optional>
 
+#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 <typename T>
+	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 <typename T>
+	thread_local T* ScopedSingleton<T>::current = nullptr;
+
+	using ScopedSymbolQueue = ScopedSingleton<SymbolQueue>;
+
 	// 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<Metadata> 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<Symbol> 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<uint64_t, Class> m_classes;
 		std::map<uint64_t, Class> m_categories;
 		std::map<uint64_t, Protocol> m_protocols;