[Debug] Fix monitor-bridge use-after-free in GdbServer#4
Open
r-log wants to merge 1 commit into
Open
Conversation
SubmitMonitorLine queued a MonitorReq holding a raw GdbMonSocket* with no ownership taken. If a monitor client sent a line and disconnected before the world thread's DrainMonitorRequests popped the request (e.g. any time the world thread is parked in EnterStop, which pumps only RSP traffic), the ACE reactor thread's handle_close/remove_reference could free the socket first. DrainMonitorRequests would then call req.writer() through the dangling pointer -> ACCESS_VIOLATION. Fix: pin the socket's ACE reference count for the lifetime of a queued request. SubmitMonitorLine now takes two extra thunks (addRef/release, mirroring the existing writer thunk) so the game-layer GdbServer stays free of any ACE dependency; GdbMonSocket wires them to add_reference()/ remove_reference(). MonitorReq became move-only and RAII-releases its pin in its destructor, so the reference is dropped exactly once on every path: normal servicing, an unrecognized command, or the queue itself being torn down with requests still pending. The RSP side (m_rspWriter/m_rspCtx, guarded by m_writerLock + m_peerClosed, cleared in DetachRsp) was already safe and is untouched.
This was referenced Jul 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Second follow-up to mangoszero#406 (independent of the thread-safety PR). Merge into your
masterand it lands in mangoszero#406.Fixes a use-after-free in the monitor bridge.
SubmitMonitorLinequeues a rawGdbMonSocket*asMonitorReq.ctxwith no ownership taken. Monitor requests are drained only on the world thread (DrainMonitorRequestsinOnWorldUpdate), and while the world thread is parked inEnterStopthey aren't drained at all — so if the client disconnects before the request is drained, the reactor thread'shandle_close→remove_reference()frees the socket and the laterreq.writer(req.ctx, …)writes through a dangling pointer → ACCESS_VIOLATION (RtlEnterCriticalSectionon the freedoutBufferLock). A long-blockingworldtickstop just widens a window that exists for any mid-flight monitor disconnect.Fix: pin the socket's ACE reference count for the queued request's lifetime.
SubmitMonitorLinetakesadd_reference()before enqueue;MonitorReqis now move-only RAII that callsremove_reference()exactly once — on service, drop, or queue teardown at shutdown. ACE stays out of the game layer: the ref ops are passed asvoid*thunks, same pattern as the existingwriter. 3 files, +90/-4.Verified on the same live 1.12.1 server: the deterministic crash-repro (send+close a monitor probe during a
worldtickstop) that previously produced a signature-identical dump now survives with no new dump, and the thread-safety tests (off-thread no-stall, on-thread stop+resume) still pass. Compiles clean, MSVC 2022 Release, 0 warnings / 0 errors.