From b855b279a78109ca1acbde22ceefbc530cb3be7e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Wed, 2 Sep 2026 14:18:34 +0200 Subject: [PATCH] [Server] Record only the dispatched command family; drop unused totals processCommandContainer recorded every family in a container even though the base if/else-if dispatch processes at most one. An unauthenticated client could batch a session command (login) with fabricated developer, moderator, and admin entries and forge genuine-looking samples that were never executed or authorized. Mirror the base's selection, skip when the handler was already deleted, and skip entries whose extension number is -1 (which would otherwise wrap into the previous kind's id range). [Server] Drop dead process-lifetime byte/uptime counters txBytesTotal/rxBytesTotal added an atomic RMW to every socket write and read for counters nothing consumes (cmdGetServerStats fills tx_bytes, rx_bytes, and uptime_secs from the DB snapshot). Remove the two atomics and the getTxBytesTotal/getRxBytesTotal/getUptimeSeconds getters; the incTxBytes/incRxBytes slots and mutexes remain for the ISL legacy counters. [Protocol] Document kind 5 as developer in CommandStats NumKinds is 6 and the server emits kind_index = 5 for developer commands; the comment stopped at 4. --- .../pb/response_get_server_stats.proto | 2 +- servatrice/src/servatrice.cpp | 4 -- servatrice/src/servatrice.h | 14 ---- servatrice/src/serversocketinterface.cpp | 67 ++++++++++++++----- 4 files changed, 51 insertions(+), 36 deletions(-) diff --git a/libcockatrice_protocol/libcockatrice/protocol/pb/response_get_server_stats.proto b/libcockatrice_protocol/libcockatrice/protocol/pb/response_get_server_stats.proto index 8788834ea..bb8ff3c43 100644 --- a/libcockatrice_protocol/libcockatrice/protocol/pb/response_get_server_stats.proto +++ b/libcockatrice_protocol/libcockatrice/protocol/pb/response_get_server_stats.proto @@ -2,7 +2,7 @@ syntax = "proto2"; import "response.proto"; message CommandStats { - optional uint32 kind_index = 1; // 0=session, 1=room, 2=game, 3=moderator, 4=admin + optional uint32 kind_index = 1; // 0=session, 1=room, 2=game, 3=moderator, 4=admin, 5=developer optional uint32 extension_number = 2; // protobuf extension number within the kind optional string command_name = 3; // e.g. "session/Command_Ping" optional uint64 count = 4; // number of times observed diff --git a/servatrice/src/servatrice.cpp b/servatrice/src/servatrice.cpp index f08da973e..26352ccd7 100644 --- a/servatrice/src/servatrice.cpp +++ b/servatrice/src/servatrice.cpp @@ -89,7 +89,6 @@ void Servatrice_GameServer::incomingConnection(qintptr socketDescriptor) Servatrice_ConnectionPool *pool = findLeastUsedConnectionPool(); auto ssi = new TcpServerSocketInterface(server, pool->getDatabaseInterface()); - connect(ssi, SIGNAL(incTxBytes(qint64)), this, SLOT(incTxBytes(qint64))); ssi->moveToThread(pool->thread()); pool->addClient(); connect(ssi, SIGNAL(destroyed()), pool, SLOT(removeClient())); @@ -160,7 +159,6 @@ void Servatrice_WebsocketGameServer::onNewConnection() Servatrice_ConnectionPool *pool = findLeastUsedConnectionPool(); auto ssi = new WebsocketServerSocketInterface(server, pool->getDatabaseInterface()); - connect(ssi, SIGNAL(incTxBytes(quint64)), this, SLOT(incTxBytes(quint64))); /* * Due to a Qt limitation, websockets can't be moved to another thread. * This will hopefully change in Qt6 if QtWebSocket will be integrated in QtNetwork @@ -785,7 +783,6 @@ void Servatrice::incTxBytes(quint64 num) txBytesMutex.lock(); txBytes += num; txBytesMutex.unlock(); - txBytesTotal.fetch_add(num, std::memory_order_relaxed); } void Servatrice::incRxBytes(quint64 num) @@ -793,7 +790,6 @@ void Servatrice::incRxBytes(quint64 num) rxBytesMutex.lock(); rxBytes += num; rxBytesMutex.unlock(); - rxBytesTotal.fetch_add(num, std::memory_order_relaxed); } void Servatrice::shutdownTimeout() diff --git a/servatrice/src/servatrice.h b/servatrice/src/servatrice.h index 03cb46719..8d964a52b 100644 --- a/servatrice/src/servatrice.h +++ b/servatrice/src/servatrice.h @@ -173,8 +173,6 @@ private: int uptime; QMutex txBytesMutex, rxBytesMutex; quint64 txBytes, rxBytes; - std::atomic txBytesTotal{0}; ///< cumulative bytes sent since process start - std::atomic rxBytesTotal{0}; ///< cumulative bytes received since process start MetricsRegistry metricsRegistry; int metricsSlowCommandMs = 500; int metricsStallWarnMs = 2000; @@ -302,18 +300,6 @@ public: { return metricsRegistry; } - quint64 getTxBytesTotal() const - { - return txBytesTotal.load(std::memory_order_relaxed); - } - quint64 getRxBytesTotal() const - { - return rxBytesTotal.load(std::memory_order_relaxed); - } - int getUptimeSeconds() const - { - return uptime; - } /** * Sums cards across all zones of all running games. Each game takes its * own gameMutex -- the hot per-game lock every game action contends on -- diff --git a/servatrice/src/serversocketinterface.cpp b/servatrice/src/serversocketinterface.cpp index 8e1da7083..aeffd7081 100644 --- a/servatrice/src/serversocketinterface.cpp +++ b/servatrice/src/serversocketinterface.cpp @@ -202,25 +202,58 @@ void AbstractServerSocketInterface::processCommandContainer(const CommandContain Server_ProtocolHandler::processCommandContainer(cont); const qint64 elapsedMs = timer.nsecsElapsed() / 1000000; - // A container usually holds a single command. When several are batched, - // each is attributed the container's total processing time. - for (const auto &cmd : cont.session_command()) { - servatrice->getMetricsRegistry().observeCommand(MetricsRegistry::typeIdFor(0, getPbExtension(cmd)), elapsedMs); + // The base dispatch is an if/else-if chain — at most one family is + // actually processed. Recording every family in the container would + // let an unauthenticated client stampforge developer/moderator/admin + // samples by batching them alongside a session command the server + // actually runs. Mirror the base's selection and skip entirely when + // deleted or when no family matched. + if (deleted) { + return; } - for (const auto &cmd : cont.room_command()) { - servatrice->getMetricsRegistry().observeCommand(MetricsRegistry::typeIdFor(1, getPbExtension(cmd)), elapsedMs); + + // When getPbExtension returns -1 (no extension set) and the kind is + // non-zero, typeIdFor wraps into the previous kind's range instead of + // hitting the typeId < 0 guard in observeCommand. Skip such entries. + int kind = -1; + if (cont.game_command_size()) { + kind = 2; + } else if (cont.room_command_size()) { + kind = 1; + } else if (cont.session_command_size()) { + kind = 0; + } else if (cont.moderator_command_size()) { + kind = 3; + } else if (cont.admin_command_size()) { + kind = 4; + } else if (cont.developer_command_size()) { + kind = 5; } - for (const auto &cmd : cont.game_command()) { - servatrice->getMetricsRegistry().observeCommand(MetricsRegistry::typeIdFor(2, getPbExtension(cmd)), elapsedMs); - } - for (const auto &cmd : cont.moderator_command()) { - servatrice->getMetricsRegistry().observeCommand(MetricsRegistry::typeIdFor(3, getPbExtension(cmd)), elapsedMs); - } - for (const auto &cmd : cont.admin_command()) { - servatrice->getMetricsRegistry().observeCommand(MetricsRegistry::typeIdFor(4, getPbExtension(cmd)), elapsedMs); - } - for (const auto &cmd : cont.developer_command()) { - servatrice->getMetricsRegistry().observeCommand(MetricsRegistry::typeIdFor(5, getPbExtension(cmd)), elapsedMs); + + if (kind >= 0) { + auto recordDispatched = [&](int familyKind, const auto &cmds) { + for (const auto &cmd : cmds) { + const int ext = getPbExtension(cmd); + if (ext >= 0) { + servatrice->getMetricsRegistry().observeCommand(MetricsRegistry::typeIdFor(familyKind, ext), + elapsedMs); + } + } + }; + + if (kind == 0) { + recordDispatched(kind, cont.session_command()); + } else if (kind == 1) { + recordDispatched(kind, cont.room_command()); + } else if (kind == 2) { + recordDispatched(kind, cont.game_command()); + } else if (kind == 3) { + recordDispatched(kind, cont.moderator_command()); + } else if (kind == 4) { + recordDispatched(kind, cont.admin_command()); + } else { + recordDispatched(kind, cont.developer_command()); + } } const int slowCommandMs = servatrice->getMetricsSlowCommandMs();