From b5186a4867cf6c8360a5afa0def8cbaf0bd6d078 Mon Sep 17 00:00:00 2001 From: r-log Date: Sat, 4 Jul 2026 14:58:27 +0200 Subject: [PATCH] [Debug] Fix monitor-bridge use-after-free in GdbServer 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. --- src/game/Debug/GdbServer/GdbServer.cpp | 14 +++++- src/game/Debug/GdbServer/GdbServer.h | 66 +++++++++++++++++++++++++- src/mangosd/GdbServerThread.cpp | 14 ++++++ 3 files changed, 90 insertions(+), 4 deletions(-) diff --git a/src/game/Debug/GdbServer/GdbServer.cpp b/src/game/Debug/GdbServer/GdbServer.cpp index db216a224..f01f20cc6 100644 --- a/src/game/Debug/GdbServer/GdbServer.cpp +++ b/src/game/Debug/GdbServer/GdbServer.cpp @@ -151,15 +151,22 @@ void GdbServer::FeedRsp(const uint8* data, uint32 len) m_inbound.emplace_back(data, data + len); } -void GdbServer::SubmitMonitorLine(void* ctx, MonWriter writer, const char* line) +void GdbServer::SubmitMonitorLine(void* ctx, MonWriter writer, MonRefFn addRef, + MonRefFn release, const char* line) { - if (line == nullptr || writer == nullptr) + if (line == nullptr || writer == nullptr || addRef == nullptr || + release == nullptr) { return; } + // Pin ctx alive until DrainMonitorRequests (or queue teardown) releases + // it — otherwise a socket close racing the world-thread drain would + // free ctx while this request still references it. + addRef(ctx); MonitorReq req; req.ctx = ctx; req.writer = writer; + req.release = release; req.line = line; std::lock_guard guard(m_monLock); m_monitor.push_back(std::move(req)); @@ -194,6 +201,9 @@ void GdbServer::DrainMonitorRequests() { req.writer(req.ctx, w.Data()); } + // req's destructor releases the pin taken in SubmitMonitorLine here, + // now that the write attempt (safe even if the socket already + // started closing) is done. } } diff --git a/src/game/Debug/GdbServer/GdbServer.h b/src/game/Debug/GdbServer/GdbServer.h index 35d235046..fb8e61dcb 100644 --- a/src/game/Debug/GdbServer/GdbServer.h +++ b/src/game/Debug/GdbServer/GdbServer.h @@ -63,6 +63,10 @@ class GdbServer public: typedef void (*RspWriter)(void* ctx, const uint8* data, uint32 len); typedef void (*MonWriter)(void* ctx, const char* text); + /// Pins/unpins the lifetime of a monitor connection's @p ctx (network + /// thread's reference-counted socket) so a queued request can never + /// outlive the object it targets; see SubmitMonitorLine. + typedef void (*MonRefFn)(void* ctx); static GdbServer& Instance(); @@ -83,8 +87,14 @@ class GdbServer void FeedRsp(const uint8* data, uint32 len); /// Submit one plain-text monitor line ("mangos ...") for execution on - /// the world thread; the reply is delivered via @p writer. - void SubmitMonitorLine(void* ctx, MonWriter writer, const char* line); + /// the world thread; the reply is delivered via @p writer. @p addRef + /// is called synchronously (network thread) to pin @p ctx alive for + /// as long as the request is queued/in-flight; @p release is called + /// exactly once, after the request has been serviced or dropped, to + /// undo that pin. This prevents a use-after-free when the socket + /// closes before the world thread drains the request. + void SubmitMonitorLine(void* ctx, MonWriter writer, MonRefFn addRef, + MonRefFn release, const char* line); // --- world thread side --------------------------------------------- @@ -116,10 +126,62 @@ class GdbServer void EnterStop(const char* reason); void CaptureContext(GdbRsp::RegSnapshot& out); + // Move-only: owns one pinning reference (taken by SubmitMonitorLine + // via addRef) on ctx and releases it exactly once — on normal + // servicing, on drop, or when leftover requests are torn down along + // with the queue itself (e.g. at shutdown) — so ctx can never be + // freed while a request still points at it. struct MonitorReq { + MonitorReq() = default; + MonitorReq(const MonitorReq&) = delete; + MonitorReq& operator=(const MonitorReq&) = delete; + + MonitorReq(MonitorReq&& other) noexcept + : ctx(other.ctx), writer(other.writer), + release(other.release), line(std::move(other.line)) + { + other.ctx = nullptr; + other.writer = nullptr; + other.release = nullptr; + } + + MonitorReq& operator=(MonitorReq&& other) noexcept + { + if (this != &other) + { + ReleasePin(); + ctx = other.ctx; + writer = other.writer; + release = other.release; + line = std::move(other.line); + other.ctx = nullptr; + other.writer = nullptr; + other.release = nullptr; + } + return *this; + } + + ~MonitorReq() + { + ReleasePin(); + } + + /// Undo the pinning reference exactly once; safe to call + /// unconditionally (no-op after the first call or on a + /// moved-from instance). + void ReleasePin() + { + if (release != nullptr) + { + release(ctx); + release = nullptr; + } + } + void* ctx = nullptr; MonWriter writer = nullptr; + MonRefFn release = nullptr; std::string line; }; diff --git a/src/mangosd/GdbServerThread.cpp b/src/mangosd/GdbServerThread.cpp index 9c0130b26..b23b5ce4b 100644 --- a/src/mangosd/GdbServerThread.cpp +++ b/src/mangosd/GdbServerThread.cpp @@ -277,6 +277,7 @@ class GdbMonSocket : public GdbSocketBase if (i > lineStart) { sGdbServer.SubmitMonitorLine(this, &GdbMonSocket::WriteTextThunk, + &GdbMonSocket::AddRefThunk, &GdbMonSocket::ReleaseThunk, inputBuffer + lineStart); } lineStart = i + 1; @@ -309,6 +310,19 @@ class GdbMonSocket : public GdbSocketBase static_cast(ctx)->sendCStr(text); } + // Pin/unpin this socket's ACE reference count around a queued + // monitor request so GdbServer can safely hold ctx across the + // network->world thread hop even if the connection closes first. + static void AddRefThunk(void* ctx) + { + static_cast(ctx)->add_reference(); + } + + static void ReleaseThunk(void* ctx) + { + static_cast(ctx)->remove_reference(); + } + char inputBuffer[MON_BUFF_SIZE]; uint32 inputBufferLen; };