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

@@ -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<uint8_t> m_RecvBuffer;
+	NetConFrameHeader_s m_FrameHeader; // Current frame header.
+	vector<byte> m_RecvBuffer;
 
 	CConnectedNetConsoleData(SocketHandle_t hSocket = -1)
 	{
@@ -56,15 +58,11 @@ public:
 /* PACKET FORMAT **********************************
 
 REQUEST:
-  int requestID;
+  NetConFrameHeader_s header;
-  int ServerDataRequestType_t;
+  byte* data;
-  NullTerminatedString (variable or command)
-  NullTerminatedString (value)
 
 RESPONSE:
-  int requestID;
+  NetConFrameHeader_s header;
-  int ServerDataResponseType_t;
+  byte* data;
-  NullTerminatedString (variable)
-  NullTerminatedString (value)
 
 ***************************************************/

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, 
+bool CNetConBase::ProcessBuffer(CConnectedNetConsoleData& data, const char* pRecvBuf, int nRecvLen, const int nMaxLen)
-	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)
+			const int bytesToCopy = Min(nRecvLen, data.m_nPayloadLen - data.m_nPayloadRead);
-			{
+			memcpy(&data.m_RecvBuffer[data.m_nPayloadRead], pRecvBuf, bytesToCopy);
-				data.m_RecvBuffer[data.m_nPayloadRead++] = *pRecvBuf;
+
+			data.m_nPayloadRead += bytesToCopy;
+
+			pRecvBuf += bytesToCopy;
+			nRecvLen -= bytesToCopy;
 
-				pRecvBuf++;
-				nRecvLen--;
-			}
 			if (data.m_nPayloadRead == data.m_nPayloadLen)
 			{
-				if (!ProcessMessage(
+				if (!ProcessMessage(reinterpret_cast<const char*>(data.m_RecvBuffer.data()), data.m_nPayloadLen))
-					reinterpret_cast<const char*>(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<char*>(&data.m_FrameHeader) + data.m_nPayloadRead, pRecvBuf, bytesToCopy);
 
-			pRecvBuf++;
+			data.m_nPayloadRead += bytesToCopy;
-			nRecvLen--;
-		}
-		else // Build prefix.
-		{
-			u_long* const pPrefix = reinterpret_cast<u_long*>(&prefix[0]);
 
-			data.m_nPayloadLen = int(ntohl(*pPrefix));
+			pRecvBuf += bytesToCopy;
-			data.m_nPayloadRead = 0;
+			nRecvLen -= bytesToCopy;
 
-			*pPrefix = 0;
+			if (data.m_nPayloadRead == sizeof(NetConFrameHeader_s))
-
-			if (!data.m_bAuthorized && nMaxLen > -1)
 			{
-				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)
+				if ((!data.m_bAuthorized && nMaxLen > -1 && header.length > (u32)nMaxLen) ||
-			{
+					header.length > RCON_FRAME_MAX_SIZE)
-				Error(eDLL_T::ENGINE, NO_ERROR, "RCON Cmd: sync error (%d)\n", data.m_nPayloadLen);
+				{
-				Disconnect("desync"); // Out of sync (irrecoverable).
+					Disconnect("overflow");
+					return false;
+				}
 
-				return false;
+				data.m_nPayloadLen = header.length;
-			}
+				data.m_nPayloadRead = 0;
-			else
+
-			{
+				data.m_RecvBuffer.resize(header.length);
-				data.m_RecvBuffer.resize(data.m_nPayloadLen);
 			}
 		}
 	}
@@ -338,7 +352,7 @@ void CNetConBase::Recv(CConnectedNetConsoleData& data, const int nMaxLen)
 			break;
 		}
 
-		nReadLen -= nRecvLen; // Process what we've got.
+		nReadLen -= static_cast<u_long>(nRecvLen); // Process what we've got.
 
 		if (!ProcessBuffer(data, szRecvBuf, nRecvLen, nMaxLen))
 			break;

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:

src/engine/shared/shared_rcon.cpp

@@ -174,13 +174,10 @@ bool NetconShared_PackEnvelope(const CNetConBase* pBase, vector<char>& 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.
+	if (!pBase->Encode(&envelope, &scratch[sizeof(NetConFrameHeader_s)], envelopeSize))
-	*reinterpret_cast<u_long*>(scratch) = htonl(u_long(envelopeSize));
-
-	if (!pBase->Encode(&envelope, &scratch[sizeof(u_long)], envelopeSize))
 	{
 		if (bDebug)
 		{
@@ -190,6 +187,12 @@ bool NetconShared_PackEnvelope(const CNetConBase* pBase, vector<char>& outMsgBuf
 		return false;
 	}
 
+	NetConFrameHeader_s* const header = reinterpret_cast<NetConFrameHeader_s*>(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;
 	}