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 +#include 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 guard(lock); + if (request[0] == "publish") { SubscriptionHandler *handler = NULL;