[SharedCache] Improve support for older shared cache versions

Fixed branch island regions being skipped

4 files changed, 33 insertions, 17 deletions

diff --git a/view/sharedcache/core/SharedCache.cpp b/view/sharedcache/core/SharedCache.cpp
index 66c737d0..e4689086 100644
--- a/view/sharedcache/core/SharedCache.cpp
+++ b/view/sharedcache/core/SharedCache.cpp
@@ -325,15 +325,16 @@ void SharedCache::ProcessEntryRegions(const CacheEntry& entry)
 	// Collect pool addresses as non image memory regions.
 	for (size_t i = 0; i < entryHeader.branchPoolsCount; i++)
 	{
-		auto branchPoolAddr = entryHeader.branchPoolsOffset + (i * m_vm->GetAddressSize());
-		auto header = SharedCacheMachOHeader::ParseHeaderForAddress(
-			m_vm, branchPoolAddr, "dyld_shared_cache_branch_islands_" + std::to_string(i));
+		auto branchPoolIdxAddr = *entry.GetMappedAddress(entryHeader.branchPoolsOffset) + (i * m_vm->GetAddressSize());
+		auto branchPoolAddr = m_vm->ReadPointer(branchPoolIdxAddr);
+		auto branchHeader = SharedCacheMachOHeader::ParseHeaderForAddress(
+			m_vm, branchPoolAddr, fmt::format("dyld_shared_cache_branch_islands_{}", i));
 		// Stop processing branch pools if a header fails to parse.
-		if (!header.has_value())
+		if (!branchHeader.has_value())
 			break;
 
 		// Gather all non image regions from the branch islands.
-		for (const auto& segment : header->segments)
+		for (const auto& segment : branchHeader->segments)
 		{
 			CacheRegion stubIslandRegion;
 			stubIslandRegion.start = segment.vmaddr;
diff --git a/view/sharedcache/core/VirtualMemory.cpp b/view/sharedcache/core/VirtualMemory.cpp
index 60ed1481..71dafd0c 100644
--- a/view/sharedcache/core/VirtualMemory.cpp
+++ b/view/sharedcache/core/VirtualMemory.cpp
@@ -46,7 +46,7 @@ bool VirtualMemory::IsAddressMapped(uint64_t address)
 	return m_regions.find(address) != m_regions.end();
 }
 
-void VirtualMemory::WritePointer(size_t address, size_t pointer)
+void VirtualMemory::WritePointer(uint64_t address, size_t pointer)
 {
 	uint64_t offset;
 	auto region = GetRegionAtAddress(address, offset);
@@ -55,6 +55,21 @@ void VirtualMemory::WritePointer(size_t address, size_t pointer)
 	region->fileAccessor.lock()->WritePointer(offset, pointer);
 }
 
+uint64_t VirtualMemory::ReadPointer(uint64_t address)
+{
+	switch (m_addressSize)
+	{
+	case 8:
+		return ReadUInt64(address);
+	case 4:
+		return ReadUInt32(address);
+	case 2:
+		return ReadUInt16(address);
+	default:
+		throw std::runtime_error("Unsupported address size");
+	}
+}
+
 std::string VirtualMemory::ReadCString(uint64_t address)
 {
 	uint64_t offset;
@@ -145,7 +160,7 @@ BinaryNinja::DataBuffer VirtualMemory::ReadBuffer(uint64_t address, size_t lengt
 	return region->fileAccessor.lock()->ReadBuffer(offset, length);
 }
 
-std::pair<const uint8_t*, const uint8_t*> VirtualMemory::ReadSpan(size_t address, size_t length)
+std::pair<const uint8_t*, const uint8_t*> VirtualMemory::ReadSpan(uint64_t address, size_t length)
 {
 	uint64_t offset;
 	auto region = GetRegionAtAddress(address, offset);
@@ -177,7 +192,7 @@ std::string VirtualMemoryReader::ReadCString(uint64_t address, size_t maxLength)
 	if (!region.has_value())
 		throw UnmappedRegionException(address);
 	// TODO: Advance cursor?
-	return region->fileAccessor.lock()->ReadNullTermString(offset);
+	return region->fileAccessor.lock()->ReadNullTermString(offset, maxLength);
 }
 
 uint64_t VirtualMemoryReader::ReadULEB128(size_t cursorLimit)
@@ -250,12 +265,8 @@ uint64_t VirtualMemoryReader::ReadPointer()
 
 uint64_t VirtualMemoryReader::ReadPointer(uint64_t address)
 {
-	if (m_addressSize == 8)
-		return ReadUInt64(address);
-	if (m_addressSize == 4)
-		return ReadUInt32(address);
-	// TODO: Throw here or assert.
-	return 0;
+	m_cursor = m_memory->GetAddressSize();
+	return m_memory->ReadPointer(address);
 }
 
 uint8_t VirtualMemoryReader::ReadUInt8()
diff --git a/view/sharedcache/core/VirtualMemory.h b/view/sharedcache/core/VirtualMemory.h
index 647f4468..fcaf5c07 100644
--- a/view/sharedcache/core/VirtualMemory.h
+++ b/view/sharedcache/core/VirtualMemory.h
@@ -61,7 +61,9 @@ public:
 	bool IsAddressMapped(uint64_t address);
 
 	// Write a pointer at a given address. This pointer is never persisted when a file accessor is closed.
-	void WritePointer(size_t address, size_t pointer);
+	void WritePointer(uint64_t address, size_t pointer);
+
+	uint64_t ReadPointer(uint64_t address);
 
 	std::string ReadCString(uint64_t address);
 
@@ -83,7 +85,7 @@ public:
 
 	BinaryNinja::DataBuffer ReadBuffer(uint64_t address, size_t length);
 
-	std::pair<const uint8_t*, const uint8_t*> ReadSpan(size_t address, size_t length);
+	std::pair<const uint8_t*, const uint8_t*> ReadSpan(uint64_t address, size_t length);
 
 	void Read(void* dest, uint64_t address, size_t length);
 };
diff --git a/view/sharedcache/workflow/SharedCacheWorkflow.cpp b/view/sharedcache/workflow/SharedCacheWorkflow.cpp
index 7f63866a..dcccfe74 100644
--- a/view/sharedcache/workflow/SharedCacheWorkflow.cpp
+++ b/view/sharedcache/workflow/SharedCacheWorkflow.cpp
@@ -126,7 +126,9 @@ void AnalyzeStubFunction(Ref<Function> func, Ref<MediumLevelILFunction> mlil, Sh
 		if (region->type == SharedCacheRegionTypeImage)
 			return false;
 		// Adjust the new region semantics to read only, this helps analysis pickup constant loads in our stub functions.
-		region->flags = static_cast<BNSegmentFlag>(SegmentReadable | SegmentContainsData);
+		// NOTE: We do NOT do this for stub island as that contains CODE!
+		if (region->type != SharedCacheRegionTypeStubIsland)
+			region->flags = static_cast<BNSegmentFlag>(SegmentReadable | SegmentContainsData);
 		return controller.ApplyRegion(*view, *region);
 	};