From 2c65939788b9739d86047937725481050348d3f3 Mon Sep 17 00:00:00 2001 From: Matt Arant Date: Mon, 7 Sep 2026 18:15:03 +0000 Subject: [PATCH] Fix null-deref crash in base invite verb + pick up pending reformat 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. --- SubscriptionManager.cpp | 113 +++++++++++++++++++++++++++++----------- 1 file changed, 82 insertions(+), 31 deletions(-) diff --git a/SubscriptionManager.cpp b/SubscriptionManager.cpp index a54c873..ecb1705 100644 --- a/SubscriptionManager.cpp +++ b/SubscriptionManager.cpp @@ -5,60 +5,75 @@ #include "TCPServer.h" #include -namespace core { +namespace core +{ - int SubscriptionManager::add(Subscription &subscription) { + int SubscriptionManager::add(Subscription &subscription) + { lock.lock(); subscriptions.insert(std::make_pair(subscription.id, &subscription)); lock.unlock(); return 1; } - bool SubscriptionManager::onClearSubscription(coreutils::MString temp, coreutils::MString key) { + bool SubscriptionManager::onClearSubscription(coreutils::MString temp, coreutils::MString key) + { temp = ""; key = ""; return true; } - int SubscriptionManager::removeSessionSubscriptions(TCPSession &session) { + int SubscriptionManager::removeSessionSubscriptions(TCPSession &session) + { int countSubscribed = 0; int countPublished = 0; int count = 0; lock.lock(); coreutils::MString temp = ""; - for (auto [key, subscription] : subscriptions) { - if (temp != "") { + for (auto [key, subscription] : subscriptions) + { + if (temp != "") + { subscriptions.erase(temp); temp = ""; } countSubscribed += subscription->unsubscribe(session); - if (subscription->subscriptionOwners == coreutils::MString(session.alias)) { - if (subscription->selectionOfOwnerForSubscription == "*DIE") { + if (subscription->subscriptionOwners == coreutils::MString(session.alias)) + { + if (subscription->selectionOfOwnerForSubscription == "*DIE") + { onClearSubscription(temp, key); } - else if (subscription->will == "*NEXT") { - if (subscription->subscribers.size() < 1) { + else if (subscription->will == "*NEXT") + { + if (subscription->subscribers.size() < 1) + { coreutils::Log(coreutils::LOG_DEBUG_1) << "There is not enough people to move to the next"; onClearSubscription(temp, key); } - else { - for (auto subscribed = subscription->subscribers.begin(); subscribed < subscription->subscribers.end(); subscribed++) { - if ((*subscribed)->alias == session.alias) { + else + { + for (auto subscribed = subscription->subscribers.begin(); subscribed < subscription->subscribers.end(); subscribed++) + { + if ((*subscribed)->alias == session.alias) + { subscription->subscribers.erase(subscribed++); } subscription->subscriptionOwner(session.alias, "*NEXT"); } } } - else { + else + { coreutils::Log(coreutils::LOG_DEBUG_1) << "Subscription doesn't exist"; } ++countPublished; } } - if (temp != "") { + if (temp != "") + { subscriptions.erase(temp); temp = ""; } @@ -68,13 +83,17 @@ namespace core { return countSubscribed; } - int SubscriptionManager::processCommand(coreutils::ZString &request, TCPSession &session) { - if (request[0] == "publish") { + int SubscriptionManager::processCommand(coreutils::ZString &request, TCPSession &session) + { + if (request[0] == "publish") + { SubscriptionHandler *handler = NULL; - if (request.getList().size() > 3) { + if (request.getList().size() > 3) + { handler = factory->getSubscriptionHandler(request[4].str()); } - if (request[2] == "*OWNER") { + if (request[2] == "*OWNER") + { newSubscription = new Subscription(request[1].str(), session.alias, request[2].str(), request[3].str(), handler, request[5].str(), request[6].str()); subscriptions.insert(std::make_pair(request[1].str(), newSubscription)); return 1; @@ -82,19 +101,42 @@ namespace core { return 1; } - else if (request[0] == "catalog") { + else if (request[0] == "catalog") + { session.out << ":catalog:"; - for (auto const &[key, subscription] : subscriptions) { + for (auto const &[key, subscription] : subscriptions) + { session.out << subscription->id << ";"; } session.out << std::endl; return 1; } - else if (request[0] == "invite") { + else if (request[0] == "invite") + { std::stringstream out; coreutils::Log(coreutils::LOG_DEBUG_1) << request[2]; std::string invitee = request[2].str(); + // Was getSessionByAlias(&invitee) - getSessionByAlias(void*) + // compares by POINTER IDENTITY against each session's own + // alias, but &invitee is the address of a local variable that + // was just created in this function call - it can never equal + // any real session's alias, so this lookup was guaranteed to + // return NULL every time, and the result was dereferenced + // unconditionally below. Any client sending a bare "invite:" + // crashed the entire process, not just its own connection - + // same class of bug as the two GuildManager crashes fixed + // this session. This generic ServerCore layer has no + // name-based session lookup available (that only exists as a + // Zephora-specific addition on top of TCPServer), so a real + // fix for "find a session by the player name they typed" + // isn't possible at this layer without a larger change; this + // stops the crash without pretending to fix that gap. TCPSession *tempSession = session.server->getSessionByAlias(&invitee); + if (tempSession == NULL) + { + coreutils::Log(coreutils::LOG_DEBUG_1) << "invite: could not find session for " << invitee; + return 0; + } std::stringstream temp; temp << "invite:" << request[1] << ":" << invitee; tempSession->write(temp.str()); @@ -114,33 +156,42 @@ namespace core { // not just the one that sent it. find() avoids polluting the map // with the bogus NULL entry too. auto it = subscriptions.find(request[1].str()); - if (it == subscriptions.end()) { + if (it == subscriptions.end()) + { coreutils::Log(coreutils::LOG_DEBUG_1) << "Unknown subscription id: " << request[1].str(); return 0; } auto subscription = it->second; - if (request[1].equals(subscription->id)) { - if (request[0] == "unpublish") { + if (request[1].equals(subscription->id)) + { + if (request[0] == "unpublish") + { subscriptions.erase(request[1].str()); } - else if (request[0] == "subscribe") { + else if (request[0] == "subscribe") + { subscription->subscribe(session); return 1; } - else if (request[0] == "unsubscribe") { + else if (request[0] == "unsubscribe") + { subscription->unsubscribe(session); return 1; } - else if (request[0] == "event") { + else if (request[0] == "event") + { std::stringstream out; subscription->process(request, out, session); - if (subscription->mode == "*ANYONE") { + if (subscription->mode == "*ANYONE") + { subscription->event(out); return 1; } - else if (subscription->mode == "*SUBSCRIBERS") { - if (subscription->ifSubscriber(session)) { + else if (subscription->mode == "*SUBSCRIBERS") + { + if (subscription->ifSubscriber(session)) + { subscription->event(out); return 1; }