From 9cc706847cc92e8408426c78c3bee575c3323d15 Mon Sep 17 00:00:00 2001 From: Patric Stout Date: Wed, 21 Jul 2021 21:55:30 +0200 Subject: [PATCH] Fix: crash when joining a server again after a TCP disconnect (#9453) "my_client" wasn't always free'd when a game ended. "my_client" keeps a reference inside the PT_NCLIENT pool. The rest of the code assumes that when you are not in a game, it can freely reset this pool. In result: several ways to trigger a use-after-free. --- src/network/core/tcp_game.cpp | 2 +- src/network/network_client.cpp | 27 +++++++++++---------------- 2 files changed, 12 insertions(+), 17 deletions(-) diff --git a/src/network/core/tcp_game.cpp b/src/network/core/tcp_game.cpp index 84fb164dee..7a1d183495 100644 --- a/src/network/core/tcp_game.cpp +++ b/src/network/core/tcp_game.cpp @@ -48,7 +48,7 @@ NetworkRecvStatus NetworkGameSocketHandler::CloseConnection(bool error) _networking = false; ShowErrorMessage(STR_NETWORK_ERROR_LOSTCONNECTION, INVALID_STRING_ID, WL_CRITICAL); - return NETWORK_RECV_STATUS_CLIENT_QUIT; + return this->CloseConnection(NETWORK_RECV_STATUS_CLIENT_QUIT); } return this->CloseConnection(NETWORK_RECV_STATUS_CONNECTION_LOST); diff --git a/src/network/network_client.cpp b/src/network/network_client.cpp index d38437cc14..db8992f3b8 100644 --- a/src/network/network_client.cpp +++ b/src/network/network_client.cpp @@ -160,24 +160,19 @@ ClientNetworkGameSocketHandler::~ClientNetworkGameSocketHandler() NetworkRecvStatus ClientNetworkGameSocketHandler::CloseConnection(NetworkRecvStatus status) { assert(status != NETWORK_RECV_STATUS_OKAY); - /* - * Sending a message just before leaving the game calls cs->SendPackets. - * This might invoke this function, which means that when we close the - * connection after cs->SendPackets we will close an already closed - * 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; + assert(this->sock != INVALID_SOCKET); - Debug(net, 3, "Closed client connection {}", this->client_id); + if (!this->HasClientQuit()) { + Debug(net, 3, "Closed client connection {}", this->client_id); - this->SendPackets(true); + this->SendPackets(true); - /* Wait a number of ticks so our leave message can reach the server. - * This is especially needed for Windows servers as they seem to get - * the "socket is closed" message before receiving our leave message, - * which would trigger the server to close the connection as well. */ - CSleep(3 * MILLISECONDS_PER_TICK); + /* Wait a number of ticks so our leave message can reach the server. + * This is especially needed for Windows servers as they seem to get + * the "socket is closed" message before receiving our leave message, + * which would trigger the server to close the connection as well. */ + CSleep(3 * MILLISECONDS_PER_TICK); + } delete this; @@ -256,7 +251,7 @@ void ClientNetworkGameSocketHandler::ClientError(NetworkRecvStatus res) /* static */ void ClientNetworkGameSocketHandler::Send() { my_client->SendPackets(); - my_client->CheckConnection(); + if (my_client != nullptr) my_client->CheckConnection(); } /**