mirror of
https://github.com/hrydgard/ppsspp.git
synced 2026-10-01 14:58:14 +00:00
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.
This commit is contained in:
1 parent
4d8a5d74e7
commit
527c50d32e
1 file changed
+13
@@ -601,6 +601,11 @@ std::vector<MemBlockInfo> 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<std::mutex> guard(pendingReadMutex);
|
||||
std::vector<MemBlockInfo> results;
|
||||
allocMap.Find(MemBlockFlags::ALLOC, start, size, results);
|
||||
suballocMap.Find(MemBlockFlags::SUB_ALLOC, start, size, results);
|
||||
@@ -617,6 +622,8 @@ std::vector<MemBlockInfo> FindMemInfoByFlag(MemBlockFlags flags, uint32_t start,
|
||||
if (pendingNotifyMinAddr2 < start + size && pendingNotifyMaxAddr2 >= start)
|
||||
FlushPendingMemInfo();
|
||||
|
||||
// See the comment in FindMemInfo() above.
|
||||
std::lock_guard<std::mutex> guard(pendingReadMutex);
|
||||
std::vector<MemBlockInfo> 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<std::mutex> 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<std::mutex> guard(pendingReadMutex);
|
||||
allocMap.DoState(p);
|
||||
suballocMap.DoState(p);
|
||||
writeMap.DoState(p);
|
||||
|
||||
Reference in new issue
Block a user