From 7de9196d7dd5792b267e9cd49b4c8421ae49d8d5 Mon Sep 17 00:00:00 2001 From: Kawe Mazidjatari <48657826+Mauler125@users.noreply.github.com> Date: Sun, 9 Feb 2025 17:05:41 +0100 Subject: [PATCH] Engine: optimize and harden RCON message processor - Directly copy available bytes into message buffer instead of 1 per iteration. - Use an actual frame header and make desync error detection more robust. --- src/common/igameserverdata.h | 20 +++---- src/engine/shared/base_rcon.cpp | 94 ++++++++++++++++++------------- src/engine/shared/base_rcon.h | 4 +- src/engine/shared/shared_rcon.cpp | 17 +++--- 4 files changed, 74 insertions(+), 61 deletions(-) diff --git a/src/common/igameserverdata.h b/src/common/igameserverdata.h index 69811952..00cd3b87 100644 --- a/src/common/igameserverdata.h +++ b/src/common/igameserverdata.h @@ -4,10 +4,11 @@ // //===========================================================================// #pragma once +#include "netcon/INetCon.h" typedef int SocketHandle_t; -enum class ServerDataRequestType_t : int +enum class ServerDataRequestType_e : int { SERVERDATA_REQUEST_VALUE = 0, SERVERDATA_REQUEST_SETVALUE, @@ -17,7 +18,7 @@ enum class ServerDataRequestType_t : int SERVERDATA_REQUEST_SEND_REMOTEBUG, }; -enum class ServerDataResponseType_t : int +enum class ServerDataResponseType_e : int { SERVERDATA_RESPONSE_VALUE = 0, SERVERDATA_RESPONSE_UPDATE, @@ -38,7 +39,8 @@ public: bool m_bValidated; // Revalidates netconsole if false. bool m_bAuthorized; // Set to true after successful netconsole auth. bool m_bInputOnly; // If set, don't send spew to this netconsole. - vector m_RecvBuffer; + NetConFrameHeader_s m_FrameHeader; // Current frame header. + vector m_RecvBuffer; CConnectedNetConsoleData(SocketHandle_t hSocket = -1) { @@ -56,15 +58,11 @@ public: /* PACKET FORMAT ********************************** REQUEST: - int requestID; - int ServerDataRequestType_t; - NullTerminatedString (variable or command) - NullTerminatedString (value) + NetConFrameHeader_s header; + byte* data; RESPONSE: - int requestID; - int ServerDataResponseType_t; - NullTerminatedString (variable) - NullTerminatedString (value) + NetConFrameHeader_s header; + byte* data; ***************************************************/ diff --git a/src/engine/shared/base_rcon.cpp b/src/engine/shared/base_rcon.cpp index 4ce723a2..c75a6af7 100644 --- a/src/engine/shared/base_rcon.cpp +++ b/src/engine/shared/base_rcon.cpp @@ -144,69 +144,83 @@ bool CNetConBase::Connect(const char* pHostName, const int nPort) // nMaxLen - // Output: true on success, false otherwise //----------------------------------------------------------------------------- -bool CNetConBase::ProcessBuffer(CConnectedNetConsoleData& data, - const char* pRecvBuf, int nRecvLen, const int nMaxLen) +bool CNetConBase::ProcessBuffer(CConnectedNetConsoleData& data, const char* pRecvBuf, int nRecvLen, const int nMaxLen) { - byte prefix[sizeof(u_long)] = {}; - while (nRecvLen > 0) { + // Read payload if it's already in progress. if (data.m_nPayloadLen) { - if (data.m_nPayloadRead < data.m_nPayloadLen) - { - data.m_RecvBuffer[data.m_nPayloadRead++] = *pRecvBuf; + const int bytesToCopy = Min(nRecvLen, data.m_nPayloadLen - data.m_nPayloadRead); + memcpy(&data.m_RecvBuffer[data.m_nPayloadRead], pRecvBuf, bytesToCopy); + + data.m_nPayloadRead += bytesToCopy; + + pRecvBuf += bytesToCopy; + nRecvLen -= bytesToCopy; - pRecvBuf++; - nRecvLen--; - } if (data.m_nPayloadRead == data.m_nPayloadLen) { - if (!ProcessMessage( - reinterpret_cast(data.m_RecvBuffer.data()), data.m_nPayloadLen)) - { + if (!ProcessMessage(reinterpret_cast(data.m_RecvBuffer.data()), data.m_nPayloadLen)) return false; - } + // Reset state. data.m_nPayloadLen = 0; data.m_nPayloadRead = 0; } } - else if (data.m_nPayloadRead < sizeof(u_long)) // Read size field. + else if (data.m_nPayloadRead < sizeof(NetConFrameHeader_s)) // Read the header if we haven't fully recv'd it. { - prefix[data.m_nPayloadRead++] = *pRecvBuf; + const int bytesToCopy = Min(nRecvLen, int(sizeof(NetConFrameHeader_s)) - data.m_nPayloadRead); + memcpy(reinterpret_cast(&data.m_FrameHeader) + data.m_nPayloadRead, pRecvBuf, bytesToCopy); - pRecvBuf++; - nRecvLen--; - } - else // Build prefix. - { - u_long* const pPrefix = reinterpret_cast(&prefix[0]); + data.m_nPayloadRead += bytesToCopy; - data.m_nPayloadLen = int(ntohl(*pPrefix)); - data.m_nPayloadRead = 0; + pRecvBuf += bytesToCopy; + nRecvLen -= bytesToCopy; - *pPrefix = 0; - - if (!data.m_bAuthorized && nMaxLen > -1) + if (data.m_nPayloadRead == sizeof(NetConFrameHeader_s)) { - if (data.m_nPayloadLen > nMaxLen) + NetConFrameHeader_s& header = data.m_FrameHeader; + + // Convert byte order and check for desync. + header.magic = ntohl(header.magic); + const char* desyncReason = nullptr; + + if (header.magic != RCON_FRAME_MAGIC) { - Disconnect("overflow"); // Sending large messages while not authenticated. + desyncReason = "invalid magic"; + } + + if (!desyncReason) + { + header.length = ntohl(header.length); + + if (header.length == 0) + { + desyncReason = "empty frame"; + } + } + + if (desyncReason) + { + Error(eDLL_T::ENGINE, NO_ERROR, "RCON Cmd: sync error (%s)\n", desyncReason); + Disconnect("desync"); + return false; } - } - if (data.m_nPayloadLen <= 0 || data.m_nPayloadLen > RCON_MAX_PAYLOAD_SIZE) - { - Error(eDLL_T::ENGINE, NO_ERROR, "RCON Cmd: sync error (%d)\n", data.m_nPayloadLen); - Disconnect("desync"); // Out of sync (irrecoverable). + if ((!data.m_bAuthorized && nMaxLen > -1 && header.length > (u32)nMaxLen) || + header.length > RCON_FRAME_MAX_SIZE) + { + Disconnect("overflow"); + return false; + } - return false; - } - else - { - data.m_RecvBuffer.resize(data.m_nPayloadLen); + data.m_nPayloadLen = header.length; + data.m_nPayloadRead = 0; + + data.m_RecvBuffer.resize(header.length); } } } @@ -338,7 +352,7 @@ void CNetConBase::Recv(CConnectedNetConsoleData& data, const int nMaxLen) break; } - nReadLen -= nRecvLen; // Process what we've got. + nReadLen -= static_cast(nRecvLen); // Process what we've got. if (!ProcessBuffer(data, szRecvBuf, nRecvLen, nMaxLen)) break; diff --git a/src/engine/shared/base_rcon.h b/src/engine/shared/base_rcon.h index 8bbf2ec4..8a664861 100644 --- a/src/engine/shared/base_rcon.h +++ b/src/engine/shared/base_rcon.h @@ -1,14 +1,12 @@ #ifndef BASE_RCON_H #define BASE_RCON_H +#include "netcon/INetCon.h" #include "tier1/NetAdr.h" #include "tier2/cryptutils.h" #include "tier2/socketcreator.h" #include "protobuf/message_lite.h" -// Max size of the payload in the envelope frame -#define RCON_MAX_PAYLOAD_SIZE 1024*1024 - class CNetConBase { public: diff --git a/src/engine/shared/shared_rcon.cpp b/src/engine/shared/shared_rcon.cpp index d8008c7e..739bdb82 100644 --- a/src/engine/shared/shared_rcon.cpp +++ b/src/engine/shared/shared_rcon.cpp @@ -174,13 +174,10 @@ bool NetconShared_PackEnvelope(const CNetConBase* pBase, vector& outMsgBuf envelope.set_data(dataBuf, nMsgLen); const size_t envelopeSize = envelope.ByteSizeLong(); - outMsgBuf.resize(envelopeSize + sizeof(u_long)); + outMsgBuf.resize(sizeof(NetConFrameHeader_s) + envelopeSize); char* const scratch = outMsgBuf.data(); - // Write out frame size in network byte order. - *reinterpret_cast(scratch) = htonl(u_long(envelopeSize)); - - if (!pBase->Encode(&envelope, &scratch[sizeof(u_long)], envelopeSize)) + if (!pBase->Encode(&envelope, &scratch[sizeof(NetConFrameHeader_s)], envelopeSize)) { if (bDebug) { @@ -190,6 +187,12 @@ bool NetconShared_PackEnvelope(const CNetConBase* pBase, vector& outMsgBuf return false; } + NetConFrameHeader_s* const header = reinterpret_cast(scratch); + + // Write out magic and frame size in network byte order. + header->magic = htonl(RCON_FRAME_MAGIC); + header->length = htonl(u32(envelopeSize)); + return true; } @@ -220,10 +223,10 @@ bool NetconShared_UnpackEnvelope(const CNetConBase* pBase, const char* pMsgBuf, const size_t msgLen = envelope.data().size(); - if (msgLen > RCON_MAX_PAYLOAD_SIZE) + if (msgLen > RCON_FRAME_MAX_SIZE) { Error(eDLL_T::ENGINE, NO_ERROR, "Data in RCON message envelope is too large (%zu > %zu)\n", - msgLen, RCON_MAX_PAYLOAD_SIZE); + msgLen, RCON_FRAME_MAX_SIZE); return false; }