From 527c50d32e9ec5f7cd1337f110b8eeca2c29f09a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Tue, 11 Aug 2026 15:01:14 +0200 Subject: [PATCH] MemBlockInfo: fix unsynchronized access to the slab maps from readers The background flush thread calls FlushPendingMemInfo() at any time while the emulator runs, holding pendingReadMutex for its whole body while calling MemSlabMap::Mark() - which does new/delete and relinks the intrusive Slab linked list via Split()/Merge(). FindMemInfo()/FindMemInfoByFlag()/FindWriteTagByFlag() only acquired that lock indirectly and conditionally, inside FlushPendingMemInfo() itself when the requested range happened to overlap pending data - the actual .Find()/.FastFindWriteTag() traversal that followed ran completely unsynchronized against the background thread's Mark() calls on the same maps. This is a genuine use-after-free: a reader could dereference a Slab* the flush thread just deleted, or race on the shared lastFind_ pointer both sides read and write. Since a Slab's tag is copied into the debugger's/WebSocket API's response, this could also leak stale/freed heap bytes back to a caller. MemBlockInfoDoState had the same gap around allocMap/suballocMap/writeMap/textureMap's .DoState() calls. Hold pendingReadMutex for the duration of these calls too, matching the comment already on FlushPendingMemInfo ("This lock prevents us from another thread reading while we're busy flushing") which wasn't actually honored by the reader side. --- Core/Debugger/MemBlockInfo.cpp | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/Core/Debugger/MemBlockInfo.cpp b/Core/Debugger/MemBlockInfo.cpp index 9a9379f473..6f2f36f745 100644 --- a/Core/Debugger/MemBlockInfo.cpp +++ b/Core/Debugger/MemBlockInfo.cpp @@ -601,6 +601,11 @@ std::vector FindMemInfo(uint32_t start, uint32_t size) { if (pendingNotifyMinAddr2 < start + size && pendingNotifyMaxAddr2 >= start) FlushPendingMemInfo(); + // pendingReadMutex doesn't just guard the pending queue - it's also what keeps + // the background flush thread's Mark() calls (which mutate the slab maps' + // linked lists via Split()/Merge()/delete) from running concurrently with the + // traversal below, which used to be completely unsynchronized against it. + std::lock_guard guard(pendingReadMutex); std::vector results; allocMap.Find(MemBlockFlags::ALLOC, start, size, results); suballocMap.Find(MemBlockFlags::SUB_ALLOC, start, size, results); @@ -617,6 +622,8 @@ std::vector FindMemInfoByFlag(MemBlockFlags flags, uint32_t start, if (pendingNotifyMinAddr2 < start + size && pendingNotifyMaxAddr2 >= start) FlushPendingMemInfo(); + // See the comment in FindMemInfo() above. + std::lock_guard guard(pendingReadMutex); std::vector results; if (flags & MemBlockFlags::ALLOC) allocMap.Find(MemBlockFlags::ALLOC, start, size, results); @@ -639,6 +646,10 @@ static const char *FindWriteTagByFlag(MemBlockFlags flags, uint32_t start, uint3 FlushPendingMemInfo(); } + // See the comment in FindMemInfo() above. Note: the returned tag pointer is + // only valid until the next Mark() call per FastFindWriteTag()'s own contract, + // so callers must treat it as transient exactly as they already do. + std::lock_guard guard(pendingReadMutex); if (flags & MemBlockFlags::ALLOC) { const char *tag = allocMap.FastFindWriteTag(MemBlockFlags::ALLOC, start, size, tagLen); if (tag) @@ -745,6 +756,8 @@ void MemBlockInfoDoState(PointerWrap &p) { return; FlushPendingMemInfo(); + // See the comment in FindMemInfo() above. + std::lock_guard guard(pendingReadMutex); allocMap.DoState(p); suballocMap.DoState(p); writeMap.DoState(p);