mirror
/
OpenTTD
mirror of https://github.com/OpenTTD/OpenTTD
1
0
Fork 0
Code Issues

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.
pull/9455/head
Patric Stout 2021-07-21 21:55:30 +02:00 committed by GitHub
parent 99d0d9be6b
commit 9cc706847c
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 12 additions and 17 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
src/network/core/tcp_game.cpp
View File

@ -48,7 +48,7 @@ NetworkRecvStatus NetworkGameSocketHandler::CloseConnection(bool error)
_networking = false; _networking = false;
ShowErrorMessage(STR_NETWORK_ERROR_LOSTCONNECTION, INVALID_STRING_ID, WL_CRITICAL); 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); return this->CloseConnection(NETWORK_RECV_STATUS_CONNECTION_LOST);

27
src/network/network_client.cpp
View File

@ -160,24 +160,19 @@ ClientNetworkGameSocketHandler::~ClientNetworkGameSocketHandler()
NetworkRecvStatus ClientNetworkGameSocketHandler::CloseConnection(NetworkRecvStatus status) NetworkRecvStatus ClientNetworkGameSocketHandler::CloseConnection(NetworkRecvStatus status)
{ {
assert(status != NETWORK_RECV_STATUS_OKAY); assert(status != NETWORK_RECV_STATUS_OKAY);
/* assert(this->sock != INVALID_SOCKET);
* 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;
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. /* 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 * This is especially needed for Windows servers as they seem to get
* the "socket is closed" message before receiving our leave message, * the "socket is closed" message before receiving our leave message,
* which would trigger the server to close the connection as well. */ * which would trigger the server to close the connection as well. */
CSleep(3 * MILLISECONDS_PER_TICK); CSleep(3 * MILLISECONDS_PER_TICK);
}
delete this; delete this;
@ -256,7 +251,7 @@ void ClientNetworkGameSocketHandler::ClientError(NetworkRecvStatus res)
/* static */ void ClientNetworkGameSocketHandler::Send() /* static */ void ClientNetworkGameSocketHandler::Send()
{ {
my_client->SendPackets(); my_client->SendPackets();
my_client->CheckConnection(); if (my_client != nullptr) my_client->CheckConnection();
} }
/** /**