From 3383595296e0a666ec27f5038e8fb02775a8c70b Mon Sep 17 00:00:00 2001
From: Matt Arant <matt@zephoragaming.com>
Date: Mon, 7 Sep 2026 20:18:14 +0000
Subject: [PATCH] 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.
---
 SubscriptionManager.cpp | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/SubscriptionManager.cpp b/SubscriptionManager.cpp
index ecb1705..4b8322e 100644
--- a/SubscriptionManager.cpp
+++ b/SubscriptionManager.cpp
@@ -4,6 +4,7 @@
 #include "SubscriptionHandlerFactory.h"
 #include "TCPServer.h"
 #include <algorithm>
+#include <mutex>
 
 namespace core
 {
@@ -85,6 +86,24 @@ namespace core
 
    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")
       {
          SubscriptionHandler *handler = NULL;