Fix unsynchronized data race in SubscriptionManager::processCommand()

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.
This commit is contained in:
Matt Arant committed 2026-09-07 20:18:14 +00:00
1 parent 2c65939788
commit 3383595296
1 file changed
+19
+19
View File
@@ -4,6 +4,7 @@
#include "SubscriptionHandlerFactory.h" #include "SubscriptionHandlerFactory.h"
#include "TCPServer.h" #include "TCPServer.h"
#include <algorithm> #include <algorithm>
#include <mutex>
namespace core namespace core
{ {
@@ -85,6 +86,24 @@ namespace core
int SubscriptionManager::processCommand(coreutils::ZString &request, TCPSession &session) int SubscriptionManager::processCommand(coreutils::ZString &request, TCPSession &session)
{ {
// Every branch below reads or mutates either the `subscriptions` map
// or some Subscription's own `subscribers` vector - both of which
// removeSessionSubscriptions() (called from ~TCPSession(), on
// whatever thread happens to be tearing down a *different*
// connection) also reads/mutates, under `lock`. This method used to
// take no lock at all, so a session disconnecting on one thread
// could run subscribers.erase() concurrently with subscribe()/
// event() iterating or push_back()-ing the same vector on another -
// an unsynchronized data race. Confirmed live: rapid connect/
// subscribe/disconnect cycles against ItemDataBase's "createItem"
// subscription produced a real segfault (gdb: Socket::write() ->
// resetSocket() dereferencing a corrupted EPoll reference) while
// broadcasting to *SUBSCRIBERS. `lock` is a plain, non-recursive
// std::mutex, and nothing reachable from this function re-enters
// processCommand()/add()/removeSessionSubscriptions() on the same
// thread, so holding it for the whole call is safe.
std::lock_guard<std::mutex> guard(lock);
if (request[0] == "publish") if (request[0] == "publish")
{ {
SubscriptionHandler *handler = NULL; SubscriptionHandler *handler = NULL;