Introduce an RAII type for managing bulk symbol modifications

Fixes https://github.com/Vector35/binaryninja-api/issues/7666.

Correctly managing the state of bulk symbol modifications via
`BeginBulkModifySymbols` / `EndBulkModifySymbols` is error-prone in the
face of exceptions and early returns. Leaking a bulk symbol modification
can leave the view in a state where no further changes to symbols will
be applied.

All users of the C++ API are encouraged to move from
`BeginBulkModifySymbols` / `EndBulkModifySymbols` to the new
`BulkSymbolModification` class.

8 files changed, 84 insertions, 29 deletions

diff --git a/binaryninjaapi.h b/binaryninjaapi.h
index 9cef3ee0..55aa96ca 100644
--- a/binaryninjaapi.h
+++ b/binaryninjaapi.h
@@ -6869,7 +6869,24 @@ namespace BinaryNinja {
 		*/
 		bool IsApplyingDebugInfo() const;
 
+		/*! Begin an operation that may potentially modify many symbols
+
+			When performing bulk symbol modifications, surrounding the modifications with
+			\c BeginBulkModifySymbols and \c EndBulkModifySymbols will defer some work until
+			the end of the operation, improving performance.
+
+			\deprecated Prefer \c BulkSymbolModification for bulk symbol modifications.
+		*/
 		void BeginBulkModifySymbols();
+
+		/*! Finish an operation that potentially modified many symbols
+
+			When performing bulk symbol modifications, surrounding the modifications with
+			\c BeginBulkModifySymbols and \c EndBulkModifySymbols will defer some work until
+			the end of the operation, improving performance.
+
+			\deprecated Prefer \c BulkSymbolModification for bulk symbol modifications.
+		*/
 		void EndBulkModifySymbols();
 
 		/*! Add a new TagType to this binaryview
@@ -8300,6 +8317,54 @@ namespace BinaryNinja {
 		}
 	};
 
+	/*! An RAII helper for use when modifying symbols in bulk.
+
+		\ingroup binaryview
+
+		When modifying many symbols, it is more efficient to wrap the modifications
+		in a \c BulkSymbolModification object. This will defer some processing until
+		all bulk modifications are complete.
+
+		While a bulk modification is active, some API calls may not reflect (or may
+		only partially reflect) any symbol changes made until all bulk modifications
+		are complete.
+
+		The bulk modification will be active for the lifetime of the
+		\c BulkSymbolModification object. It can be ended early via the \c End method,
+		if needed.
+	*/
+	class BulkSymbolModification
+	{
+	public:
+		BulkSymbolModification(Ref<BinaryView> view)
+			: m_view(std::move(view))
+		{
+			m_view->BeginBulkModifySymbols();
+		}
+
+		~BulkSymbolModification()
+		{
+			if (m_view)
+				m_view->EndBulkModifySymbols();
+		}
+
+		/*! End this bulk modification early. */
+		void End()
+		{
+			m_view->EndBulkModifySymbols();
+			m_view = nullptr;
+		}
+
+		// Move-only.
+		BulkSymbolModification(const BulkSymbolModification&) = delete;
+		BulkSymbolModification& operator=(const BulkSymbolModification&) = delete;
+		BulkSymbolModification(BulkSymbolModification&&) = default;
+		BulkSymbolModification& operator=(BulkSymbolModification&&) = default;
+
+	private:	
+		Ref<BinaryView> m_view;
+	};
+
 	/*!
 		\ingroup binaryview
 	*/
diff --git a/objectivec/objc.cpp b/objectivec/objc.cpp
index d4476de4..170cd78a 100644
--- a/objectivec/objc.cpp
+++ b/objectivec/objc.cpp
@@ -1584,7 +1584,7 @@ void ObjCProcessor::ProcessObjCData()
 	protocolBuilder.AddMember(Type::IntegerType(4, false), "flags");
 	m_typeNames.protocol = finalizeStructureBuilder(m_data, protocolBuilder, "objc_protocol_t").first;
 
-	m_data->BeginBulkModifySymbols();
+	BulkSymbolModification bulkSymbolModification(m_data);
 	if (auto classList = GetSectionWithName("__objc_classlist"))
 		LoadClasses(reader.get(), classList);
 	if (auto nonLazyClassList = GetSectionWithName("__objc_nlclslist"))
@@ -1607,7 +1607,6 @@ void ObjCProcessor::ProcessObjCData()
 	PostProcessObjCSections(reader.get());
 
 	ScopedSymbolQueue::Get().Process();
-	m_data->EndBulkModifySymbols();
 
 	auto meta = SerializeMetadata();
 	m_data->StoreMetadata("Objective-C", meta, true);
@@ -1666,7 +1665,7 @@ void ObjCProcessor::ProcessCFStrings()
 		auto start = cfstrings->GetStart();
 		auto end = cfstrings->GetEnd();
 		auto typeWidth = Type::NamedType(m_data, m_typeNames.cfString)->GetWidth();
-		m_data->BeginBulkModifySymbols();
+		BulkSymbolModification bulkSymbolModification(m_data);
 		for (view_ptr_t i = start; i < end; i += typeWidth)
 		{
 			reader->Seek(i + ptrSize);
@@ -1737,7 +1736,6 @@ void ObjCProcessor::ProcessCFStrings()
 		}
 
 		ScopedSymbolQueue::Get().Process();
-		m_data->EndBulkModifySymbols();
 	}
 }
 
@@ -1759,7 +1757,7 @@ void ObjCProcessor::ProcessNSConstantArrays()
 		auto start = arrays->GetStart();
 		auto end = arrays->GetEnd();
 		auto typeWidth = Type::NamedType(m_data, m_typeNames.nsConstantArray)->GetWidth();
-		m_data->BeginBulkModifySymbols();
+		BulkSymbolModification bulkSymbolModification(m_data);
 		for (view_ptr_t i = start; i < end; i += typeWidth)
 		{
 			reader->Seek(i + ptrSize);
@@ -1771,7 +1769,6 @@ void ObjCProcessor::ProcessNSConstantArrays()
 				fmt::format("nsarray_{:x}", i), i, true);
 		}
 		ScopedSymbolQueue::Get().Process();
-		m_data->EndBulkModifySymbols();
 	}
 	
 }
@@ -1796,7 +1793,7 @@ void ObjCProcessor::ProcessNSConstantDictionaries()
 		auto start = dicts->GetStart();
 		auto end = dicts->GetEnd();
 		auto typeWidth = Type::NamedType(m_data, m_typeNames.nsConstantDictionary)->GetWidth();
-		m_data->BeginBulkModifySymbols();
+		BulkSymbolModification bulkSymbolModification(m_data);
 		for (view_ptr_t i = start; i < end; i += typeWidth)
 		{
 			reader->Seek(i + (ptrSize * 2));
@@ -1812,7 +1809,6 @@ void ObjCProcessor::ProcessNSConstantDictionaries()
 				fmt::format("nsdict_{:x}", i), i, true);
 		}
 		ScopedSymbolQueue::Get().Process();
-		m_data->EndBulkModifySymbols();
 	}
 }
 
@@ -1834,7 +1830,7 @@ void ObjCProcessor::ProcessNSConstantIntegerNumbers()
 		auto start = numbers->GetStart();
 		auto end = numbers->GetEnd();
 		auto typeWidth = Type::NamedType(m_data, m_typeNames.nsConstantIntegerNumber)->GetWidth();
-		m_data->BeginBulkModifySymbols();
+		BulkSymbolModification bulkSymbolModification(m_data);
 		for (view_ptr_t i = start; i < end; i += typeWidth)
 		{
 			reader->Seek(i + ptrSize);
@@ -1867,7 +1863,6 @@ void ObjCProcessor::ProcessNSConstantIntegerNumbers()
 			}
 		}
 		ScopedSymbolQueue::Get().Process();
-		m_data->EndBulkModifySymbols();
 	}
 }
 
@@ -1917,7 +1912,7 @@ void ObjCProcessor::ProcessNSConstantFloatingPointNumbers()
 		auto start = numbers->GetStart();
 		auto end = numbers->GetEnd();
 		auto typeWidth = Type::NamedType(m_data, m_typeNames.nsConstantDoubleNumber)->GetWidth();
-		m_data->BeginBulkModifySymbols();
+		BulkSymbolModification bulkSymbolModification(m_data);
 		for (view_ptr_t i = start; i < end; i += typeWidth)
 		{
 			reader->Seek(i + ptrSize);
@@ -1955,7 +1950,6 @@ void ObjCProcessor::ProcessNSConstantFloatingPointNumbers()
 			DefineObjCSymbol(DataSymbol, Type::NamedType(m_data, *typeName), name, i, true);
 		}
 		ScopedSymbolQueue::Get().Process();
-		m_data->EndBulkModifySymbols();
 	}
 }
 
@@ -1977,7 +1971,7 @@ void ObjCProcessor::ProcessNSConstantDatas()
 		auto start = datas->GetStart();
 		auto end = datas->GetEnd();
 		auto typeWidth = Type::NamedType(m_data, m_typeNames.nsConstantData)->GetWidth();
-		m_data->BeginBulkModifySymbols();
+		BulkSymbolModification bulkSymbolModification(m_data);
 		for (view_ptr_t i = start; i < end; i += typeWidth)
 		{
 			reader->Seek(i + ptrSize);
@@ -1989,7 +1983,6 @@ void ObjCProcessor::ProcessNSConstantDatas()
 				DataSymbol, Type::NamedType(m_data, m_typeNames.nsConstantData), fmt::format("nsdata_{:x}", i), i, true);
 		}
 		ScopedSymbolQueue::Get().Process();
-		m_data->EndBulkModifySymbols();
 	}
 }
 
diff --git a/plugins/rtti/itanium.cpp b/plugins/rtti/itanium.cpp
index 63f2c563..34fd0f71 100644
--- a/plugins/rtti/itanium.cpp
+++ b/plugins/rtti/itanium.cpp
@@ -760,7 +760,7 @@ void ItaniumRTTIProcessor::ProcessRTTI()
             m_logger->LogWarn("Too many failed scans for section %llx... skipping", section->GetStart());
     };
 
-    m_view->BeginBulkModifySymbols();
+    BulkSymbolModification bulkSymbolModification(m_view);
     // Scan data sections for rtti.
     for (const Ref<Section> &section : m_view->GetSections())
     {
@@ -782,7 +782,7 @@ void ItaniumRTTIProcessor::ProcessRTTI()
             }
         }
     }
-    m_view->EndBulkModifySymbols();
+    bulkSymbolModification.End();
 
     // Go through all classes and recurse into the base classes using the base class name
     for (auto &[classAddr, classInfo]: m_classInfo)
diff --git a/view/elf/elfview.cpp b/view/elf/elfview.cpp
index 0e206032..e473795f 100644
--- a/view/elf/elfview.cpp
+++ b/view/elf/elfview.cpp
@@ -1053,7 +1053,7 @@ bool ElfView::Init()
 		m_logger->LogError("ELF relocation table invalid");
 	}
 
-	BeginBulkModifySymbols();
+	BulkSymbolModification bulkSymbolModification(this);
 
 	vector<ElfSymbolTableEntry> auxSymbolTable;
 	try
@@ -1408,7 +1408,7 @@ bool ElfView::Init()
 	delete m_symbolQueue;
 	m_symbolQueue = nullptr;
 
-	EndBulkModifySymbols();
+	bulkSymbolModification.End();
 
 	auto relocHandler = m_arch->GetRelocationHandler("ELF");
 	if (relocHandler)
diff --git a/view/kernelcache/core/MachOProcessor.cpp b/view/kernelcache/core/MachOProcessor.cpp
index fb3d2b27..5731d80e 100644
--- a/view/kernelcache/core/MachOProcessor.cpp
+++ b/view/kernelcache/core/MachOProcessor.cpp
@@ -50,7 +50,7 @@ void KernelCacheMachOProcessor::ApplyHeader(const KernelCache& cache, KernelCach
 			m_view->AddFunctionForAnalysis(targetPlatform, func, false);
 	}
 
-	m_view->BeginBulkModifySymbols();
+	BulkSymbolModification bulkSymbolModification(m_view);
 
 	// Apply symbols from symbol table.
 	if (header.symtab.symoff != 0)
@@ -78,7 +78,6 @@ void KernelCacheMachOProcessor::ApplyHeader(const KernelCache& cache, KernelCach
 			ApplySymbol(m_view, typeLib, symbol, symbolType);
 		}
 	}
-	m_view->EndBulkModifySymbols();
 }
 
 uint64_t KernelCacheMachOProcessor::ApplyHeaderSections(KernelCacheMachOHeader& header)
diff --git a/view/macho/machoview.cpp b/view/macho/machoview.cpp
index 7353e995..1c8d8512 100644
--- a/view/macho/machoview.cpp
+++ b/view/macho/machoview.cpp
@@ -2088,7 +2088,7 @@ bool MachoView::InitializeHeader(MachOHeader& header, bool isMainHeader, uint64_
 			ParseFunctionStarts(GetDefaultPlatform(), header.textBase, header.functionStarts);
 	}
 
-	BeginBulkModifySymbols();
+	BulkSymbolModification bulkSymbolModification(this);
 	m_symbolQueue = new SymbolQueue();
 
 	try
@@ -2105,8 +2105,7 @@ bool MachoView::InitializeHeader(MachOHeader& header, bool isMainHeader, uint64_
 	m_symbolQueue->Process();
 	delete m_symbolQueue;
 	m_symbolQueue = nullptr;
-
-	EndBulkModifySymbols();
+	bulkSymbolModification.End();
 
 	for (auto& relocation : header.rebaseRelocations)
 	{
diff --git a/view/pe/peview.cpp b/view/pe/peview.cpp
index 8537a036..ffe9c91a 100644
--- a/view/pe/peview.cpp
+++ b/view/pe/peview.cpp
@@ -1307,7 +1307,8 @@ bool PEView::Init()
 	}
 
 	vector<pair<BNRelocationInfo, string>> relocs;
-	BeginBulkModifySymbols();
+	
+	BulkSymbolModification bulkSymbolModification(this);
 	m_symbolQueue = new SymbolQueue();
 	m_symExternMappingMetadata = new Metadata(KeyValueDataType);
 
@@ -2582,7 +2583,7 @@ bool PEView::Init()
 	delete m_symbolQueue;
 	m_symbolQueue = nullptr;
 
-	EndBulkModifySymbols();
+	bulkSymbolModification.End();
 
 	StoreMetadata("SymbolExternalLibraryMapping", m_symExternMappingMetadata, true);
 
diff --git a/view/sharedcache/core/MachOProcessor.cpp b/view/sharedcache/core/MachOProcessor.cpp
index 281f40c2..31fdbe6c 100644
--- a/view/sharedcache/core/MachOProcessor.cpp
+++ b/view/sharedcache/core/MachOProcessor.cpp
@@ -52,7 +52,7 @@ void SharedCacheMachOProcessor::ApplyHeader(const SharedCache& cache, SharedCach
 				m_view->AddFunctionForAnalysis(targetPlatform, func, false);
 		}
 
-		m_view->BeginBulkModifySymbols();
+		BulkSymbolModification bulkSymbolModification(m_view);
 
 		// Apply symbols from symbol table.
 		if (header.symtab.symoff != 0)
@@ -80,7 +80,6 @@ void SharedCacheMachOProcessor::ApplyHeader(const SharedCache& cache, SharedCach
 				ApplySymbol(m_view, typeLib, symbol, symbolType);
 			}
 		}
-		m_view->EndBulkModifySymbols();
 	}
 
 	// Apply symbols from the .symbols cache files.
@@ -127,14 +126,13 @@ void SharedCacheMachOProcessor::ApplyUnmappedLocalSymbols(const SharedCache& cac
 		uint64_t symbolTableStart = localSymbolsAddr + (localSymbolsEntry.nlistStartIndex * sizeof(nlist_64));
 		TableInfo symbolInfo = {symbolTableStart, localSymbolsEntry.nlistCount};
 		TableInfo stringInfo = {localStringsAddr, localSymbolsInfo.stringsSize};
-		m_view->BeginBulkModifySymbols();
+		BulkSymbolModification bulkSymbolModification(m_view);
 		const auto symbols = header.ReadSymbolTable(*localSymbolsVM, symbolInfo, stringInfo);
 		for (const auto &sym: symbols)
 		{
 			auto [symbol, symbolType] = sym.GetBNSymbolAndType(*m_view);
 			ApplySymbol(m_view, typeLib, std::move(symbol), std::move(symbolType));
 		}
-		m_view->EndBulkModifySymbols();
 		return;
 	}
 }