From 49ef21d6505c76fb26d50c99934e801ba6016049 Mon Sep 17 00:00:00 2001 From: Tiger Wang Date: Sun, 17 Jan 2021 15:13:32 +0000 Subject: 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 | 22 ++++--- src/Protocol/ProtocolRecognizer.cpp | 120 ++++++++++++++---------------------- src/Protocol/ProtocolRecognizer.h | 13 ++-- src/Protocol/Protocol_1_8.cpp | 7 +++ src/UI/SlotArea.cpp | 1 - 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 cMultiVersionProtocol::TryRecognizeLengthedProtocol(cClientHandle & a_Client, std::string_view & a_Data) +std::unique_ptr cMultiVersionProtocol::TryRecognizeLengthedProtocol(cClientHandle & a_Client, const std::string_view a_Data) { UInt32 PacketType; UInt32 ProtocolVersion; @@ -243,8 +225,8 @@ std::unique_ptr 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 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 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 TryRecognizeLengthedProtocol(cClientHandle & a_Client, std::string_view & a_Data); + std::unique_ptr 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; } -- cgit v1.2.3