FindWriteTagByFlag(flush=false) is only called from
FormatMemWriteTagAtNoFlush(), which is itself only called from within
FlushPendingMemInfo() - which already holds pendingReadMutex for its
entire body. The previous commit added an unconditional lock of that
same (non-recursive) mutex here, so any path that reaches a memory
write's tag formatting while a flush is in progress double-locks it
and hangs/crashes.
Repro: PPSSPPHeadless --graphics=software on
pspautotests/tests/gpu/clipping/homogeneous.prx reliably hit this.
Only take the lock when flush=true (i.e. when we're not already
guaranteed to be called from inside FlushPendingMemInfo's locked
section), using a defer_lock so the two call sites stay consistent.
Verified fixed: 4/4 clean runs of the repro above, plus the full
UnitTest suite (49/49) still passes.
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.
Turns out these were needed after all. For some reason, on Windows and
Mac, <algorithm> gets auto-included by something else so I don't notice
when it's missing, and MSVC's include dependency tracker doesn't see it
either.
Unfortunately the ub (undefined behavior) sanitizer has some bugs, it doesn't
understand pointers to member functions, so can't use it in-game (due to the
vertex decoder).
Thanks Nemoumbra for the reminder.
If a debugger (i.e. the memory view) checks for memory block info while a
save state is being loaded, it can crash. This was already rare, but this
change makes it significantly rarer.
Of course, it's still possible without a mutex, but I'm wanting to avoid
slowing down the lookups as they are used at runtime within emulation.
Henrik Rydgård