MultiVersionProtocol: fix two crashes

First one: add missing exception handler in ProcessProtocolIn

Second: remove faulty logic dealing with incomplete packets.

`a_Data = a_Data.substr(m_Buffer.GetUsedSpace() - m_Buffer.GetReadableSpace());`

was incorrect; it attempted to apply a length derived from m_Buffer to an unrelated a_Data. Its purpose was to give cProtocol the data the client sent, minus initial handshake bytes. However, we can use the knowledge that during initial handshake, there is no encryption and every byte can be written unchanged into m_Buffer, to just call cProtocol with a data length of zero. This will cause it to parse from m_Buffer - wherein we have already written everything the client sent - with no a_Data manipulation needed.

Additionally, removed UnsupportedButPingableProtocolException (use of exception as control flow) and encode this state as m_Protocol == nullptr, id est "no protocol for this unsupported version", which is then handled by cMultiVersionProtocol itself.

src/ClientHandle.cpp

@@ -1981,14 +1981,7 @@ void cClientHandle::Tick(float a_Dt)
 		m_BreakProgress += m_Player->GetMiningProgressPerTick(Block);
 	}
 
-	try
+	ProcessProtocolIn();
-	{
-		ProcessProtocolIn();
-	}
-	catch (const std::exception & Oops)
-	{
-		Kick(Oops.what());
-	}
 
 	if (IsDestroyed())
 	{
@@ -3171,7 +3164,7 @@ void cClientHandle::PacketBufferFull(void)
 {
 	// Too much data in the incoming queue, the server is probably too busy, kick the client:
 	LOGERROR("Too much data in queue for client \"%s\" @ %s, kicking them.", m_Username.c_str(), m_IPString.c_str());
-	SendDisconnect("Server busy");
+	SendDisconnect("The server is busy; please try again later.");
 }
 
 
@@ -3249,10 +3242,19 @@ void cClientHandle::ProcessProtocolIn(void)
 		std::swap(IncomingData, m_IncomingData);
 	}
 
-	if (!IncomingData.empty())
+	if (IncomingData.empty())
+	{
+		return;
+	}
+
+	try
 	{
 		m_Protocol.HandleIncomingData(*this, IncomingData);
 	}
+	catch (const std::exception & Oops)
+	{
+		Kick(Oops.what());
+	}
 }
 
 

src/Protocol/ProtocolRecognizer.cpp

@@ -25,18 +25,6 @@
 
 
 
-struct UnsupportedButPingableProtocolException : public std::runtime_error
-{
-	explicit UnsupportedButPingableProtocolException() :
-		std::runtime_error("")
-	{
-	}
-};
-
-
-
-
-
 struct TriedToJoinWithUnsupportedProtocolException : public std::runtime_error
 {
 	explicit TriedToJoinWithUnsupportedProtocolException(const std::string & a_Message) :
@@ -90,31 +78,44 @@ AString cMultiVersionProtocol::GetVersionTextFromInt(cProtocol::Version a_Protoc
 
 void cMultiVersionProtocol::HandleIncomingDataInRecognitionStage(cClientHandle & a_Client, std::string_view a_Data)
 {
-	// We read more than the handshake packet here, oh well.
+	// NOTE: If a new protocol is added or an old one is removed, adjust MCS_CLIENT_VERSIONS and MCS_PROTOCOL_VERSIONS macros in the header file
+
+	/* Write all incoming data unmodified into m_Buffer.
+	Writing everything is always okay to do:
+	1. We can be sure protocol encryption hasn't started yet since m_Protocol hasn't been called, hence no decryption needs to take place
+	2. The extra data are processed at the end of this function */
 	if (!m_Buffer.Write(a_Data.data(), a_Data.size()))
 	{
-		a_Client.Kick("Your client sent too much data; please try again later.");
+		a_Client.PacketBufferFull();
 		return;
 	}
 
-	try
+	// TODO: recover from git history
+	// Unlengthed protocol, ...
+
+	// Lengthed protocol, try if it has the entire initial handshake packet:
+	if (
+		UInt32 PacketLen;
+
+		// If not enough bytes for the packet length, keep waiting
+		!m_Buffer.ReadVarInt(PacketLen) ||
+
+		// If not enough bytes for the packet, keep waiting
+		// (More of a sanity check to make sure no one tries anything funny, since ReadXXX can wait for data themselves)
+		!m_Buffer.CanReadBytes(PacketLen)
+	)
 	{
-		// Note that a_Data is assigned to a subview containing the data to pass to m_Protocol or UnsupportedPing
+		m_Buffer.ResetRead();
-
+		return;
-		TryRecognizeProtocol(a_Client, a_Data);
-		if (m_Protocol == nullptr)
-		{
-			m_Buffer.ResetRead();
-			return;
-		}
-
-		// The protocol recogniser succesfully identified, switch mode:
-		HandleIncomingData = [this](cClientHandle &, const std::string_view a_In)
-		{
-			m_Protocol->DataReceived(m_Buffer, a_In.data(), a_In.size());
-		};
 	}
-	catch (const UnsupportedButPingableProtocolException &)
+
+	/* Figure out the client's version.
+	1. m_Protocol != nullptr: the protocol is supported and we have a handler
+	2. m_Protocol == nullptr: the protocol is unsupported, handling is a special case done by ourselves
+	3. Exception: the data sent were garbage, the client handle deals with it by disconnecting */
+	m_Protocol = TryRecognizeLengthedProtocol(a_Client, a_Data);
+
+	if (m_Protocol == nullptr)
 	{
 		// Got a server list ping for an unrecognised version,
 		// switch into responding to unknown protocols mode:
@@ -123,9 +124,17 @@ void cMultiVersionProtocol::HandleIncomingDataInRecognitionStage(cClientHandle &
 			HandleIncomingDataInOldPingResponseStage(a_Clyent, a_In);
 		};
 	}
+	else
+	{
+		// The protocol recogniser succesfully identified, switch mode:
+		HandleIncomingData = [this](cClientHandle &, const std::string_view a_In)
+		{
+			m_Protocol->DataReceived(m_Buffer, a_In.data(), a_In.size());
+		};
+	}
 
-	// Explicitly process any remaining data with the new handler:
+	// Explicitly process any remaining data (already written to m_Buffer) with the new handler:
-	HandleIncomingData(a_Client, a_Data);
+	HandleIncomingData(a_Client, {});
 }
 
 
@@ -136,7 +145,7 @@ void cMultiVersionProtocol::HandleIncomingDataInOldPingResponseStage(cClientHand
 {
 	if (!m_Buffer.Write(a_Data.data(), a_Data.size()))
 	{
-		a_Client.Kick("Server list ping failed, too much data.");
+		a_Client.PacketBufferFull();
 		return;
 	}
 
@@ -170,7 +179,7 @@ void cMultiVersionProtocol::HandleIncomingDataInOldPingResponseStage(cClientHand
 		}
 		else
 		{
-			a_Client.Kick("Server list ping failed, unrecognized packet.");
+			a_Client.PacketUnknown(PacketID);
 			return;
 		}
 
@@ -206,34 +215,7 @@ void cMultiVersionProtocol::SendDisconnect(cClientHandle & a_Client, const AStri
 
 
 
-void cMultiVersionProtocol::TryRecognizeProtocol(cClientHandle & a_Client, std::string_view & a_Data)
+std::unique_ptr<cProtocol> cMultiVersionProtocol::TryRecognizeLengthedProtocol(cClientHandle & a_Client, const std::string_view a_Data)
-{
-	// NOTE: If a new protocol is added or an old one is removed, adjust MCS_CLIENT_VERSIONS and MCS_PROTOCOL_VERSIONS macros in the header file
-
-	// Lengthed protocol, try if it has the entire initial handshake packet:
-	UInt32 PacketLen;
-	if (!m_Buffer.ReadVarInt(PacketLen))
-	{
-		// Not enough bytes for the packet length, keep waiting
-		return;
-	}
-
-	if (!m_Buffer.CanReadBytes(PacketLen))
-	{
-		// Not enough bytes for the packet, keep waiting
-		// More of a sanity check to make sure no one tries anything funny (since ReadXXX can wait for data themselves):
-		return;
-	}
-
-	m_Protocol = TryRecognizeLengthedProtocol(a_Client, a_Data);
-	ASSERT(m_Protocol != nullptr);
-}
-
-
-
-
-
-std::unique_ptr<cProtocol> cMultiVersionProtocol::TryRecognizeLengthedProtocol(cClientHandle & a_Client, std::string_view & a_Data)
 {
 	UInt32 PacketType;
 	UInt32 ProtocolVersion;
@@ -243,8 +225,8 @@ std::unique_ptr<cProtocol> cMultiVersionProtocol::TryRecognizeLengthedProtocol(c
 
 	if (!m_Buffer.ReadVarInt(PacketType) || (PacketType != 0x00))
 	{
-		// Not an initial handshake packet, we don't know how to talk to them
+		// Not an initial handshake packet, we don't know how to talk to them:
-		LOGINFO("Client \"%s\" uses an unsupported protocol (lengthed, initial packet %u)",
+		LOGD("Client \"%s\" uses an unsupported protocol (lengthed, initial packet %u)",
 			a_Client.GetIPString().c_str(), PacketType
 		);
 
@@ -277,15 +259,6 @@ std::unique_ptr<cProtocol> cMultiVersionProtocol::TryRecognizeLengthedProtocol(c
 	// TODO: this should be a protocol property, not ClientHandle:
 	a_Client.SetProtocolVersion(ProtocolVersion);
 
-	// The protocol has just been recognized, advance data start
-	// to after the handshake and leave the rest to the protocol:
-	a_Data = a_Data.substr(m_Buffer.GetUsedSpace() - m_Buffer.GetReadableSpace());
-
-	// We read more than we can handle, purge the rest:
-	[[maybe_unused]] const bool Success =
-		m_Buffer.SkipRead(m_Buffer.GetReadableSpace());
-	ASSERT(Success);
-
 	// All good, eat up the data:
 	m_Buffer.CommitRead();
 
@@ -319,7 +292,8 @@ std::unique_ptr<cProtocol> cMultiVersionProtocol::TryRecognizeLengthedProtocol(c
 				);
 			}
 
-			throw UnsupportedButPingableProtocolException();
+			// No cProtocol can handle the client:
+			return nullptr;
 		}
 	}
 }

src/Protocol/ProtocolRecognizer.h

@@ -47,22 +47,17 @@ public:
 
 private:
 
-	/** Handles data reception in a newly-created client handle that doesn't yet have known protocol.
+	/** Handles data reception in a newly-created client handle that doesn't yet have a known protocol.
 	a_Data contains a view of data that were just received.
-	Calls TryRecognizeProtocol to populate m_Protocol, and transitions to another mode depending on success. */
+	Tries to recognize a protocol, populate m_Protocol, and transitions to another mode depending on success. */
 	void HandleIncomingDataInRecognitionStage(cClientHandle & a_Client, std::string_view a_Data);
 
 	/** Handles and responds to unsupported clients sending pings. */
-	void HandleIncomingDataInOldPingResponseStage(cClientHandle & a_Client, const std::string_view a_Data);
+	void HandleIncomingDataInOldPingResponseStage(cClientHandle & a_Client, std::string_view a_Data);
-
-	/* Tries to recognize a protocol based on a_Data and m_Buffer contents.
-	a_Data is replaced with a sub-view, with handshake packet removed. */
-	void TryRecognizeProtocol(cClientHandle & a_Client, std::string_view & a_Data);
 
 	/** Tries to recognize a protocol in the lengthed family (1.7+), based on m_Buffer.
-	The packet length and type have already been read, type is 0.
 	Returns a cProtocol_XXX instance if recognized. */
-	std::unique_ptr<cProtocol> TryRecognizeLengthedProtocol(cClientHandle & a_Client, std::string_view & a_Data);
+	std::unique_ptr<cProtocol> TryRecognizeLengthedProtocol(cClientHandle & a_Client, std::string_view a_Data);
 
 	/** Sends one packet inside a cByteBuffer.
 	This is used only when handling an outdated server ping. */

src/Protocol/Protocol_1_8.cpp

@@ -177,6 +177,13 @@ void cProtocol_1_8_0::DataReceived(cByteBuffer & a_Buffer, const char * a_Data,
 {
 	if (m_IsEncrypted)
 	{
+		// An artefact of the protocol recogniser, will be removed when decryption done in-place:
+		if (a_Size == 0)
+		{
+			AddReceivedData(a_Buffer, nullptr, 0);
+			return;
+		}
+
 		std::byte Decrypted[512];
 		while (a_Size > 0)
 		{

src/UI/SlotArea.cpp

@@ -2815,7 +2815,6 @@ void cSlotAreaHorse::Clicked(cPlayer & a_Player, int a_SlotNum, eClickAction a_C
 				}
 				default: break;
 			}
-
 		}
 		default: break;
 	}