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.
Matt Arant