Move TCPSession lifetime to shared_ptr ownership, fix the whole double-delete/UAF class of crash
Every crash root-caused earlier tonight (Subscription::subscribers, TCPServer::sessions, the Socket::write()/eventReceived() re-arm race) was the same underlying shape: some thread calling a method - or Thread::run() calling delete - on a TCPSession* while another thread concurrently freed that same object, because nothing actually guaranteed the object stayed alive for the duration of a call. Mutex-based patches at each individual call site kept narrowing the window without closing it (confirmed live, repeatedly, via AddressSanitizer under concurrent load - each fix produced a new, different crash signature at the same underlying race), and adding more locks around every write() call also works against the fifo-based design's own goal: no thread should have to block just to hand a message to another thread's session. TCPSession now derives from std::enable_shared_from_this<TCPSession> and exposes pin() - a safe upgrade from a stored raw pointer to a real, ownership-extending shared_ptr, returning nullptr instead of throwing once the object's real owner has let go. TCPServer::sessions is now the one real strong-owning shared_ptr<TCPSession> per live session; every other place that stores a TCPSession* (Subscription::subscribers, Zephora's own sessionsByPlayerName, GroupObject's membership list, etc.) is just a non-owning observer and must pin() before touching the object. A second, distinct problem: Thread::run()'s epoll dispatch starts from a raw void* of unknown validity. An early version of this fix used dynamic_cast<TCPSession*> to identify session objects before calling pin() - confirmed live via AddressSanitizer that dynamic_cast itself segfaults on a stale pointer, since it has to read the (potentially freed) object's own vtable to work at all. TCPSession::tryGet()/TCPServer:: isKnownListeningSocket() replace that: both look the raw pointer up as a key in a registry, never dereferencing it for the check itself - safe to call on any value, live session, immortal listening socket, or fully freed garbage alike. Verified via an AddressSanitizer-instrumented build under sustained concurrent load (up to 15-way parallel connect/subscribe/create/disconnect cycles, multiple rounds, 100+ total connections) with zero errors, then against the real production build with a genuine 500-connection load test (463/500 succeeded; the rest hit the process's own file-descriptor ulimit under peak burst, an unrelated and separately deferred tuning matter, not a crash). Files touched: TCPSession.h (enable_shared_from_this, pin(), the tryGet() registry), TCPServer.h/.cpp (sessions as shared_ptr, isKnownListening Socket() registry, lock made protected for subclasses), Thread.cpp (the whole dispatch rewrite), Subscription.cpp/SubscriptionManager.cpp (every subscriber write() site now pin()s first), ConsoleServer.cpp (logSend() was iterating `sessions` with no lock at all - fixed alongside the type change it needed anyway).
This commit is contained in:
1 parent
8009564a7a
commit
d058322cc6
10 files changed
+411
-64
No files matched your search
+119
-3
@@ -3,6 +3,9 @@
|
||||
|
||||
# include "SessionFilter.h"
|
||||
# include "TCPSocket.h"
|
||||
# include <map>
|
||||
# include <memory>
|
||||
# include <mutex>
|
||||
# include <sstream>
|
||||
|
||||
namespace core {
|
||||
@@ -22,10 +25,123 @@ namespace core {
|
||||
///
|
||||
///
|
||||
|
||||
class TCPSession : public TCPSocket {
|
||||
|
||||
class TCPSession : public TCPSocket, public std::enable_shared_from_this<TCPSession> {
|
||||
|
||||
public:
|
||||
|
||||
|
||||
///
|
||||
/// pin() safely upgrades a raw TCPSession* (as stored in
|
||||
/// Subscription::subscribers, ZephoraServer's own
|
||||
/// sessionsByPlayerName, GroupManager's membership lists, etc.)
|
||||
/// into a real, ownership-extending shared_ptr before it's used.
|
||||
///
|
||||
/// The core problem this solves: every crash found and root-caused
|
||||
/// this session (SubscriptionManager.cpp/TCPServer.cpp/Socket.cpp's
|
||||
/// own fixes) was the same shape - one thread calling a method
|
||||
/// (write(), a destructor) on a TCPSession* while another thread
|
||||
/// concurrently deleted that same object, because nothing actually
|
||||
/// guaranteed the object stayed alive for the duration of the
|
||||
/// call. Mutex-based fixes at each individual call site kept
|
||||
/// narrowing the window without fully closing it, and a mutex
|
||||
/// around every write() call also isn't what the fifo-based
|
||||
/// design (see Socket::write()) was going for in the first place -
|
||||
/// no thread should have to block just to hand off a message.
|
||||
///
|
||||
/// TCPServer::sessions now holds the one real, strong-owning
|
||||
/// shared_ptr<TCPSession> per live session (see its own comment).
|
||||
/// Every OTHER place that stores a TCPSession* for later use is a
|
||||
/// plain observer, not an owner - it must call pin() and check for
|
||||
/// null immediately before touching the object, rather than
|
||||
/// dereferencing the raw pointer directly. pin() returns nullptr
|
||||
/// (instead of throwing, unlike raw shared_from_this()) once the
|
||||
/// object's last real owner has let go and it's on its way out -
|
||||
/// at that point there is nothing left to safely do with it, and
|
||||
/// the caller should just skip it as if it were never there,
|
||||
/// exactly like a stale/already-unsubscribed entry.
|
||||
///
|
||||
/// This adds no blocking anywhere: shared_ptr's own refcounting is
|
||||
/// a couple of atomic increments, not a mutex - the actual
|
||||
/// deletion still only ever happens once, whenever the last
|
||||
/// outstanding shared_ptr (held by TCPServer::sessions, or
|
||||
/// briefly by whichever thread(s) are mid-call via their own
|
||||
/// pin()'d reference) goes out of scope.
|
||||
///
|
||||
|
||||
std::shared_ptr<TCPSession> pin() {
|
||||
try { return shared_from_this(); }
|
||||
catch (const std::bad_weak_ptr &) { return nullptr; }
|
||||
}
|
||||
|
||||
///
|
||||
/// A second, distinct safety problem pin()/shared_from_this()
|
||||
/// alone cannot solve: Thread::run()'s epoll dispatch loop starts
|
||||
/// from a raw `void*` (epoll_event.data.ptr) whose TYPE isn't known
|
||||
/// yet - the original code cast it straight to `Socket*` and
|
||||
/// called virtual methods on it; an early version of this fix used
|
||||
/// dynamic_cast<TCPSession*> to identify session objects before
|
||||
/// calling pin(). Both of those REQUIRE reading the pointed-to
|
||||
/// object's own memory (its vtable, at minimum) to work at all -
|
||||
/// confirmed live via AddressSanitizer that dynamic_cast on a
|
||||
/// stale pointer (one epoll handed out for a TCPSession that
|
||||
/// something else had already fully destroyed and freed on another
|
||||
/// thread) segfaults outright, before pin()'s own safety logic
|
||||
/// ever gets a chance to run.
|
||||
///
|
||||
/// tryGet() sidesteps this entirely: it looks the raw pointer VALUE
|
||||
/// up as a key in a plain map, and only ever touches a weak_ptr's
|
||||
/// own control block (a separate heap allocation that outlives the
|
||||
/// TCPSession object itself for as long as any weak_ptr or
|
||||
/// shared_ptr referencing it exists) - never the TCPSession's own
|
||||
/// memory - to decide whether it's still valid. That makes it safe
|
||||
/// to call on ANY raw pointer value, including ones that turn out
|
||||
/// to be completely unrelated to this registry (never a
|
||||
/// TCPSession, e.g. a listening TCPServer/ConsoleServer - see
|
||||
/// TCPServer::isKnownListeningSocket() for how Thread::run()
|
||||
/// distinguishes that case) or ones pointing at freed memory.
|
||||
///
|
||||
/// registerLive()/unregisterLive() are called exactly once each,
|
||||
/// by TCPServer::connectionRequest() and removeFromSessionList()
|
||||
/// respectively - see their own comments for why those are the
|
||||
/// right, and only, call sites.
|
||||
///
|
||||
|
||||
static void registerLive(TCPSession *raw, std::shared_ptr<TCPSession> owner) {
|
||||
std::lock_guard<std::mutex> guard(registryMutex());
|
||||
registry()[raw] = owner;
|
||||
}
|
||||
|
||||
static void unregisterLive(TCPSession *raw) {
|
||||
std::lock_guard<std::mutex> guard(registryMutex());
|
||||
registry().erase(raw);
|
||||
}
|
||||
|
||||
static std::shared_ptr<TCPSession> tryGet(void *raw) {
|
||||
std::lock_guard<std::mutex> guard(registryMutex());
|
||||
auto it = registry().find((TCPSession *)raw);
|
||||
if (it == registry().end())
|
||||
return nullptr;
|
||||
return it->second.lock();
|
||||
}
|
||||
|
||||
private:
|
||||
// Function-local statics (constructed on first use, in whichever
|
||||
// translation unit/thread reaches them first, thread-safely per
|
||||
// C++11's own guarantee for static local initialization) instead
|
||||
// of ordinary static data members - avoids needing a .cpp
|
||||
// definition for these two and any static-initialization-order
|
||||
// concerns with other translation units, since TCPSession.h is
|
||||
// included widely.
|
||||
static std::mutex ®istryMutex() {
|
||||
static std::mutex m;
|
||||
return m;
|
||||
}
|
||||
static std::map<TCPSession *, std::weak_ptr<TCPSession>> ®istry() {
|
||||
static std::map<TCPSession *, std::weak_ptr<TCPSession>> r;
|
||||
return r;
|
||||
}
|
||||
|
||||
public:
|
||||
|
||||
///
|
||||
///
|
||||
///
|
||||
|
||||
Reference in new issue
Block a user