From 5150f3bb66d9a8c64b0bf1134a57571c4ddac5f6 Mon Sep 17 00:00:00 2001 From: Jonathan G Rennison Date: Fri, 16 Jun 2023 20:54:04 +0100 Subject: [PATCH] Fix #11016: Defer deletion of client and server game socket handlers This fixes various use after free scenarios in error handling paths --- src/network/core/tcp_game.cpp | 14 ++++++++++++++ src/network/core/tcp_game.h | 8 +++++++- src/network/network.cpp | 10 ++++++++-- src/network/network_client.cpp | 6 +++++- src/network/network_server.cpp | 7 ++++--- 5 files changed, 38 insertions(+), 7 deletions(-) diff --git a/src/network/core/tcp_game.cpp b/src/network/core/tcp_game.cpp index acbbc52562..aea4ef6825 100644 --- a/src/network/core/tcp_game.cpp +++ b/src/network/core/tcp_game.cpp @@ -20,6 +20,8 @@ #include "../../safeguards.h" +static std::vector> _deferred_deletions; + /** * Create a new socket for the game connection. * @param s The socket to connect with. @@ -199,3 +201,15 @@ NetworkRecvStatus NetworkGameSocketHandler::Receive_SERVER_MOVE(Packet *p) { ret NetworkRecvStatus NetworkGameSocketHandler::Receive_CLIENT_MOVE(Packet *p) { return this->ReceiveInvalidPacket(PACKET_CLIENT_MOVE); } NetworkRecvStatus NetworkGameSocketHandler::Receive_SERVER_COMPANY_UPDATE(Packet *p) { return this->ReceiveInvalidPacket(PACKET_SERVER_COMPANY_UPDATE); } NetworkRecvStatus NetworkGameSocketHandler::Receive_SERVER_CONFIG_UPDATE(Packet *p) { return this->ReceiveInvalidPacket(PACKET_SERVER_CONFIG_UPDATE); } + +void NetworkGameSocketHandler::DeferDeletion() +{ + _deferred_deletions.emplace_back(this); + this->is_pending_deletion = true; +} + +/* static */ void NetworkGameSocketHandler::ProcessDeferredDeletions() +{ + /* Calls deleter on all items */ + _deferred_deletions.clear(); +} diff --git a/src/network/core/tcp_game.h b/src/network/core/tcp_game.h index cd0facd781..e1372b73bc 100644 --- a/src/network/core/tcp_game.h +++ b/src/network/core/tcp_game.h @@ -154,7 +154,8 @@ public: class NetworkGameSocketHandler : public NetworkTCPSocketHandler { /* TODO: rewrite into a proper class */ private: - NetworkClientInfo *info; ///< Client info related to this socket + NetworkClientInfo *info; ///< Client info related to this socket + bool is_pending_deletion = false; ///< Whether this socket is pending deletion protected: NetworkRecvStatus ReceiveInvalidPacket(PacketGameType type); @@ -543,6 +544,11 @@ public: const char *ReceiveCommand(Packet *p, CommandPacket *cp); void SendCommand(Packet *p, const CommandPacket *cp); + + bool IsPendingDeletion() const { return this->is_pending_deletion; } + + void DeferDeletion(); + static void ProcessDeferredDeletions(); }; #endif /* NETWORK_CORE_TCP_GAME_H */ diff --git a/src/network/network.cpp b/src/network/network.cpp index c3b4974c36..f4ae3a361e 100644 --- a/src/network/network.cpp +++ b/src/network/network.cpp @@ -599,6 +599,7 @@ void NetworkClose(bool close_admins) _network_coordinator_client.CloseAllConnections(); } + NetworkGameSocketHandler::ProcessDeferredDeletions(); TCPConnecter::KillAll(); @@ -996,12 +997,15 @@ void NetworkUpdateServerGameType() */ static bool NetworkReceive() { + bool result; if (_network_server) { ServerNetworkAdminSocketHandler::Receive(); - return ServerNetworkGameSocketHandler::Receive(); + result = ServerNetworkGameSocketHandler::Receive(); } else { - return ClientNetworkGameSocketHandler::Receive(); + result = ClientNetworkGameSocketHandler::Receive(); } + NetworkGameSocketHandler::ProcessDeferredDeletions(); + return result; } /* This sends all buffered commands (if possible) */ @@ -1013,6 +1017,7 @@ static void NetworkSend() } else { ClientNetworkGameSocketHandler::Send(); } + NetworkGameSocketHandler::ProcessDeferredDeletions(); } /** @@ -1027,6 +1032,7 @@ void NetworkBackgroundLoop() TCPConnecter::CheckCallbacks(); NetworkHTTPSocketHandler::HTTPReceive(); QueryNetworkGameSocketHandler::SendReceive(); + NetworkGameSocketHandler::ProcessDeferredDeletions(); NetworkBackgroundUDPLoop(); } diff --git a/src/network/network_client.cpp b/src/network/network_client.cpp index e01daa126e..ac4da07ee9 100644 --- a/src/network/network_client.cpp +++ b/src/network/network_client.cpp @@ -160,6 +160,8 @@ ClientNetworkGameSocketHandler::~ClientNetworkGameSocketHandler() NetworkRecvStatus ClientNetworkGameSocketHandler::CloseConnection(NetworkRecvStatus status) { assert(status != NETWORK_RECV_STATUS_OKAY); + if (this->IsPendingDeletion()) return status; + assert(this->sock != INVALID_SOCKET); if (!this->HasClientQuit()) { @@ -174,7 +176,7 @@ NetworkRecvStatus ClientNetworkGameSocketHandler::CloseConnection(NetworkRecvSta CSleep(3 * MILLISECONDS_PER_TICK); } - delete this; + this->DeferDeletion(); return status; } @@ -185,6 +187,8 @@ NetworkRecvStatus ClientNetworkGameSocketHandler::CloseConnection(NetworkRecvSta */ void ClientNetworkGameSocketHandler::ClientError(NetworkRecvStatus res) { + if (this->IsPendingDeletion()) return; + /* First, send a CLIENT_ERROR to the server, so it knows we are * disconnected (and why!) */ NetworkErrorCode errorno; diff --git a/src/network/network_server.cpp b/src/network/network_server.cpp index d3cbdf9e3a..67d59bddda 100644 --- a/src/network/network_server.cpp +++ b/src/network/network_server.cpp @@ -224,6 +224,8 @@ ServerNetworkGameSocketHandler::ServerNetworkGameSocketHandler(SOCKET s) : Netwo */ ServerNetworkGameSocketHandler::~ServerNetworkGameSocketHandler() { + delete this->GetInfo(); + if (_redirect_console_to_client == this->client_id) _redirect_console_to_client = INVALID_CLIENT_ID; OrderBackup::ResetUser(this->client_id); @@ -256,7 +258,7 @@ NetworkRecvStatus ServerNetworkGameSocketHandler::CloseConnection(NetworkRecvSta * connection. This handles that case gracefully without having to make * that code any more complex or more aware of the validity of the socket. */ - if (this->sock == INVALID_SOCKET) return status; + if (this->IsPendingDeletion() || this->sock == INVALID_SOCKET) return status; if (status != NETWORK_RECV_STATUS_CLIENT_QUIT && status != NETWORK_RECV_STATUS_SERVER_ERROR && !this->HasClientQuit() && this->status >= STATUS_AUTHORIZED) { /* We did not receive a leave message from this client... */ @@ -292,8 +294,7 @@ NetworkRecvStatus ServerNetworkGameSocketHandler::CloseConnection(NetworkRecvSta this->SendPackets(true); - delete this->GetInfo(); - delete this; + this->DeferDeletion(); InvalidateWindowData(WC_CLIENT_LIST, 0);