[SharedCache] Replace write log with callback registration for reapplying slide info

This improves performance by ~10x with the only real issue being a copy of the cache entry existing for each weak ref.

Also fixes
- Some old TODO's that are no longer relevant
- Some function signatures not being const
- FileAccessorCache not evicting enough accessors to fit under an adjusted cache size
- Added a warning if we are over the global cache size in view initialization
- Logger name not having a `.` in `SlideInfoProcessor::SlideInfoProcessor`
- Re-added the accessor dirty check before applying slide info to fix #6570

Author: emesare

view/sharedcache/core/FileAccessorCache.cpp

@@ -32,7 +32,6 @@ FileAccessorCache& FileAccessorCache::Global()
 	return cache;
 }
 
-
 WeakFileAccessor FileAccessorCache::Open(const std::string& filePath)
 {
 	const auto id = GetCacheAccessorID(filePath);
@@ -51,7 +50,7 @@ WeakFileAccessor FileAccessorCache::Open(const std::string& filePath)
 	}
 
 	// Evict if we are going to go above the limit.
-	if (m_cache.size() >= m_cacheSize)
+	while (m_cache.size() >= m_cacheSize)
 		EvictLastUsed();
 
 	// Create a new file accessor and add it to the cache.
@@ -69,20 +68,6 @@ WeakFileAccessor FileAccessorCache::Open(const std::string& filePath)
 	return WeakFileAccessor(sharedAccessor, filePath);
 }
 
-void FileAccessorWriteLog::AddPointer(const size_t address, const size_t pointer)
-{
-	// TODO: A sharded map here would be better. see: rust dashmap
-	std::unique_lock<std::shared_mutex> lock(m_persistedMutex);
-	m_persistedPointers.emplace_back(address, pointer);
-}
-
-void FileAccessorWriteLog::ApplyWrites(MappedFileAccessor& accessor)
-{
-	std::shared_lock<std::shared_mutex> lock(m_persistedMutex);
-	for (const auto& [address, pointer] : m_persistedPointers)
-		accessor.WritePointer(address, pointer);
-}
-
 std::shared_ptr<MappedFileAccessor> WeakFileAccessor::lock()
 {
 	auto sharedPtr = m_weakPtr.lock();
@@ -93,20 +78,12 @@ std::shared_ptr<MappedFileAccessor> WeakFileAccessor::lock()
 		m_weakPtr = FileAccessorCache::Global().Open(m_filePath).m_weakPtr;
 		sharedPtr = m_weakPtr.lock();
 
-		// Apply any previously written pointers back.
-		if (sharedPtr)
-			m_writeLog->ApplyWrites(*sharedPtr);
+		// Call the function registered with `RegisterReviveCallback`.
+		// TODO: This races if two functions cannot acquire and revive the same file at the same time.
+		// TODO: This will be called twice.
+		if (m_reviveCallback.has_value())
+			(*m_reviveCallback)(*sharedPtr);
 	}
 
 	return sharedPtr;
-}
-
-void WeakFileAccessor::WritePointer(const size_t address, const size_t pointer)
-{
-	// Persist the pointer after the file accessor is revived.
-	m_writeLog->AddPointer(address, pointer);
-
-	// And then actually apply the written pointer...
-	if (auto sharedPtr = m_weakPtr.lock())
-		sharedPtr->WritePointer(address, pointer);
-}
+}

view/sharedcache/core/FileAccessorCache.h

@@ -37,48 +37,36 @@ class FileAccessorCache
 	// Adjust the cache size limit.
 	// This will NOT evict current cache entries, as they are already available.
 	// Any subsequent call to `Open` will assume this cache size, evicting until the size is equal to the cache size.
-	void SetCacheSize(uint64_t size) { m_cacheSize = size; };
-};
-
-// Write log to be used in conjunction with `WeakFileAccessor` to re-apply written data to a "revived" file.
-struct FileAccessorWriteLog
-{
-	// To persist writes to a file accessor being revived (within the lock() function)
-	// we keep a list of writes that will be re-applied in the lock function.
-	std::shared_mutex m_persistedMutex;
-	std::vector<std::pair<size_t, uint64_t>> m_persistedPointers;
-
-	FileAccessorWriteLog() = default;
+	void SetCacheSize(const uint64_t size) { m_cacheSize = size; };
 
-	// Add the pointer to the persisted pointers.
-	void AddPointer(size_t address, size_t pointer);
-
-	// Apply all logged writes to the given accessor.
-	void ApplyWrites(MappedFileAccessor& accessor);
+	size_t GetCacheSize() const { return m_cacheSize; }
 };
 
 class WeakFileAccessor
 {
+	using ReviveCallback = std::function<void(MappedFileAccessor&)>;
+
 	// Weak pointer to the mapped file accessor, once this is expired we will re-open.
 	std::weak_ptr<MappedFileAccessor> m_weakPtr;
 	// File path for re-opening if needed
 	std::string m_filePath;
 
 	// Used to re-add writes once the file accessor is "revived".
-	std::shared_ptr<FileAccessorWriteLog> m_writeLog;
+	std::optional<ReviveCallback> m_reviveCallback;
 
 	// TODO: Store a weak_ptr/shared_ptr to FileAccessorCache? That way we dont access Global()
 	// TODO: Only need to do the above if we want multiple caches.
 
 public:
 	explicit WeakFileAccessor(std::weak_ptr<MappedFileAccessor> weakPtr, std::string filePath) :
-		m_weakPtr(std::move(weakPtr)), m_filePath(std::move(filePath)),
-		m_writeLog(std::make_shared<FileAccessorWriteLog>())
+		m_weakPtr(std::move(weakPtr)), m_filePath(std::move(filePath))
 	{}
 
-	std::shared_ptr<MappedFileAccessor> lock();
+	// Register the function to be called once the file accessor is revived, this is typically
+	// used to re-apply writes such as from slide info.
+	void RegisterReviveCallback(const ReviveCallback& callback) {
+		m_reviveCallback = callback;
+	}
 
-	// Persists the written pointer within this weak file accessor.
-	// This works as we expect the weak file accessor to be stored per virtual memory region.
-	void WritePointer(size_t address, size_t pointer);
+	std::shared_ptr<MappedFileAccessor> lock();
 };

view/sharedcache/core/MappedFileAccessor.cpp

@@ -1,6 +1,5 @@
 #include "MappedFileAccessor.h"
 
-
 std::shared_ptr<MappedFileAccessor> MappedFileAccessor::Open(const std::string& filePath)
 {
 	auto file = MappedFile::OpenFile(filePath);
@@ -40,48 +39,48 @@ std::string MappedFileAccessor::ReadNullTermString(size_t address, const size_t
 	return str;
 }
 
-uint8_t MappedFileAccessor::ReadUInt8(size_t address)
+uint8_t MappedFileAccessor::ReadUInt8(size_t address) const
 {
 	return Read<uint8_t>(address);
 }
 
-int8_t MappedFileAccessor::ReadInt8(size_t address)
+int8_t MappedFileAccessor::ReadInt8(size_t address) const
 {
 	return Read<int8_t>(address);
 }
 
-uint16_t MappedFileAccessor::ReadUInt16(size_t address)
+uint16_t MappedFileAccessor::ReadUInt16(size_t address) const
 {
 	return Read<uint16_t>(address);
 }
 
-int16_t MappedFileAccessor::ReadInt16(size_t address)
+int16_t MappedFileAccessor::ReadInt16(size_t address) const
 {
 	return Read<int16_t>(address);
 }
 
 
-uint32_t MappedFileAccessor::ReadUInt32(size_t address)
+uint32_t MappedFileAccessor::ReadUInt32(size_t address) const
 {
 	return Read<uint32_t>(address);
 }
 
-int32_t MappedFileAccessor::ReadInt32(size_t address)
+int32_t MappedFileAccessor::ReadInt32(size_t address) const
 {
 	return Read<int32_t>(address);
 }
 
-uint64_t MappedFileAccessor::ReadUInt64(size_t address)
+uint64_t MappedFileAccessor::ReadUInt64(size_t address) const
 {
 	return Read<uint64_t>(address);
 }
 
-int64_t MappedFileAccessor::ReadInt64(size_t address)
+int64_t MappedFileAccessor::ReadInt64(size_t address) const
 {
 	return Read<int64_t>(address);
 }
 
-BinaryNinja::DataBuffer MappedFileAccessor::ReadBuffer(size_t addr, size_t length)
+BinaryNinja::DataBuffer MappedFileAccessor::ReadBuffer(size_t addr, size_t length) const
 {
 	if (addr + length > Length())
 		throw UnmappedAccessException(addr + length, Length());
@@ -104,7 +103,7 @@ void MappedFileAccessor::Read(void* dest, size_t addr, size_t length) const
 }
 
 template <typename T>
-T MappedFileAccessor::Read(size_t address)
+T MappedFileAccessor::Read(size_t address) const
 {
 	T result;
 	Read(&result, address, sizeof(T));

view/sharedcache/core/MappedFileAccessor.h

@@ -70,23 +70,23 @@ class MappedFileAccessor : public std::enable_shared_from_this<MappedFileAccesso
 
 	std::string ReadNullTermString(size_t address, size_t maxLength = -1) const;
 
-	uint8_t ReadUInt8(size_t address);
+	uint8_t ReadUInt8(size_t address) const;
 
-	int8_t ReadInt8(size_t address);
+	int8_t ReadInt8(size_t address) const;
 
-	uint16_t ReadUInt16(size_t address);
+	uint16_t ReadUInt16(size_t address) const;
 
-	int16_t ReadInt16(size_t address);
+	int16_t ReadInt16(size_t address) const;
 
-	uint32_t ReadUInt32(size_t address);
+	uint32_t ReadUInt32(size_t address) const;
 
-	int32_t ReadInt32(size_t address);
+	int32_t ReadInt32(size_t address) const;
 
-	uint64_t ReadUInt64(size_t address);
+	uint64_t ReadUInt64(size_t address) const;
 
-	int64_t ReadInt64(size_t address);
+	int64_t ReadInt64(size_t address) const;
 
-	BinaryNinja::DataBuffer ReadBuffer(size_t addr, size_t length);
+	BinaryNinja::DataBuffer ReadBuffer(size_t addr, size_t length) const;
 
 	// Returns a range of pointers within the mapped memory region corresponding to
 	// {addr, length}.
@@ -99,5 +99,5 @@ class MappedFileAccessor : public std::enable_shared_from_this<MappedFileAccesso
 	void Read(void* dest, size_t addr, size_t length) const;
 
 	template <typename T>
-	T Read(size_t address);
+	T Read(size_t address) const;
 };

view/sharedcache/core/SharedCache.cpp

@@ -397,10 +397,42 @@ void SharedCache::ProcessEntryRegions(const CacheEntry& entry)
 	}
 }
 
-void SharedCache::ProcessEntrySlideInfo(const CacheEntry& entry)
+void SharedCache::ProcessEntrySlideInfo(const CacheEntry& entry) const
 {
 	auto slideInfoProcessor = SlideInfoProcessor(GetBaseAddress());
-	slideInfoProcessor.ProcessEntry(*m_vm, entry);
+
+	// This will be set for every associated `VirtualMemoryRegion` so that any accesses though the VM will be always be slid.
+	// NOTE: This MUST be called on the CacheEntry object owned by SharedCache, otherwise persistence through the `SharedCacheController` will not occur.
+	// NOTE: This will keep a copy of a processor in the `WeakFileAccessor` until that object is destroyed (likely view destruction).
+	// NOTE: This will keep a copy of the cache entry in the `WeakFileAccessor` until that object is destroyed (likely view destruction).
+	auto reviveCallback = [slideInfoProcessor, entry](MappedFileAccessor& revivedAccessor) {
+		slideInfoProcessor.ProcessEntry(revivedAccessor, entry);
+	};
+
+	// Use the current entry accessor, don't register the callback for this one as we want calls through the VM to be slid only.
+	// Actually process the slide info for this entry, everything else besides this is to support revived file accessors.
+	auto slideMappings = slideInfoProcessor.ProcessEntry(*entry.GetAccessor().lock(), entry);
+
+	// Register the revive callback for all virtual memory regions that have been slid.
+	// The reason we don't just set this on the entry accessor is that accessor is not consulted for anything really after
+	// this point, everything else will be going through the virtual memory, and because the callback is on the weak accessor
+	// reference and not the file accessor cache itself this matters.
+	auto vm = GetVirtualMemory();
+	for (const auto& mapping : slideMappings)
+	{
+		// Because the mapping address is a file offset for us to consult the virtual memory we must first call `GetMappedAddress`.
+		if (auto mappedMappingAddr = entry.GetMappedAddress(mapping.address))
+		{
+			if (auto vmRegion = vm->GetRegionAtAddress(*mappedMappingAddr))
+			{
+				// Ok we have the virtual memory region, lets register the callback on its accessor.
+				vmRegion->fileAccessor.RegisterReviveCallback(reviveCallback);
+				continue;
+			}
+		}
+
+		LogWarn("Failed to register revive callback for slide mapping %llx in entry '%s'", mapping.address, entry.GetFileName().c_str());
+	}
 }
 
 void SharedCache::ProcessSymbols()

view/sharedcache/core/SharedCache.h

@@ -134,9 +134,8 @@ class CacheEntry
 	// Mappings tell us _where_ to map the regions within the flat address space.
 	// Without this we wouldn't know where the entry is supposed to exist in the address space.
 	std::vector<dyld_cache_mapping_info> m_mappings {};
-	// TODO: We really should remove this methinks.
-	// TODO: Storing this here is basically useless? IDK
 	// Mapping of image path to image info, used within ProcessImagesAndRegions to add them to the cache.
+	// Also used to retrieve the image dependencies.
 	std::unordered_map<std::string, dyld_cache_image_info> m_images {};
 
 public:
@@ -221,7 +220,7 @@ class SharedCache
 	SharedCache &operator=(SharedCache &&) noexcept = default;
 
 	uint64_t GetBaseAddress() const { return m_baseAddress; }
-	std::shared_ptr<VirtualMemory> GetVirtualMemory() { return m_vm; }
+	std::shared_ptr<VirtualMemory> GetVirtualMemory() const { return m_vm; }
 	const std::unordered_map<CacheEntryId, CacheEntry>& GetEntries() const { return m_entries; }
 	const AddressRangeMap<CacheRegion>& GetRegions() const { return m_regions; }
 	const std::unordered_map<uint64_t, CacheImage>& GetImages() const { return m_images; }
@@ -245,7 +244,7 @@ class SharedCache
 
 	void ProcessEntryRegions(const CacheEntry& entry);
 
-	void ProcessEntrySlideInfo(const CacheEntry& entry);
+	void ProcessEntrySlideInfo(const CacheEntry& entry) const;
 
 	// Construct the named symbols lookup map for use with `GetSymbolWithName`.
 	void ProcessSymbols();

view/sharedcache/core/SharedCacheView.cpp

@@ -787,7 +787,8 @@ bool SharedCacheView::Init()
 		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());
+		auto entryCount = sharedCache.GetEntries().size();
+		logger->LogInfo("Processing %zu entries took %.3f seconds", entryCount, elapsed.count());
 
 		if (!result)
 		{
@@ -800,6 +801,10 @@ bool SharedCacheView::Init()
 			// Skip all the other shared cache stuff.
 			return true;
 		}
+
+		// If we can't store all of our files for this cache in the accessor cache we might run into issues, warn the user.
+		if (entryCount > FileAccessorCache::Global().GetCacheSize())
+			logger->LogWarn("Cache contains more entries than the allowed number of opened file handles, this may impact reliability.");
 	}
 
 	{