5 files changed, 70 insertions, 93 deletions

diff --git a/src/ClientHandle.cpp b/src/ClientHandle.cpp
index ffb968a0c..9d236e4d5 100644
--- a/src/ClientHandle.cpp
+++ b/src/ClientHandle.cpp
@@ -1981,14 +1981,7 @@ void cClientHandle::Tick(float a_Dt)
 		m_BreakProgress += m_Player->GetMiningProgressPerTick(Block);
 	}
 
-	try
-	{
-		ProcessProtocolIn();
-	}
-	catch (const std::exception & Oops)
-	{
-		Kick(Oops.what());
-	}
+	ProcessProtocolIn();
 
 	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());
+	}
 }
 
 
diff --git a/src/Protocol/ProtocolRecognizer.cpp b/src/Protocol/ProtocolRecognizer.cpp
index dc6b93b01..3af4f9654 100644
--- a/src/Protocol/ProtocolRecognizer.cpp
+++ b/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
-	{
-		// Note that a_Data is assigned to a subview containing the data to pass to m_Protocol or UnsupportedPing
+	// TODO: recover from git history
+	// Unlengthed protocol, ...
 
-		TryRecognizeProtocol(a_Client, a_Data);
-		if (m_Protocol == nullptr)
-		{
-			m_Buffer.ResetRead();
-			return;
-		}
+	// Lengthed protocol, try if it has the entire initial handshake packet:
+	if (
+		UInt32 PacketLen;
 
-		// 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());
-		};
+		// 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)
+	)
+	{
+		m_Buffer.ResetRead();
+		return;
 	}
-	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:
-	HandleIncomingData(a_Client, a_Data);
+	// Explicitly process any remaining data (already written to m_Buffer) with the new handler:
+	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)
-{
-	// 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)
+std::unique_ptr<cProtocol> cMultiVersionProtocol::TryRecognizeLengthedProtocol(cClientHandle & a_Client, const 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
-		LOGINFO("Client \"%s\" uses an unsupported protocol (lengthed, initial packet %u)",
+		// Not an initial handshake packet, we don't know how to talk to them:
+		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;
 		}
 	}
 }
diff --git a/src/Protocol/ProtocolRecognizer.h b/src/Protocol/ProtocolRecognizer.h
index b19adaefe..56d5645c0 100644
--- a/src/Protocol/ProtocolRecognizer.h
+++ b/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);
-
-	/* 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);
+	void HandleIncomingDataInOldPingResponseStage(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. */
diff --git a/src/Protocol/Protocol_1_8.cpp b/src/Protocol/Protocol_1_8.cpp
index be855678e..a4876c448 100644
--- a/src/Protocol/Protocol_1_8.cpp
+++ b/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)
 		{
diff --git a/src/UI/SlotArea.cpp b/src/UI/SlotArea.cpp
index dbdebd320..1de2a9c6a 100644
--- a/src/UI/SlotArea.cpp
+++ b/src/UI/SlotArea.cpp
@@ -2815,7 +2815,6 @@ void cSlotAreaHorse::Clicked(cPlayer & a_Player, int a_SlotNum, eClickAction a_C
 				}
 				default: break;
 			}
-
 		}
 		default: break;
 	}