Files
ServerCore/TCPSession.h
T
Matt Arant d058322cc6 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).
2026-09-08 02:24:09 +00:00

261 lines
9.5 KiB
C++

#ifndef __Session_h__
# define __Session_h__
# include "SessionFilter.h"
# include "TCPSocket.h"
# include <map>
# include <memory>
# include <mutex>
# include <sstream>
namespace core {
class Command;
class TCPServer;
///
/// TCPSession
///
/// TCPSession defines the nature of the interaction with the client
/// and stores persistent data for a defined session. TCPSession objects
/// are not sockets but instead provide a communications control
/// mechanism. Protocol conversations are provided through extensions
/// from this object.
///
///
///
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 &registryMutex() {
static std::mutex m;
return m;
}
static std::map<TCPSession *, std::weak_ptr<TCPSession>> &registry() {
static std::map<TCPSession *, std::weak_ptr<TCPSession>> r;
return r;
}
public:
///
///
///
TCPSession(EPoll &ePoll, std::string text = "");
///
///
///
TCPSession(EPoll &ePoll, TCPServer *server, std::string text = "");
///
///
///
virtual ~TCPSession();
Command *grab = NULL;
virtual void output(std::stringstream &data);
///
/// The send method is used to output the contents of the out stream
/// to the session containing the stream.
///
void send();
///
/// Use this method to terminate this TCPSession.
///
void terminate();
///
/// This is a pointer to the server that was used to 'spawn' the server connection.
/// If the session is just client side connection this pointer should be NULL;
///
TCPServer *server;
///
/// Use out to send data to the session socket or other session sockets.
///
std::stringstream out;
///
/// uuid is generated automatically when the session object is instantiated. This
/// value can be used to uniquely identify a session and is the default value
/// pointed to by the alias pointer.
///
char uuid[37];
///
/// alias is a void pointer that can be set to point to any object that identifies
/// this session uniquely. Using this approach, inheriting objects can determine
/// how it knows the contacts that this server manages.
///
void *alias;
///
///
///
virtual bool compareAlias(void *alias);
virtual void outputAlias(std::stringstream &out);
protected:
///
///
///
virtual void onRegistered() override;
///
/// Override the onLineReceived method to receive a string of characters that
/// represents a single line of data terminated by a LF or CRLF. If onDataReceived
/// was overriden this method will not be called unless the onDataReceived calls
/// this method explicitly using the class and member name.
///
virtual void onLineReceived(coreutils::ZString &line) override;
///
/// This method is called from within the protocol method when protocol is called
/// on the initial connection where the data is an empty string. Use this method
/// to deliver a message to the connection upon connection.
///
virtual void onConnected();
///
/// Override the protocol method to manage and control the session communications
/// in your inherited session. If you do not override this method then the Session
/// default will process the 'commands' added to the server object using the
/// processRequest method on the session input.
///
/// When data is received within the session two modes are available to pass the
/// data through the protocol method: LINE or BLOCK.
///
virtual void protocol(coreutils::ZString &data);
private:
std::mutex mtx;
};
}
#endif