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

@@ -21,6 +21,8 @@
 
 #include "../../safeguards.h"
 
+static std::vector<NetworkGameSocketHandler *> _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();
+}

src/network/core/tcp_game.h

@@ -169,6 +169,7 @@ class NetworkGameSocketHandler : public NetworkTCPSocketHandler {
 /* TODO: rewrite into a proper class */
 private:
 	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 */

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();
 }

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;

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();
+	this->DeferDeletion();
-	delete this;
 
 	InvalidateWindowData(WC_CLIENT_LIST, 0);