From 6877b8214ac1a5ae5fdecd9f8410e433fc91ea38 Mon Sep 17 00:00:00 2001 From: ZehMatt Date: Thu, 29 Jul 2021 01:24:24 +0300 Subject: [PATCH 1/3] Fix unhandled exceptions during packet processing --- src/openrct2/network/NetworkBase.cpp | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/src/openrct2/network/NetworkBase.cpp b/src/openrct2/network/NetworkBase.cpp index 053339fafc..4bce56d75e 100644 --- a/src/openrct2/network/NetworkBase.cpp +++ b/src/openrct2/network/NetworkBase.cpp @@ -1698,15 +1698,23 @@ bool NetworkBase::ProcessConnection(NetworkConnection& connection) void NetworkBase::ProcessPacket(NetworkConnection& connection, NetworkPacket& packet) { const auto& handlerList = GetMode() == NETWORK_MODE_SERVER ? server_command_handlers : client_command_handlers; - auto it = handlerList.find(packet.GetCommand()); - if (it != handlerList.end()) + + try { - auto commandHandler = it->second; - if (connection.AuthStatus == NetworkAuth::Ok || !packet.CommandRequiresAuth()) + auto it = handlerList.find(packet.GetCommand()); + if (it != handlerList.end()) { - (this->*commandHandler)(connection, packet); + auto commandHandler = it->second; + if (connection.AuthStatus == NetworkAuth::Ok || !packet.CommandRequiresAuth()) + { + (this->*commandHandler)(connection, packet); + } } } + catch (const std::exception& ex) + { + log_verbose("Exception during packet processing: %s", ex.what()); + } packet.Clear(); } From 4f54aa5c42a6614427e62ab116a74138fa2996ea Mon Sep 17 00:00:00 2001 From: ZehMatt Date: Thu, 29 Jul 2021 02:26:02 +0300 Subject: [PATCH 2/3] Limit the count of packets processed per update --- src/openrct2/network/NetworkBase.cpp | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/openrct2/network/NetworkBase.cpp b/src/openrct2/network/NetworkBase.cpp index 4bce56d75e..84ac7ae653 100644 --- a/src/openrct2/network/NetworkBase.cpp +++ b/src/openrct2/network/NetworkBase.cpp @@ -49,6 +49,9 @@ static int32_t _pickup_peep_old_x = LOCATION_NULL; // with uint16_t and needs some spare room for other data in the packet. static constexpr uint32_t CHUNK_SIZE = 1024 * 63; +// If data is sent fast enough it would halt the entire server, process only a maximum amount. +static constexpr uint32_t MaxPacketsPerUpdate = 100; + # include "../Cheats.h" # include "../ParkImporter.h" # include "../Version.h" @@ -1654,8 +1657,11 @@ void NetworkBase::Server_Send_EVENT_PLAYER_DISCONNECTED(const char* playerName, bool NetworkBase::ProcessConnection(NetworkConnection& connection) { NetworkReadPacket packetStatus; + + uint32_t countProcessed = 0; do { + countProcessed++; packetStatus = connection.ReadPacket(); switch (packetStatus) { @@ -1681,7 +1687,7 @@ bool NetworkBase::ProcessConnection(NetworkConnection& connection) // could not read anything from socket break; } - } while (packetStatus == NetworkReadPacket::Success); + } while (packetStatus == NetworkReadPacket::Success && countProcessed < MaxPacketsPerUpdate); if (!connection.ReceivedPacketRecently()) { From dd2ffec14be3312549f84a2f4fb6b5dddc3debb4 Mon Sep 17 00:00:00 2001 From: ZehMatt Date: Thu, 29 Jul 2021 20:08:57 +0300 Subject: [PATCH 3/3] Code review changes --- src/openrct2/network/NetworkBase.cpp | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/openrct2/network/NetworkBase.cpp b/src/openrct2/network/NetworkBase.cpp index 84ac7ae653..cbacc0f88e 100644 --- a/src/openrct2/network/NetworkBase.cpp +++ b/src/openrct2/network/NetworkBase.cpp @@ -50,6 +50,7 @@ static int32_t _pickup_peep_old_x = LOCATION_NULL; static constexpr uint32_t CHUNK_SIZE = 1024 * 63; // If data is sent fast enough it would halt the entire server, process only a maximum amount. +// This limit is per connection, the current value was determined by tests with fuzzing. static constexpr uint32_t MaxPacketsPerUpdate = 100; # include "../Cheats.h" @@ -1705,22 +1706,22 @@ void NetworkBase::ProcessPacket(NetworkConnection& connection, NetworkPacket& pa { const auto& handlerList = GetMode() == NETWORK_MODE_SERVER ? server_command_handlers : client_command_handlers; - try + auto it = handlerList.find(packet.GetCommand()); + if (it != handlerList.end()) { - auto it = handlerList.find(packet.GetCommand()); - if (it != handlerList.end()) + auto commandHandler = it->second; + if (connection.AuthStatus == NetworkAuth::Ok || !packet.CommandRequiresAuth()) { - auto commandHandler = it->second; - if (connection.AuthStatus == NetworkAuth::Ok || !packet.CommandRequiresAuth()) + try { (this->*commandHandler)(connection, packet); } + catch (const std::exception& ex) + { + log_verbose("Exception during packet processing: %s", ex.what()); + } } } - catch (const std::exception& ex) - { - log_verbose("Exception during packet processing: %s", ex.what()); - } packet.Clear(); }