From 0bb926a26412d5600f746226b3238e8595deca6a Mon Sep 17 00:00:00 2001
From: Matt Arant <matt@zephoragaming.com>
Date: Mon, 7 Sep 2026 23:31:28 +0000
Subject: [PATCH] Fix unsynchronized data race in TCPServer::sessions

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.
---
 TCPServer.cpp | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/TCPServer.cpp b/TCPServer.cpp
index 83073e5..e3ba0a8 100644
--- a/TCPServer.cpp
+++ b/TCPServer.cpp
@@ -123,6 +123,21 @@ namespace core {
 
    int TCPServer::processCommand(coreutils::ZString &request, TCPSession &session)
    {
+      // Was unguarded, like every other method below - `sessions` is
+      // mutated under `lock` by connectionRequest()/removeFromSessionList()
+      // (a session accepting or disconnecting on another thread), but
+      // every READ of it here had no lock at all. Confirmed live via
+      // dmesg: a real segfault at a stable, deterministic instruction
+      // offset (same crash signature as the Subscription::subscribers
+      // race fixed earlier this session, in SubscriptionManager.cpp) -
+      // Socket::write() -> resetSocket() dereferencing a null EPoll
+      // reference, consistent with iterating `sessions` while another
+      // thread concurrently erased and freed one of its entries.
+      // sendToAll() in particular is what most real gameplay broadcasts
+      // (combat, position, chat) go through, so this is very likely the
+      // actual dominant cause of that crash, not just the ItemDataBase
+      // subscriber list this session's earlier fix addressed.
+      std::lock_guard<std::mutex> guard(lock);
       int sequence = 0;
       for (auto *sessionx : sessions)
       {
@@ -134,12 +149,14 @@ namespace core {
    }
 
    void TCPServer::sendToAll(std::stringstream &data) {
+      std::lock_guard<std::mutex> guard(lock);
       for (auto session : sessions)
 	session->write(data.str());
       data.str("");
    }
 
    void TCPServer::sendToAll(std::stringstream &data, TCPSession &sender) {
+      std::lock_guard<std::mutex> guard(lock);
       for (auto session : sessions)
          if (session != &sender)
             session->write(data.str());
@@ -147,6 +164,7 @@ namespace core {
    }
 
    void TCPServer::sendToAll(std::stringstream &data, TCPSession &sender, SessionFilter filter) {
+      std::lock_guard<std::mutex> guard(lock);
       for (auto session : sessions)
          if (filter.test(*session))
             if (session != &sender)
@@ -156,6 +174,7 @@ namespace core {
 
    TCPSession *TCPServer::getSessionByAlias(void *alias) {
       coreutils::Log(coreutils::LOG_DEBUG_1) << alias;
+      std::lock_guard<std::mutex> guard(lock);
       for (auto session : sessions)
 	if (session->alias == alias)
 	  return session;