Misc shared cache improvements

- Removed last use of user object creation in objective-c
- Fixed function type info being omitted from certain images loaded automatically
- Add TODO about undo action in view init. This is not critical, just a non-actionable warning because of a design flaw

I will restate this again for anyone in the future, until the undo action system is thread-safe avoid calling it from any thread other than the main thread, it is very easy to deadlock or cause other issues. That goes for basically all user analysis object creation.

5 files changed, 49 insertions, 47 deletions

diff --git a/objectivec/objc.cpp b/objectivec/objc.cpp
index 46add246..f66c0a47 100644
--- a/objectivec/objc.cpp
+++ b/objectivec/objc.cpp
@@ -288,43 +288,38 @@ void ObjCProcessor::DefineObjCSymbol(
 			new Symbol(type, shortName, fullName, name, addr, LocalBinding, nameSpace), typeRef);
 	};
 
-	if (deferred)
-	{
-		m_symbolQueue->Append(process, [this, addr = addr](Symbol* symbol, Type* type) {
-			// Armv7/Thumb: This will rewrite the symbol's address.
-			// e.g. We pass in 0xc001, it will rewrite it to 0xc000 and create the function w/ the "thumb2" arch.
-			if (Ref<Symbol> existingSymbol = m_data->GetSymbolByAddress(addr))
-				m_data->UndefineAutoSymbol(existingSymbol);
-			auto funcSym = m_data->DefineAutoSymbolAndVariableOrFunction(m_data->GetDefaultPlatform(), symbol, type);
-			if (funcSym->GetType() == FunctionSymbol)
-			{
-				uint64_t target = symbol->GetAddress();
-				Ref<Platform> targetPlatform =
-					m_data->GetDefaultPlatform()->GetAssociatedPlatformByAddress(target);  // rewrites target.
-				if (Ref<Function> targetFunction = m_data->GetAnalysisFunction(targetPlatform, target))
-				{
-					if (!m_isBackedByDatabase)
-						targetFunction->SetUserType(type);
-				}
-			}
-		});
-		return;
-	}
+	auto defineSymbol = [this](Ref<Symbol> symbol, Ref<Type> type) {
+		uint64_t symbolAddress = symbol->GetAddress();
+		// Armv7/Thumb: This will rewrite the symbol's address.
+		// e.g. We pass in 0xc001, it will rewrite it to 0xc000 and create the function w/ the "thumb2" arch.
+		if (Ref<Symbol> existingSymbol = m_data->GetSymbolByAddress(symbolAddress))
+			m_data->UndefineAutoSymbol(existingSymbol);
+		Ref<Platform> targetPlatform = m_data->GetDefaultPlatform()->GetAssociatedPlatformByAddress(symbolAddress);
+		if (symbol->GetType() == FunctionSymbol)
+		{
+			// For thumb2 we want to get the adjusted address, we can do that using the target function.
+			Ref<Function> targetFunction = m_data->GetAnalysisFunction(targetPlatform, symbolAddress);
+			if (targetFunction && type)
+				targetFunction->ApplyAutoDiscoveredType(type);
 
-	if (Ref<Symbol> existingSymbol = m_data->GetSymbolByAddress(addr))
-		m_data->UndefineAutoSymbol(existingSymbol);
-	auto result = process();
-	auto sym = m_data->DefineAutoSymbolAndVariableOrFunction(m_data->GetDefaultPlatform(), result.first, result.second);
-	if (sym->GetType() == FunctionSymbol)
-	{
-		uint64_t target = result.first->GetAddress();
-		Ref<Platform> targetPlatform = m_data->GetDefaultPlatform()->GetAssociatedPlatformByAddress(target);  // rewrites
-		                                                                                                      // target.
-		if (Ref<Function> targetFunction = m_data->GetAnalysisFunction(targetPlatform, target))
+			auto adjustedSym = new Symbol(FunctionSymbol, symbol->GetShortName(), symbol->GetFullName(), symbol->GetRawName(), symbolAddress);
+			m_data->DefineAutoSymbol(adjustedSym);
+		}
+		else
 		{
-			if (!m_isBackedByDatabase)
-				targetFunction->SetUserType(result.second);
+			// Other symbol types can just use this, they don't need to worry about linear sweep removing them.
+			m_data->DefineAutoSymbolAndVariableOrFunction(targetPlatform, symbol, type);
 		}
+	};
+
+	if (!deferred)
+	{
+		m_symbolQueue->Append(process, defineSymbol);
+	}
+	else
+	{
+		auto [symbol, type]  = process();
+		defineSymbol(symbol, type);
 	}
 }
 
@@ -1455,11 +1450,9 @@ void ObjCProcessor::ProcessObjCData()
 
 	PostProcessObjCSections(reader.get());
 
-	auto id = m_data->BeginUndoActions();
 	m_symbolQueue->Process();
 	m_data->EndBulkModifySymbols();
 	delete m_symbolQueue;
-	m_data->ForgetUndoActions(id);
 
 	auto meta = SerializeMetadata();
 	m_data->StoreMetadata("Objective-C", meta, true);
@@ -1577,12 +1570,10 @@ void ObjCProcessor::ProcessCFStrings()
 				DefineObjCSymbol(DataSymbol, Type::NamedType(m_data, m_typeNames.cfString), "cfstr_" + str, i, true);
 			}
 		}
-		auto id = m_data->BeginUndoActions();
 		m_symbolQueue->Process();
 		m_data->EndBulkModifySymbols();
-		m_data->ForgetUndoActions(id);
+		delete m_symbolQueue;
 	}
-	delete m_symbolQueue;
 }
 
 void ObjCProcessor::AddRelocatedPointer(uint64_t location, uint64_t rewrite)
diff --git a/objectivec/objc.h b/objectivec/objc.h
index 01fb5e5a..9aeb159a 100644
--- a/objectivec/objc.h
+++ b/objectivec/objc.h
@@ -280,7 +280,7 @@ 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 = nullptr;
+		SymbolQueue* m_symbolQueue;
 		std::map<uint64_t, Class> m_classes;
 		std::map<uint64_t, Class> m_categories;
 		std::map<uint64_t, Protocol> m_protocols;
@@ -331,7 +331,6 @@ namespace BinaryNinja {
 		virtual ~ObjCProcessor() = default;
 
 		ObjCProcessor(BinaryView* data, const char* loggerName, bool isBackedByDatabase, bool skipClassBaseProtocols = false);
-		// TODO: Instead of passing in image name the processor must be given section refs in a structure that outlines all objc sections.
 		void ProcessObjCData();
 		void ProcessCFStrings();
 		void AddRelocatedPointer(uint64_t location, uint64_t rewrite);
diff --git a/view/sharedcache/core/SharedCacheView.cpp b/view/sharedcache/core/SharedCacheView.cpp
index b226e302..e9699ab1 100644
--- a/view/sharedcache/core/SharedCacheView.cpp
+++ b/view/sharedcache/core/SharedCacheView.cpp
@@ -956,6 +956,10 @@ bool SharedCacheView::InitController()
 	m_logger->LogDebug("Loading images using pattern: %s", autoLoadPattern.c_str());
 
 	{
+		// TODO: Refusing to add undo action "Added section libsystem_c.dylib::__macho_header", there is literally
+		// TODO: no way around this warning as undo actions are implicit with user sections, for more detail see:
+		// TODO:	- https://github.com/Vector35/binaryninja-api/issues/6742
+		// TODO:	- https://github.com/Vector35/binaryninja-api/issues/6289
 		// Load all images that match the `autoLoadPattern`.
 		auto startTime = std::chrono::high_resolution_clock::now();
 		size_t loadedImages = 0;
diff --git a/view/sharedcache/core/Utility.cpp b/view/sharedcache/core/Utility.cpp
index 8f49c6bf..6f8323bf 100644
--- a/view/sharedcache/core/Utility.cpp
+++ b/view/sharedcache/core/Utility.cpp
@@ -90,8 +90,11 @@ void ApplySymbol(Ref<BinaryView> view, Ref<TypeLibrary> typeLib, Ref<Symbol> sym
 		// Make sure to check for already added function from the function table.
 		// Unless we have retrieved a type here we don't need to make a new function.
 		func = view->GetAnalysisFunction(targetPlatform, symbolAddress);
-		if (!func || selectedType)
+		if (!func || selectedType != nullptr)
 			func = view->AddFunctionForAnalysis(targetPlatform, symbolAddress, false, selectedType);
+		// The above function might be overwritten so we also want to apply the type here.
+		if (func && selectedType != nullptr)
+			func->ApplyAutoDiscoveredType(selectedType);
 	}
 	else
 	{
diff --git a/view/sharedcache/workflow/SharedCacheWorkflow.cpp b/view/sharedcache/workflow/SharedCacheWorkflow.cpp
index cde8310c..3fce2e39 100644
--- a/view/sharedcache/workflow/SharedCacheWorkflow.cpp
+++ b/view/sharedcache/workflow/SharedCacheWorkflow.cpp
@@ -87,7 +87,7 @@ void IdentifyStub(BinaryView& view, const SharedCacheController& controller, uin
 	if (!symbol.has_value())
 		return;
 
-	// Demangle if possible, the type pulled will be used, however type library will take precedence.
+	// TODO: The demangled type here is almost always wrong so we omit it for now.
 	auto [demangledName, demangledType] = symbol->DemangledName(view);
 	auto rawName = STUB_PREFIX + symbol->name;
 	auto shortName = STUB_PREFIX + demangledName;
@@ -97,13 +97,16 @@ void IdentifyStub(BinaryView& view, const SharedCacheController& controller, uin
 	{
 		// NOTE: The type library name is expected to be the image name currently.
 		// Try and pull the type from the associated type library (if there is one)
-		Ref<Type> selectedType = demangledType;
+		// TODO: The demangled type here is missing a param
+		// Ref<Type> selectedType = demangledType;
+		Ref<Type> selectedType = nullptr;
 		if (const auto image = controller.GetImageContaining(symbolAddr))
 			if (auto typeLib = TypeLibraryFromName(view, image->name))
-				selectedType = view.ImportTypeLibraryObject(typeLib, {symbol->name});
+				if (Ref<Type> libraryType = view.ImportTypeLibraryObject(typeLib, {symbol->name}); libraryType)
+					selectedType = libraryType;
 
-		if (selectedType)
-			targetFunc->SetAutoType(selectedType);
+		if (selectedType != nullptr)
+			targetFunc->ApplyAutoDiscoveredType(selectedType);
 	}
 
 	// Define the new symbol!
@@ -167,6 +170,8 @@ void AnalyzeStubFunction(Ref<Function> func, Ref<MediumLevelILFunction> mlil, Sh
 				if (defInstr.operation != MLIL_SET_VAR_SSA)
 					return;
 				expr = defInstr.GetSourceExpr<MLIL_SET_VAR_SSA>();
+				if (expr.operation != MLIL_LOAD_SSA)
+					return;
 				// Fallthrough to MLIL_LOAD_SSA.
 			}
 		case MLIL_LOAD_SSA: