From 495db43b7281973828d801b643fdca5ffac2d37b Mon Sep 17 00:00:00 2001 From: Jonathan G Rennison Date: Fri, 16 Jun 2023 20:44:48 +0100 Subject: [PATCH] Network: Defer deletion of client and server game socket handler This fixes various use after free scenarios in error handling paths --- src/network/core/tcp_game.cpp | 16 ++++++++++++++++ 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, 40 insertions(+), 7 deletions(-) diff --git a/src/network/core/tcp_game.cpp b/src/network/core/tcp_game.cpp index 447a26ba80..68094a147e 100644 --- a/src/network/core/tcp_game.cpp +++ b/src/network/core/tcp_game.cpp @@ -21,6 +21,8 @@ #include "../../safeguards.h" +static std::vector _deferred_deletions; + static const char* _packet_game_type_names[] { "SERVER_FULL", "SERVER_BANNED", @@ -290,3 +292,17 @@ void NetworkGameSocketHandler::LogSentPacket(const Packet &pkt) PacketGameType type = (PacketGameType)pkt.GetPacketType(); DEBUG(net, 5, "[tcp/game] sent packet type %d (%s) to client %d, %s", type, GetPacketGameTypeName(type), this->client_id, this->GetDebugInfo().c_str()); } + +void NetworkGameSocketHandler::DeferDeletion() +{ + _deferred_deletions.push_back(this); + this->is_pending_deletion = true; +} + +/* static */ void NetworkGameSocketHandler::ProcessDeferredDeletions() +{ + for (NetworkGameSocketHandler *cs : _deferred_deletions) { + delete cs; + } + _deferred_deletions.clear(); +} diff --git a/src/network/core/tcp_game.h b/src/network/core/tcp_game.h index 454bfabccf..ffdc9c2966 100644 --- a/src/network/core/tcp_game.h +++ b/src/network/core/tcp_game.h @@ -168,7 +168,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: bool ignore_close = false; @@ -592,6 +593,11 @@ public: virtual std::string GetDebugInfo() const; virtual void LogSentPacket(const Packet &pkt) override; + + 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 775362056c..04f5a32491 100644 --- a/src/network/network.cpp +++ b/src/network/network.cpp @@ -663,6 +663,7 @@ void NetworkClose(bool close_admins) _network_coordinator_client.CloseAllConnections(); } + NetworkGameSocketHandler::ProcessDeferredDeletions(); TCPConnecter::KillAll(); @@ -1075,12 +1076,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) */ @@ -1092,6 +1096,7 @@ static void NetworkSend() } else { ClientNetworkGameSocketHandler::Send(); } + NetworkGameSocketHandler::ProcessDeferredDeletions(); } /** @@ -1106,6 +1111,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 b04b8e642b..a83dc38e47 100644 --- a/src/network/network_client.cpp +++ b/src/network/network_client.cpp @@ -184,6 +184,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->status == STATUS_CLOSING) return status; @@ -211,7 +213,7 @@ NetworkRecvStatus ClientNetworkGameSocketHandler::CloseConnection(NetworkRecvSta this->ReceivePackets(); } - delete this; + this->DeferDeletion(); return status; } @@ -222,6 +224,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 a01812b8e2..d4baac5a82 100644 --- a/src/network/network_server.cpp +++ b/src/network/network_server.cpp @@ -215,6 +215,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); @@ -305,7 +307,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... */ @@ -343,8 +345,7 @@ NetworkRecvStatus ServerNetworkGameSocketHandler::CloseConnection(NetworkRecvSta this->SendPackets(true); - delete this->GetInfo(); - delete this; + this->DeferDeletion(); InvalidateWindowData(WC_CLIENT_LIST, 0);