[SharedCache] Split view-specific state into a separate struct

The existing view-specific state was stored in several global unordered
maps. Many of these were accessed without locking, including
`viewSpecificMutexes`, which is racy in the face of multiple threads.

View-specific state is stored in a new heap-allocated
`ViewSpecificState` struct that is reference counted via
`std::shared_ptr`. A static map holds a `std::weak_ptr` to each
view-specific state, keyed by session id. `SharedCache` retrieves its
view-specific state during its constructor.

Since `ViewSpecificState` is reference counted it will naturally be
deallocated when the last `SharedCache` instance that references it goes
away. Its corresponding entry will remain in the static map, though
since it only holds a `std::weak_ptr` rather than any state it will not
use much memory. The next time view-specific state is retrieved any
expired entries will be removed from the map.

Author: bdash

view/sharedcache/core/SharedCache.cpp

@@ -27,6 +27,8 @@
 #include "SharedCache.h"
 #include "ObjC.h"
 #include <filesystem>
+#include <mutex>
+#include <unordered_map>
 #include <utility>
 #include <fcntl.h>
 #include <memory>
@@ -78,19 +80,51 @@ struct SharedCache::State
 	DSCViewState viewState = DSCViewStateUnloaded;
 };
 
-static std::recursive_mutex viewStateMutex;
-static std::unordered_map<uint64_t, std::shared_ptr<struct SharedCache::State>> viewStateCache;
+struct SharedCache::ViewSpecificState {
+	std::mutex typeLibraryMutex;
+	std::mutex viewOperationsThatInfluenceMetadataMutex;
 
-std::mutex progressMutex;
-std::unordered_map<uint64_t, BNDSCViewLoadProgress> progressMap;
+	std::atomic<BNDSCViewLoadProgress> progress;
 
-struct ViewSpecificMutexes {
-	std::mutex viewOperationsThatInfluenceMetadataMutex;
-	std::mutex typeLibraryLookupAndApplicationMutex;
+	std::mutex stateMutex;
+	std::shared_ptr<struct SharedCache::State> cachedState;
 };
 
-static std::unordered_map<uint64_t, ViewSpecificMutexes> viewSpecificMutexes;
 
+std::shared_ptr<SharedCache::ViewSpecificState> ViewSpecificStateForId(uint64_t viewIdentifier, bool insertIfNeeded = true) {
+	static std::mutex viewSpecificStateMutex;
+	static std::unordered_map<uint64_t, std::weak_ptr<SharedCache::ViewSpecificState>> viewSpecificState;
+
+	std::lock_guard lock(viewSpecificStateMutex);
+
+	if (auto it = viewSpecificState.find(viewIdentifier); it != viewSpecificState.end()) {
+		if (auto statePtr = it->second.lock()) {
+			return statePtr;
+		}
+	}
+
+	if (!insertIfNeeded) {
+		return nullptr;
+	}
+
+	auto statePtr = std::make_shared<SharedCache::ViewSpecificState>();
+	viewSpecificState[viewIdentifier] = statePtr;
+
+	// Prune entries for any views that are no longer in use.
+	for (auto it = viewSpecificState.begin(); it != viewSpecificState.end(); ) {
+		if (it->second.expired()) {
+			it = viewSpecificState.erase(it);
+		} else {
+			++it;
+		}
+	}
+
+	return statePtr;
+}
+
+std::shared_ptr<SharedCache::ViewSpecificState> ViewSpecificStateForView(Ref<BinaryNinja::BinaryView> view) {
+	return ViewSpecificStateForId(view->GetFile()->GetSessionId());
+}
 
 std::string base_name(std::string const& path)
 {
@@ -220,9 +254,7 @@ void SharedCache::PerformInitialLoad()
 	auto path = m_dscView->GetFile()->GetOriginalFilename();
 	auto baseFile = MMappedFileAccessor::Open(m_dscView, m_dscView->GetFile()->GetSessionId(), path)->lock();
 
-    progressMutex.lock();
-	progressMap[m_dscView->GetFile()->GetSessionId()] = LoadProgressLoadingCaches;
-	progressMutex.unlock();
+	m_viewSpecificState->progress = LoadProgressLoadingCaches;
 
 	WillMutateState();
 
@@ -737,9 +769,8 @@ void SharedCache::PerformInitialLoad()
 	}
 	}
 	baseFile.reset();
-	progressMutex.lock();
-	progressMap[m_dscView->GetFile()->GetSessionId()] = LoadProgressLoadingImages;
-	progressMutex.unlock();
+
+	m_viewSpecificState->progress = LoadProgressLoadingImages;
 
 	// We have set up enough metadata to map VM now.
 
@@ -952,9 +983,7 @@ void SharedCache::PerformInitialLoad()
 
 	m_logger->LogDebug("Finished initial load of Shared Cache");
 
-	progressMutex.lock();
-	progressMap[m_dscView->GetFile()->GetSessionId()] = LoadProgressFinished;
-	progressMutex.unlock();
+	m_viewSpecificState->progress = LoadProgressFinished;
 }
 
 std::shared_ptr<VM> SharedCache::GetVMMap(bool mapPages)
@@ -983,10 +1012,10 @@ void SharedCache::DeserializeFromRawView()
 {
 	if (m_dscView->QueryMetadata(SharedCacheMetadataTag))
 	{
-		std::unique_lock<std::recursive_mutex> viewStateCacheLock(viewStateMutex);
-		if (auto it = viewStateCache.find(m_dscView->GetFile()->GetSessionId()); it != viewStateCache.end())
+		std::lock_guard lock(m_viewSpecificState->stateMutex);
+		if (m_viewSpecificState->cachedState)
 		{
-			m_state = it->second;
+			m_state = m_viewSpecificState->cachedState;
 			m_stateIsShared = true;
 			m_metadataValid = true;
 		}
@@ -1367,7 +1396,7 @@ void SharedCache::ParseAndApplySlideInfoForFile(std::shared_ptr<MMappedFileAcces
 }
 
 
-SharedCache::SharedCache(BinaryNinja::Ref<BinaryNinja::BinaryView> dscView) : m_dscView(dscView)
+SharedCache::SharedCache(BinaryNinja::Ref<BinaryNinja::BinaryView> dscView) : m_dscView(dscView), m_viewSpecificState(ViewSpecificStateForView(dscView))
 {
 	if (dscView->GetTypeName() != VIEW_NAME)
 	{
@@ -1381,46 +1410,43 @@ SharedCache::SharedCache(BinaryNinja::Ref<BinaryNinja::BinaryView> dscView) : m_
 	DeserializeFromRawView();
 	if (!m_metadataValid)
 		return;
-	if (State().viewState == DSCViewStateUnloaded)
+
+	if (State().viewState != DSCViewStateUnloaded) {
+		m_viewSpecificState->progress = LoadProgressFinished;
+		return;
+	}
+
+	std::unique_lock lock(m_viewSpecificState->viewOperationsThatInfluenceMetadataMutex);
+	try {
+		PerformInitialLoad();
+	}
+	catch (...)
 	{
-		std::unique_lock<std::mutex> lock(viewSpecificMutexes[m_dscView->GetFile()->GetSessionId()].viewOperationsThatInfluenceMetadataMutex);
-		try {
-			PerformInitialLoad();
-		}
-		catch (...)
-		{
-			m_logger->LogError("Failed to perform initial load of Shared Cache");
-		}
+		m_logger->LogError("Failed to perform initial load of Shared Cache");
+	}
 
-		auto settings = m_dscView->GetLoadSettings(VIEW_NAME);
-		bool autoLoadLibsystem = true;
-		if (settings && settings->Contains("loader.dsc.autoLoadLibSystem"))
-		{
-			autoLoadLibsystem = settings->Get<bool>("loader.dsc.autoLoadLibSystem", m_dscView);
-		}
-		if (autoLoadLibsystem)
+	auto settings = m_dscView->GetLoadSettings(VIEW_NAME);
+	bool autoLoadLibsystem = true;
+	if (settings && settings->Contains("loader.dsc.autoLoadLibSystem"))
+	{
+		autoLoadLibsystem = settings->Get<bool>("loader.dsc.autoLoadLibSystem", m_dscView);
+	}
+	if (autoLoadLibsystem)
+	{
+		for (const auto& [_, header] : State().headers)
 		{
-			for (const auto& [_, header] : State().headers)
+			if (header.installName.find("libsystem_c.dylib") != std::string::npos)
 			{
-				if (header.installName.find("libsystem_c.dylib") != std::string::npos)
-				{
-					lock.unlock();
-					m_logger->LogInfo("Loading core libsystem_c.dylib library");
-					LoadImageWithInstallName(header.installName, false);
-					lock.lock();
-					break;
-				}
+				lock.unlock();
+				m_logger->LogInfo("Loading core libsystem_c.dylib library");
+				LoadImageWithInstallName(header.installName, false);
+				break;
 			}
 		}
-		MutableState().viewState = DSCViewStateLoaded;
-		SaveToDSCView();
-	}
-	else
-	{
-		progressMutex.lock();
-		progressMap[m_dscView->GetFile()->GetSessionId()] = LoadProgressFinished;
-		progressMutex.unlock();
 	}
+
+	MutableState().viewState = DSCViewStateLoaded;
+	SaveToDSCView();
 }
 
 SharedCache::~SharedCache() {
@@ -1536,7 +1562,7 @@ bool SharedCache::LoadImageContainingAddress(uint64_t address, bool skipObjC)
 
 bool SharedCache::LoadSectionAtAddress(uint64_t address)
 {
-	std::unique_lock<std::mutex> lock(viewSpecificMutexes[m_dscView->GetFile()->GetSessionId()].viewOperationsThatInfluenceMetadataMutex);
+	std::unique_lock lock(m_viewSpecificState->viewOperationsThatInfluenceMetadataMutex);
 	DeserializeFromRawView();
 	WillMutateState();
 
@@ -1806,7 +1832,7 @@ bool SharedCache::LoadImageWithInstallName(std::string installName, bool skipObj
 {
 	auto settings = m_dscView->GetLoadSettings(VIEW_NAME);
 
-	std::unique_lock<std::mutex> lock(viewSpecificMutexes[m_dscView->GetFile()->GetSessionId()].viewOperationsThatInfluenceMetadataMutex);
+	std::unique_lock lock(m_viewSpecificState->viewOperationsThatInfluenceMetadataMutex);
 
 	DeserializeFromRawView();
 	WillMutateState();
@@ -1880,7 +1906,7 @@ bool SharedCache::LoadImageWithInstallName(std::string installName, bool skipObj
 		return false;
 	}
 
-	std::unique_lock<std::mutex> typelibLock(viewSpecificMutexes[m_dscView->GetFile()->GetSessionId()].typeLibraryLookupAndApplicationMutex);
+	std::unique_lock typelibLock(m_viewSpecificState->typeLibraryMutex);
 	auto typeLib = m_dscView->GetTypeLibrary(header.installName);
 
 	if (!typeLib)
@@ -2875,7 +2901,7 @@ std::vector<std::pair<std::string, Ref<Symbol>>> SharedCache::LoadAllSymbolsAndW
 {
 	WillMutateState();
 
-	std::unique_lock<std::mutex> initialLoadBlock(viewSpecificMutexes[m_dscView->GetFile()->GetSessionId()].viewOperationsThatInfluenceMetadataMutex);
+	std::lock_guard initialLoadBlock(m_viewSpecificState->viewOperationsThatInfluenceMetadataMutex);
 
 	std::vector<std::pair<std::string, Ref<Symbol>>> symbols;
 	for (const auto& img : State().images)
@@ -2973,7 +2999,7 @@ void SharedCache::FindSymbolAtAddrAndApplyToAddr(
 		}
 		auto exportList = SharedCache::ParseExportTrie(mapping, *header);
 		std::vector<std::pair<uint64_t, std::pair<BNSymbolType, std::string>>> exportMapping;
-		std::unique_lock<std::mutex> lock(viewSpecificMutexes[m_dscView->GetFile()->GetSessionId()].typeLibraryLookupAndApplicationMutex);
+		std::unique_lock lock(m_viewSpecificState->typeLibraryMutex);
 		auto typeLib = m_dscView->GetTypeLibrary(header->installName);
 		if (!typeLib)
 		{
@@ -3020,7 +3046,7 @@ void SharedCache::FindSymbolAtAddrAndApplyToAddr(
 			}
 		}
 		{
-			std::unique_lock<std::mutex> _lock(viewSpecificMutexes[m_dscView->GetFile()->GetSessionId()].viewOperationsThatInfluenceMetadataMutex);
+			std::lock_guard lock(m_viewSpecificState->viewOperationsThatInfluenceMetadataMutex);
 			MutableState().exportInfos[header->textBase] = std::move(exportMapping);
 		}
 		m_dscView->EndBulkModifySymbols();
@@ -3044,8 +3070,8 @@ bool SharedCache::SaveToDSCView()
 		m_state = cachedState;
 		m_stateIsShared = true;
 
-		std::unique_lock<std::recursive_mutex> viewStateCacheLock(viewStateMutex);
-		viewStateCache[m_dscView->GetFile()->GetSessionId()] = std::move(cachedState);
+		std::lock_guard lock(m_viewSpecificState->stateMutex);
+		m_viewSpecificState->cachedState = std::move(cachedState);
 
 		m_metadataValid = true;
 
@@ -3055,7 +3081,7 @@ bool SharedCache::SaveToDSCView()
 }
 std::vector<MemoryRegion> SharedCache::GetMappedRegions() const
 {
-	std::unique_lock<std::mutex> lock(viewSpecificMutexes[m_dscView->GetFile()->GetSessionId()].viewOperationsThatInfluenceMetadataMutex);
+	std::lock_guard lock(m_viewSpecificState->viewOperationsThatInfluenceMetadataMutex);
 	return State().regionsMappedIntoMemory;
 }
 
@@ -3392,15 +3418,11 @@ extern "C"
 
 	BNDSCViewLoadProgress BNDSCViewGetLoadProgress(uint64_t sessionID)
 	{
-		progressMutex.lock();
-		if (progressMap.find(sessionID) == progressMap.end())
-		{
-			progressMutex.unlock();
-			return LoadProgressNotStarted;
+		if (auto viewSpecificState = ViewSpecificStateForId(sessionID, false)) {
+			return viewSpecificState->progress;
 		}
-		auto progress = progressMap[sessionID];
-		progressMutex.unlock();
-		return progress;
+
+		return LoadProgressNotStarted;
 	}
 
 	uint64_t BNDSCViewFastGetBackingCacheCount(BNBinaryView* data)

view/sharedcache/core/SharedCache.h

@@ -538,6 +538,8 @@ namespace SharedCacheCore {
 
 		struct State;
 
+		struct ViewSpecificState;
+
 	private:
 		Ref<Logger> m_logger;
 		/* VIEW STATE BEGIN -- SERIALIZE ALL OF THIS AND STORE IT IN RAW VIEW */
@@ -552,6 +554,7 @@ namespace SharedCacheCore {
 		bool m_metadataValid = false;
 
 		/* VIEWSTATE END -- NOTHING PAST THIS IS SERIALIZED */
+		std::shared_ptr<ViewSpecificState> m_viewSpecificState;
 
 		/* API VIEW START */
 		BinaryNinja::Ref<BinaryNinja::BinaryView> m_dscView;