[SharedCache] Fix some bugs

Author: emesare

objectivec/objc.cpp

@@ -592,7 +592,7 @@ void ObjCProcessor::LoadCategories(ObjCReader* reader, Ref<Section> classPtrSect
 		}
 		if (categoryBaseClassName.empty())
 		{
-			m_logger->LogError(
+			m_logger->LogWarn(
 				"Failed to determine base classname for category at 0x%llx. Using base address as stand-in classname",
 				catLocation);
 			categoryBaseClassName = std::to_string(catLocation);
@@ -604,7 +604,7 @@ void ObjCProcessor::LoadCategories(ObjCReader* reader, Ref<Section> classPtrSect
 		}
 		catch (...)
 		{
-			m_logger->LogError(
+			m_logger->LogWarn(
 				"Failed to read category name for category at 0x%llx. Using base address as stand-in category name",
 				catLocation);
 			categoryAdditionsName = std::to_string(catLocation);

view/sharedcache/api/sharedcache.cpp

@@ -9,21 +9,21 @@ using namespace SharedCacheAPI;
 
 BNSharedCacheImage ImageToApi(CacheImage image)
 {
-	BNSharedCacheImage apiImage;
+	BNSharedCacheImage apiImage {};
 	apiImage.name = BNAllocString(image.name.c_str());
 	apiImage.headerAddress = image.headerAddress;
 	apiImage.regionStartCount = image.regionStarts.size();
-	// TODO: If we alloc then core cannot delete
 	uint64_t *regionStarts = new uint64_t[image.regionStarts.size()];
 	for (size_t i = 0; i < image.regionStarts.size(); i++)
 		regionStarts[i] = image.regionStarts[i];
-	apiImage.regionStarts = regionStarts;
+	apiImage.regionStarts = BNSharedCacheAllocRegionList(regionStarts, image.regionStarts.size());
+	delete[] regionStarts;
 	return apiImage;
 }
 
 CacheImage ImageFromApi(BNSharedCacheImage image)
 {
-	CacheImage apiImage;
+	CacheImage apiImage {};
 	apiImage.name = image.name;
 	apiImage.headerAddress = image.headerAddress;
 	apiImage.regionStarts.reserve(image.regionStartCount);
@@ -34,7 +34,7 @@ CacheImage ImageFromApi(BNSharedCacheImage image)
 
 BNSharedCacheRegion RegionToApi(const CacheRegion &region)
 {
-	BNSharedCacheRegion apiRegion;
+	BNSharedCacheRegion apiRegion {};
 	apiRegion.vmAddress = region.start;
 	apiRegion.name = BNAllocString(region.name.c_str());
 	apiRegion.size = region.size;
@@ -47,7 +47,7 @@ BNSharedCacheRegion RegionToApi(const CacheRegion &region)
 
 CacheRegion RegionFromApi(BNSharedCacheRegion apiRegion)
 {
-	CacheRegion region;
+	CacheRegion region {};
 	region.start = apiRegion.vmAddress;
 	region.name = apiRegion.name;
 	region.size = apiRegion.size;
@@ -58,7 +58,7 @@ CacheRegion RegionFromApi(BNSharedCacheRegion apiRegion)
 
 BNSharedCacheMappingInfo MappingToApi(const CacheMappingInfo &mapping)
 {
-	BNSharedCacheMappingInfo apiMapping;
+	BNSharedCacheMappingInfo apiMapping {};
 	apiMapping.vmAddress = mapping.vmAddress;
 	apiMapping.size = mapping.size;
 	apiMapping.fileOffset = mapping.fileOffset;
@@ -67,7 +67,7 @@ BNSharedCacheMappingInfo MappingToApi(const CacheMappingInfo &mapping)
 
 CacheMappingInfo MappingFromApi(BNSharedCacheMappingInfo apiMapping)
 {
-	CacheMappingInfo mapping;
+	CacheMappingInfo mapping {};
 	mapping.vmAddress = apiMapping.vmAddress;
 	mapping.size = apiMapping.size;
 	mapping.fileOffset = apiMapping.fileOffset;
@@ -76,7 +76,7 @@ CacheMappingInfo MappingFromApi(BNSharedCacheMappingInfo apiMapping)
 
 BNSharedCacheEntry EntryToApi(const CacheEntry &entry)
 {
-	BNSharedCacheEntry apiEntry;
+	BNSharedCacheEntry apiEntry {};
 	apiEntry.path = BNAllocString(entry.path.c_str());
 	apiEntry.entryType = entry.entryType;
 	const auto &mappings = entry.mappings;
@@ -90,7 +90,7 @@ BNSharedCacheEntry EntryToApi(const CacheEntry &entry)
 
 CacheEntry EntryFromApi(BNSharedCacheEntry apiEntry)
 {
-	CacheEntry entry;
+	CacheEntry entry {};
 	entry.path = apiEntry.path;
 	entry.entryType = apiEntry.entryType;
 	entry.mappings.reserve(apiEntry.mappingCount);
@@ -125,6 +125,21 @@ std::string SharedCacheAPI::GetRegionTypeAsString(const BNSharedCacheRegionType
 	}
 }
 
+std::string SharedCacheAPI::GetSymbolTypeAsString(const BNSymbolType &type)
+{
+	// NOTE: We currently only use the function and data symbol for cache symbols.
+	// update this if that changes.
+	switch (type)
+	{
+	case FunctionSymbol:
+		return "Function";
+	case DataSymbol:
+		return "Data";
+	default:
+		return "Unknown";
+	}
+}
+
 SharedCacheController::SharedCacheController(BNSharedCacheController *controller)
 {
 	m_object = controller;

view/sharedcache/api/sharedcacheapi.h

@@ -289,6 +289,8 @@ namespace SharedCacheAPI {
 		std::string name;
 	};
 
+	std::string GetSymbolTypeAsString(const BNSymbolType& type);
+
 	class SharedCacheController : public DSCCoreRefCountObject<BNSharedCacheController, BNNewSharedCacheControllerReference, BNFreeSharedCacheControllerReference> {
 	public:
 		explicit SharedCacheController(BNSharedCacheController* controller);

view/sharedcache/core/MachOProcessor.cpp

@@ -35,11 +35,7 @@ void SharedCacheMachOProcessor::ApplyHeader(SharedCacheMachOHeader& header)
 			auto targetPlatform = m_view->GetDefaultPlatform();
 			auto functions = header.ReadFunctionTable(*m_vm);
 			for (const auto& func : functions)
-			{
-				// TODO: Check to make sure the func exists in a loaded region?
-				// TODO: ^ this check existed prior so we should add it back.
-				m_view->AddFunctionForAnalysis(targetPlatform, func);
-			}
+				m_view->AddFunctionForAnalysis(targetPlatform, func, true);
 		}
 
 		auto typeLib = m_view->GetTypeLibrary(header.installName);
@@ -52,10 +48,9 @@ void SharedCacheMachOProcessor::ApplyHeader(SharedCacheMachOHeader& header)
 			// Mach-O View symtab processing with
 			// a ton of stuff cut out so it can work
 			// NOTE: This table is read relative to the link edit segment file base.
-			// TODO: Remove this and use the m_symbols in the cache?
 			const auto symbols = header.ReadSymbolTable(*m_view, *m_vm);
 			for (const auto& sym : symbols)
-				ApplySymbol(m_view, typeLib, sym.ToBNSymbol());
+				ApplySymbol(m_view, typeLib, sym.ToBNSymbol(*m_view));
 		}
 
 		// Apply symbols from export trie.
@@ -65,7 +60,7 @@ void SharedCacheMachOProcessor::ApplyHeader(SharedCacheMachOHeader& header)
 			// TODO: Remove this and use the m_symbols in the cache?
 			const auto exportSymbols = header.ReadExportSymbolTrie(*m_vm);
 			for (const auto& sym : exportSymbols)
-				ApplySymbol(m_view, typeLib, sym.ToBNSymbol());
+				ApplySymbol(m_view, typeLib, sym.ToBNSymbol(*m_view));
 		}
 		m_view->EndBulkModifySymbols();
 	}

view/sharedcache/core/SharedCache.cpp

@@ -11,11 +11,12 @@ using namespace BinaryNinja;
 // The next id to use when calling Cache::AddEntry
 static CacheEntryId nextId = 1;
 
-Ref<Symbol> CacheSymbol::ToBNSymbol() const
+Ref<Symbol> CacheSymbol::ToBNSymbol(BinaryView& view) const
 {
 	QualifiedName qname;
+	Ref<Type> outType;
 	std::string shortName = name;
-	if (DemangleLLVM(name, qname, true))
+	if (DemangleGeneric(view.GetDefaultArchitecture(), name, outType, qname, &view, true))
 		shortName = qname.GetString();
 	return new Symbol(type, shortName, shortName, name, address, nullptr);
 }
@@ -458,7 +459,7 @@ std::optional<CacheImage> SharedCache::GetImageContaining(const uint64_t address
 {
 	// TODO: What if we are using this on a shared region? Return a list of images?
 	auto region = GetRegionContaining(address);
-	if (region.has_value())
+	if (region.has_value() && region->imageStart.has_value())
 		return GetImageAt(*region->imageStart);
 	return std::nullopt;
 }

view/sharedcache/core/SharedCache.h

@@ -32,7 +32,7 @@ struct CacheSymbol
 	CacheSymbol& operator=(CacheSymbol&& other) noexcept = default;
 
 	// NOTE: you should really only call this when adding the symbol to the view.
-	BinaryNinja::Ref<BinaryNinja::Symbol> ToBNSymbol() const;
+	BinaryNinja::Ref<BinaryNinja::Symbol> ToBNSymbol(BinaryNinja::BinaryView& view) const;
 };
 
 enum class CacheRegionType

view/sharedcache/core/SharedCacheController.cpp

@@ -60,6 +60,9 @@ SharedCacheController::SharedCacheController(SharedCache cache, Ref<Logger> logg
 	m_logger = std::move(logger);
 	m_loadedRegions = {};
 	m_loadedImages = {};
+	m_processObjC = true;
+	m_processCFStrings = true;
+	m_regionFilter = std::regex(".*LINKEDIT.*");
 }
 
 DSCRef<SharedCacheController> SharedCacheController::Initialize(BinaryView& view, SharedCache cache)
@@ -111,16 +114,16 @@ bool SharedCacheController::ApplyRegionAtAddress(BinaryView& view, const uint64_
 
 bool SharedCacheController::ApplyRegion(BinaryView& view, const CacheRegion& region)
 {
-	// TODO: Check m_loadedRegions? This seems redundant...
+	std::unique_lock<std::shared_mutex> lock(m_loadMutex);
 	// Loads the given region into the BinaryView and marks it as loaded.
-	// First check to make sure there isn't already a segment at the address in the view.
-	if (view.GetSegmentAt(region.start))
+	// First check to make sure we haven't already loaded the region.
+	if (m_loadedRegions.find(region.start) != m_loadedRegions.end())
 		return false;
 
 	// Skip filtered regions, this defaults to just LINKEDIT regions.
 	if (std::regex_match(region.name, m_regionFilter))
 	{
-		LogDebug("Skipping filtered region at %llx", region.start);
+		m_logger->LogDebug("Skipping filtered region at %llx", region.start);
 		return false;
 	}
 
@@ -161,24 +164,29 @@ bool SharedCacheController::ApplyRegion(BinaryView& view, const CacheRegion& reg
 	return true;
 }
 
-bool SharedCacheController::IsRegionLoaded(const CacheRegion& region) const
+bool SharedCacheController::IsRegionLoaded(const CacheRegion& region)
 {
+	std::shared_lock<std::shared_mutex> lock(m_loadMutex);
 	return std::any_of(m_loadedRegions.begin(), m_loadedRegions.end(), [&](const auto& loadedRegion) {
 		return loadedRegion == region.start;
 	});
 }
 
 bool SharedCacheController::ApplyImage(BinaryView& view, const CacheImage& image)
 {
-	// TODO: Check m_loadedImages? This seems redundant...
 	// Load all regions of an image and mark the image as loaded.
+	// NOTE: The regions lock m_loadMutex themselves, so we do not hold it up here.
 	bool loadedRegion = false;
 	for (const auto& regionStart : image.regionStarts)
 		if (ApplyRegionAtAddress(view, regionStart))
 			loadedRegion = true;
 
+	// The ApplyRegionAtAddress no longer holds the lock, we can take it now.
+	std::unique_lock<std::shared_mutex> lock(m_loadMutex);
 	// If there was no loaded regions than we just want to forgo loading the image.
-	if (!loadedRegion)
+	// We also skip if we already loaded the image itself. We do this after loading regions
+	// as we regions have their own check.
+	if (!loadedRegion || m_loadedImages.find(image.headerAddress) != m_loadedImages.end())
 		return false;
 
 	if (image.header)
@@ -187,17 +195,16 @@ bool SharedCacheController::ApplyImage(BinaryView& view, const CacheImage& image
 		auto machoProcessor = SharedCacheMachOProcessor(&view, m_cache.GetVirtualMemory());
 		machoProcessor.ApplyHeader(*image.header);
 
-		// TODO: Make this optional.
-		// Load objective-c information.
-		auto objcProcessor = DSCObjC::SharedCacheObjCProcessor(&view, false);
 		// TODO: Passing in an image name here is weird considering this is shared with the MACHO view.
 		// TODO: We should abstract out the "image" into an objc image type that represents what is required, which ig is the name?
+		// Load objective-c information.
+		auto objcProcessor = DSCObjC::SharedCacheObjCProcessor(&view, false);
 		try
 		{
 			if (m_processObjC)
 				objcProcessor.ProcessObjCData(image.GetName());
-			if (m_processObjC)
-			objcProcessor.ProcessCFStrings(image.GetName());
+			if (m_processCFStrings)
+				objcProcessor.ProcessCFStrings(image.GetName());
 		}
 		catch (std::exception& e)
 		{
@@ -216,8 +223,9 @@ bool SharedCacheController::ApplyImage(BinaryView& view, const CacheImage& image
 	return true;
 }
 
-bool SharedCacheController::IsImageLoaded(const CacheImage& image) const
+bool SharedCacheController::IsImageLoaded(const CacheImage& image)
 {
+	std::shared_lock<std::shared_mutex> lock(m_loadMutex);
 	return std::any_of(m_loadedImages.begin(), m_loadedImages.end(), [&](const auto& loadedImage) {
 		return loadedImage == image.headerAddress;
 	});

view/sharedcache/core/SharedCacheController.h

@@ -18,8 +18,11 @@ namespace BinaryNinja::DSC {
 	{
 		IMPLEMENT_DSC_API_OBJECT(BNSharedCacheController);
 		Ref<Logger> m_logger;
-
 		SharedCache m_cache;
+
+		// Locks on load attempts (region or image).
+		std::shared_mutex m_loadMutex;
+
 		// Store the open images.
 		// Things other than the cache here will be serialized.
 		std::unordered_set<uint64_t> m_loadedRegions;
@@ -49,13 +52,13 @@ namespace BinaryNinja::DSC {
 
 		bool ApplyRegion(BinaryView& view, const CacheRegion& region);
 
-		bool IsRegionLoaded(const CacheRegion& region) const;
+		bool IsRegionLoaded(const CacheRegion& region);
 
 		// Loads the relevant image info into the view. This does not update analysis so if you
 		// call this make sure at some point you update analysis and likely with linear sweep.
 		bool ApplyImage(BinaryView& view, const CacheImage& image);
 
-		bool IsImageLoaded(const CacheImage& image) const;
+		bool IsImageLoaded(const CacheImage& image);
 
 		// Get the metadata for saving the state of the shared cache.
 		Ref<Metadata> GetMetadata() const;

view/sharedcache/core/SharedCacheView.cpp

@@ -82,7 +82,7 @@ Ref<Settings> SharedCacheViewType::GetLoadSettingsForData(BinaryView* data)
 	R"({
 		"title" : "Region Regex Filter",
 		"type" : "string",
-		"default" : "LINKEDIT",
+		"default" : ".*LINKEDIT.*",
 		"description" : "Regex filter for region names to skip loading, by default this filters out the link edit region which is not necessary to be applied to the view."
 		})");
 
@@ -784,19 +784,22 @@ bool SharedCacheView::Init()
 		// After this we should have all the mappings available as well.
 		CacheProcessor cacheProcessor = CacheProcessor(this);
 		auto startTime = std::chrono::high_resolution_clock::now();
-		if (!cacheProcessor.ProcessCache(sharedCache))
+		bool result = cacheProcessor.ProcessCache(sharedCache);
+		auto endTime = std::chrono::high_resolution_clock::now();
+		std::chrono::duration<double> elapsed = endTime - startTime;
+		logger->LogInfo("Processing %zu entries took %.3f seconds", sharedCache.GetEntries().size(), elapsed.count());
+
+		if (!result)
 		{
-			// Oh no, we failed to process the cache, this likely means the primary on-disk file was not able to be
-			// found.
 			// TODO: Prompt the user to select the primary cache file? We can still recover from here.
+			// Oh no, we failed to process the cache, this likely means the primary on-disk file was not able to be found.
 			logger->LogError("Failed to process cache, likely missing cache files.");
 			// NOTE: An interaction handler headlessly could select yes to this, however for the purposes of this we will just let them continue by defualt.
 			if (IsUIEnabled() && ShowMessageBox("Continue opening", "This shared cache file was unable to be processed, would you like to open without shared cache information?", YesNoButtonSet, QuestionIcon) != YesButton)
 				return false;
+			// Skip all the other shared cache stuff.
+			return true;
 		}
-		auto endTime = std::chrono::high_resolution_clock::now();
-		std::chrono::duration<double> elapsed = endTime - startTime;
-		logger->LogInfo("Processing %zu entries took %.3f seconds", sharedCache.GetEntries().size(), elapsed.count());
 	}
 
 	{

view/sharedcache/core/Utility.cpp

@@ -74,7 +74,7 @@ void ApplySymbol(Ref<BinaryView> view, Ref<TypeLibrary> typeLib, Ref<Symbol> sym
 	if (symbol->GetType() == FunctionSymbol)
 	{
 		Ref<Platform> targetPlatform = view->GetDefaultPlatform();
-		func = view->AddFunctionForAnalysis(targetPlatform, symbolAddress);
+		func = view->AddFunctionForAnalysis(targetPlatform, symbolAddress, true);
 	}
 
 	if (typeLib)