Merging upstream TCPServer.h changed lock from std::mutex to
std::recursive_mutex (the ConsoleServer self-deadlock fix); this call
site still hardcoded std::lock_guard<std::mutex>, which no longer
compiles since lock_guard cannot bind to a different mutex type. Uses
the same class-template-argument-deduction form every other lock_guard
in this file already uses.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
TCPServer accepted a connection and never revisited it again once it
stopped generating epoll events - a connection that completes the TCP
handshake and then goes permanently silent (an incomplete TLS
handshake, a port scanner/prober) sat in sessions forever, one
TCPSession + one fd each. sweepStaleSessions() now drops any session
that has gone 30+ seconds without a single byte of real data ever
passing through onDataReceived(), run each time a new connection
arrives rather than needing a dedicated timer. hasReceivedData latches
permanently true on a sessions first real byte, so an ordinary
connected-but-briefly-idle client is never at risk.
Also raised listen()s backlog from 20 to 128, so a burst of connection
attempts has more headroom before the kernel starts dropping SYNs.
Verified live on zephora.barant.com: a silent connection held open was
reaped within the grace window while a real concurrent connection was
left untouched, with the service remaining healthy throughout.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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).
Socket::write() unconditionally called resetSocket() (via EPOLL_CTL_MOD)
from whatever thread called it, whenever it could grab `lock` - including
while this same socket's OWN eventReceived() call was still mid-flight on
another thread (past receiveData()'s lock_guard, but before its own
trailing resetSocket()/return). Re-arming makes the fd immediately
eligible for a third thread to pick up a fresh event and, if it decides
to shut the connection down, delete the object outright (Thread::run()
does exactly that on a false return) - while the original thread is still
touching it. A real heap-corruption crash inside libc's malloc internals
was confirmed live via dmesg under concurrent load, consistent with
exactly this: a socket freed while another thread still held a reference
to it.
inHandler existed as a flag already but only fed a diagnostic log line -
never actually gated anything. Made it a real std::atomic<bool> (was
`volatile`, which gives no actual atomicity or cross-thread visibility)
and used it two ways:
- write() now skips its own resetSocket() call entirely while inHandler
is true - that call's own eventReceived() will re-arm once it's
actually done, seeing the fifo entry write() just queued.
- eventReceived() itself was clearing inHandler well before its own
resetSocket() call (several statements up, right after EPOLLIN/
EPOLLWRNORM handling) - moved the clear to immediately precede that
call, closing the exposed window down to the minimum achievable
without restructuring how write() re-arms sockets altogether.
This is a distinct bug from the two subscriber/session-list races fixed
earlier tonight (SubscriptionManager.cpp, TCPServer.cpp) - found while
stress-testing those fixes turned up a third, different crash signature
(heap corruption, not the null-EPoll-reference pattern the earlier two
produced).
connectionRequest()/removeFromSessionList() already lock `lock` around
every mutation of `sessions` (a session accepting or disconnecting), but
every read of it - sendToAll() (all three overloads), getSessionByAlias(),
and the debug-console processCommand() - iterated it with no lock at all.
Exact same bug shape as Subscription::subscribers, fixed earlier today in
SubscriptionManager.cpp, just in a completely separate container.
Confirmed live via dmesg: a real segfault at a stable, deterministic
instruction offset inside the binary, hit twice across separate process
launches tonight - Socket::write() -> resetSocket() dereferencing a null
EPoll reference, the same crash signature as the earlier Subscription fix,
consistent with iterating `sessions` while another thread concurrently
erased and freed one of its entries. sendToAll() backs most real gameplay
broadcasts (combat, position, chat), so this is very likely the actual
dominant cause of tonight's crash - not just the ItemDataBase-specific
subscriber list the earlier fix addressed.
Took the existing `lock` mutex around all five read sites. No new
reentrancy risk: SubscriptionManager's "invite" verb already calls
getSessionByAlias() while holding SubscriptionManager::lock, so the two
mutexes now nest in one fixed order (SubscriptionManager::lock, then
TCPServer::lock) - nothing acquires them in the reverse order anywhere
in this codebase, so no AB-BA deadlock risk.
processCommand() read/mutated the `subscriptions` map and individual
Subscription objects' `subscribers` vectors (via subscribe()/unsubscribe()/
event()/the "publish" branch's own insert()) without taking `lock` at all,
even though add() and removeSessionSubscriptions() already do take it for
the exact same state. removeSessionSubscriptions() runs from ~TCPSession(),
on whatever thread happens to be tearing down a *different* connection, so
one thread erasing a session from a Subscription's subscribers vector could
run concurrently with another thread iterating (event()) or appending to
(subscribe()) that same vector - a plain unsynchronized data race.
Reproduced live while testing an unrelated ItemDataBase fix: rapid connect/
subscribe/disconnect cycles against the "createItem" subscription produced
a real segfault, confirmed via gdb - Socket::write() -> resetSocket()
dereferencing a corrupted EPoll reference, consistent with iterating a
vector that another thread was concurrently erasing from.
Wrapped the whole method body in a std::lock_guard on the same `lock`
mutex SubscriptionManager already has. Verified no reentrancy risk: nothing
reachable from processCommand() (including subscription handlers' own
process()/onSubscribe() implementations) calls back into processCommand(),
add(), or removeSessionSubscriptions() on the same thread, and `lock` is a
plain non-recursive std::mutex, so this can't deadlock.
Verified fix: the exact repro sequence that segfaulted the server twice
now runs clean, server stays up.
Not touched: Subscription::sendToAll() has no live callers (dead code);
dynamically `new`'d *OWNER subscriptions are still never delete()'d when
unpublished (a pre-existing leak, not a crash) - both out of scope here.
SubscriptionManager::processCommand()'s "invite" branch looked up its
target via getSessionByAlias(&invitee), where invitee is a std::string
local to that function call. getSessionByAlias(void*) compares by
pointer identity against each session's own alias - the address of a
brand-new local variable can never equal any of those, so the lookup
was guaranteed to return NULL every time, and the result was
dereferenced unconditionally right after. Any client sending a bare
"invite:" command crashed the entire process, not just its own
connection - same class of bug as the two GuildManager crashes fixed
earlier this session, just found afterward while documenting this
file's own mechanics.
This generic ServerCore layer has no name-based session lookup
available (that only exists as a Zephora-specific addition on top of
TCPServer), so this stops the crash with a null check rather than
attempting a real name-based fix, which would need a larger change.
Also includes this file's pending brace-style reformat (K&R -> Allman)
that was sitting uncommitted before this fix - bundled in rather than
left stranded, no functional change from that part.
subscriptions[id] via operator[] default-constructed (NULL) and
inserted an entry for any unregistered id, and the very next line
dereferenced it unconditionally - any client referencing a
subscription id that was never created (e.g. Zephora client sending
subscribe:invite for a channel nobody ever Publish()d) segfaulted the
entire process, not just that connection. Use find() and return
gracefully instead.
