Fix #9501: [Network] crash when more than one game-info query was pending (#9502)

src/network/CMakeLists.txt

@@ -22,6 +22,8 @@ add_files(
     network_gui.cpp
     network_gui.h
     network_internal.h
+    network_query.cpp
+    network_query.h
     network_server.cpp
     network_server.h
     network_stun.cpp

src/network/network.cpp

@@ -14,6 +14,7 @@
 #include "../date_func.h"
 #include "network_admin.h"
 #include "network_client.h"
+#include "network_query.h"
 #include "network_server.h"
 #include "network_content.h"
 #include "network_udp.h"
@@ -638,9 +639,7 @@ public:
 
 	void OnConnect(SOCKET s) override
 	{
-		_networking = true;
+		QueryNetworkGameSocketHandler::QueryServer(s, this->connection_string);
-		new ClientNetworkGameSocketHandler(s, this->connection_string);
-		MyClient::SendInformationQuery();
 	}
 };
 
@@ -652,8 +651,6 @@ void NetworkQueryServer(const std::string &connection_string)
 {
 	if (!_network_available) return;
 
-	NetworkInitialize();
-
 	new TCPQueryConnecter(connection_string);
 }
 
@@ -1020,6 +1017,7 @@ void NetworkBackgroundLoop()
 	_network_coordinator_client.SendReceive();
 	TCPConnecter::CheckCallbacks();
 	NetworkHTTPSocketHandler::HTTPReceive();
+	QueryNetworkGameSocketHandler::SendReceive();
 
 	NetworkBackgroundUDPLoop();
 }

src/network/network_client.cpp

@@ -23,7 +23,6 @@
 #include "../gfx_func.h"
 #include "../error.h"
 #include "../rev.h"
-#include "core/game_info.h"
 #include "network.h"
 #include "network_base.h"
 #include "network_client.h"
@@ -328,17 +327,6 @@ static_assert(NETWORK_SERVER_ID_LENGTH == 16 * 2 + 1);
  *   DEF_CLIENT_SEND_COMMAND has no parameters
  ************/
 
-/**
- * Query the server for server information.
- */
-NetworkRecvStatus ClientNetworkGameSocketHandler::SendInformationQuery()
-{
-	my_client->status = STATUS_GAME_INFO;
-	my_client->SendPacket(new Packet(PACKET_CLIENT_GAME_INFO));
-
-	return NETWORK_RECV_STATUS_OKAY;
-}
-
 /** Tell the server we would like to join. */
 NetworkRecvStatus ClientNetworkGameSocketHandler::SendJoin()
 {
@@ -557,26 +545,6 @@ NetworkRecvStatus ClientNetworkGameSocketHandler::Receive_SERVER_BANNED(Packet *
 	return NETWORK_RECV_STATUS_SERVER_BANNED;
 }
 
-NetworkRecvStatus ClientNetworkGameSocketHandler::Receive_SERVER_GAME_INFO(Packet *p)
-{
-	if (this->status != STATUS_GAME_INFO) return NETWORK_RECV_STATUS_MALFORMED_PACKET;
-
-	NetworkGameList *item = NetworkGameListAddItem(this->connection_string);
-
-	/* Clear any existing GRFConfig chain. */
-	ClearGRFConfigList(&item->info.grfconfig);
-	/* Retrieve the NetworkGameInfo from the packet. */
-	DeserializeNetworkGameInfo(p, &item->info);
-	/* Check for compatability with the client. */
-	CheckGameCompatibility(item->info);
-	/* Ensure we consider the server online. */
-	item->online = true;
-
-	UpdateNetworkGameWindow();
-
-	return NETWORK_RECV_STATUS_CLOSE_QUERY;
-}
-
 /* This packet contains info about the client (playas and name)
  *  as client we save this in NetworkClientInfo, linked via 'client_id'
  *  which is always an unique number on a server. */
@@ -665,15 +633,6 @@ NetworkRecvStatus ClientNetworkGameSocketHandler::Receive_SERVER_ERROR(Packet *p
 
 	NetworkErrorCode error = (NetworkErrorCode)p->Recv_uint8();
 
-	/* If we query a server that is 1.11.1 or older, we get an
-	 * NETWORK_ERROR_NOT_EXPECTED on requesting the game info. Show a special
-	 * error popup in that case.
-	 */
-	if (error == NETWORK_ERROR_NOT_EXPECTED && this->status == STATUS_GAME_INFO) {
-		ShowErrorMessage(STR_NETWORK_ERROR_SERVER_TOO_OLD, INVALID_STRING_ID, WL_CRITICAL);
-		return NETWORK_RECV_STATUS_CLOSE_QUERY;
-	}
-
 	StringID err = STR_NETWORK_ERROR_LOSTCONNECTION;
 	if (error < (ptrdiff_t)lengthof(network_error_strings)) err = network_error_strings[error];
 	/* In case of kicking a client, we assume there is a kick message in the packet if we can read one byte */

src/network/network_client.h

@@ -22,7 +22,6 @@ private:
 	/** Status of the connection with the server. */
 	enum ServerStatus {
 		STATUS_INACTIVE,      ///< The client is not connected nor active.
-		STATUS_GAME_INFO,     ///< We are trying to get the game information.
 		STATUS_JOIN,          ///< We are trying to join a server.
 		STATUS_NEWGRFS_CHECK, ///< Last action was checking NewGRFs.
 		STATUS_AUTH_GAME,     ///< Last action was requesting game (server) password.
@@ -44,7 +43,6 @@ protected:
 	NetworkRecvStatus Receive_SERVER_FULL(Packet *p) override;
 	NetworkRecvStatus Receive_SERVER_BANNED(Packet *p) override;
 	NetworkRecvStatus Receive_SERVER_ERROR(Packet *p) override;
-	NetworkRecvStatus Receive_SERVER_GAME_INFO(Packet *p) override;
 	NetworkRecvStatus Receive_SERVER_CLIENT_INFO(Packet *p) override;
 	NetworkRecvStatus Receive_SERVER_NEED_GAME_PASSWORD(Packet *p) override;
 	NetworkRecvStatus Receive_SERVER_NEED_COMPANY_PASSWORD(Packet *p) override;
@@ -80,8 +78,6 @@ public:
 	NetworkRecvStatus CloseConnection(NetworkRecvStatus status) override;
 	void ClientError(NetworkRecvStatus res);
 
-	static NetworkRecvStatus SendInformationQuery();
-
 	static NetworkRecvStatus SendJoin();
 	static NetworkRecvStatus SendCommand(const CommandPacket *cp);
 	static NetworkRecvStatus SendError(NetworkErrorCode errorno);

src/network/network_query.cpp

@@ -0,0 +1,145 @@
+/*
+ * This file is part of OpenTTD.
+ * OpenTTD is free software; you can redistribute it and/or modify it under the terms of the GNU General Public License as published by the Free Software Foundation, version 2.
+ * OpenTTD is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
+ * See the GNU General Public License for more details. You should have received a copy of the GNU General Public License along with OpenTTD. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+/** @file network_query.cpp Query part of the network protocol. */
+
+#include "../stdafx.h"
+#include "core/game_info.h"
+#include "network_query.h"
+#include "network_gamelist.h"
+#include "../error.h"
+
+#include "table/strings.h"
+
+#include "../safeguards.h"
+
+std::vector<std::unique_ptr<QueryNetworkGameSocketHandler>> QueryNetworkGameSocketHandler::queries = {};
+
+NetworkRecvStatus QueryNetworkGameSocketHandler::CloseConnection(NetworkRecvStatus status)
+{
+	assert(status != NETWORK_RECV_STATUS_OKAY);
+	assert(this->sock != INVALID_SOCKET);
+
+	return status;
+}
+
+/**
+ * Check the connection's state, i.e. is the connection still up?
+ */
+bool QueryNetworkGameSocketHandler::CheckConnection()
+{
+	std::chrono::steady_clock::duration lag = std::chrono::steady_clock::now() - this->last_packet;
+
+	/* If there was no response in 5 seconds, terminate the query. */
+	if (lag > std::chrono::seconds(5)) {
+		this->CloseConnection(NETWORK_RECV_STATUS_CONNECTION_LOST);
+		return false;
+	}
+
+	return true;
+}
+
+/**
+ * Check whether we received/can send some data from/to the server and
+ * when that's the case handle it appropriately.
+ * @return true when everything went okay.
+ */
+bool QueryNetworkGameSocketHandler::Receive()
+{
+	if (this->CanSendReceive()) {
+		NetworkRecvStatus res = this->ReceivePackets();
+		if (res != NETWORK_RECV_STATUS_OKAY) {
+			this->CloseConnection(res);
+			return false;
+		}
+	}
+	return true;
+}
+
+/** Send the packets of this socket handler. */
+void QueryNetworkGameSocketHandler::Send()
+{
+	this->SendPackets();
+}
+
+/**
+ * Query the server for server information.
+ */
+NetworkRecvStatus QueryNetworkGameSocketHandler::SendGameInfo()
+{
+	this->SendPacket(new Packet(PACKET_CLIENT_GAME_INFO));
+	return NETWORK_RECV_STATUS_OKAY;
+}
+
+NetworkRecvStatus QueryNetworkGameSocketHandler::Receive_SERVER_FULL(Packet *p)
+{
+	/* We try to join a server which is full */
+	ShowErrorMessage(STR_NETWORK_ERROR_SERVER_FULL, INVALID_STRING_ID, WL_CRITICAL);
+	return NETWORK_RECV_STATUS_SERVER_FULL;
+}
+
+NetworkRecvStatus QueryNetworkGameSocketHandler::Receive_SERVER_BANNED(Packet *p)
+{
+	/* We try to join a server where we are banned */
+	ShowErrorMessage(STR_NETWORK_ERROR_SERVER_BANNED, INVALID_STRING_ID, WL_CRITICAL);
+	return NETWORK_RECV_STATUS_SERVER_BANNED;
+}
+
+NetworkRecvStatus QueryNetworkGameSocketHandler::Receive_SERVER_GAME_INFO(Packet *p)
+{
+	NetworkGameList *item = NetworkGameListAddItem(this->connection_string);
+
+	/* Clear any existing GRFConfig chain. */
+	ClearGRFConfigList(&item->info.grfconfig);
+	/* Retrieve the NetworkGameInfo from the packet. */
+	DeserializeNetworkGameInfo(p, &item->info);
+	/* Check for compatability with the client. */
+	CheckGameCompatibility(item->info);
+	/* Ensure we consider the server online. */
+	item->online = true;
+
+	UpdateNetworkGameWindow();
+
+	return NETWORK_RECV_STATUS_CLOSE_QUERY;
+}
+
+NetworkRecvStatus QueryNetworkGameSocketHandler::Receive_SERVER_ERROR(Packet *p)
+{
+	NetworkErrorCode error = (NetworkErrorCode)p->Recv_uint8();
+
+	/* If we query a server that is 1.11.1 or older, we get an
+	 * NETWORK_ERROR_NOT_EXPECTED on requesting the game info. Show a special
+	 * error popup in that case.
+	 */
+	if (error == NETWORK_ERROR_NOT_EXPECTED) {
+		ShowErrorMessage(STR_NETWORK_ERROR_SERVER_TOO_OLD, INVALID_STRING_ID, WL_CRITICAL);
+		return NETWORK_RECV_STATUS_CLOSE_QUERY;
+	}
+
+	ShowErrorMessage(STR_NETWORK_ERROR_LOSTCONNECTION, INVALID_STRING_ID, WL_CRITICAL);
+	return NETWORK_RECV_STATUS_SERVER_ERROR;
+}
+
+/**
+ * Check if any query needs to send or receive.
+ */
+/* static */ void QueryNetworkGameSocketHandler::SendReceive()
+{
+	for (auto it = QueryNetworkGameSocketHandler::queries.begin(); it != QueryNetworkGameSocketHandler::queries.end(); /* nothing */) {
+		if (!(*it)->Receive()) {
+			it = QueryNetworkGameSocketHandler::queries.erase(it);
+		} else if (!(*it)->CheckConnection()) {
+			it = QueryNetworkGameSocketHandler::queries.erase(it);
+		} else {
+			it++;
+		}
+	}
+
+	for (auto &query : QueryNetworkGameSocketHandler::queries) {
+		query->Send();
+	}
+}

src/network/network_query.h

@@ -0,0 +1,59 @@
+/*
+ * This file is part of OpenTTD.
+ * OpenTTD is free software; you can redistribute it and/or modify it under the terms of the GNU General Public License as published by the Free Software Foundation, version 2.
+ * OpenTTD is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
+ * See the GNU General Public License for more details. You should have received a copy of the GNU General Public License along with OpenTTD. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+/** @file network_query.h Query part of the network protocol. */
+
+#ifndef NETWORK_QUERY_H
+#define NETWORK_QUERY_H
+
+#include "network_internal.h"
+
+/** Class for handling the client side of quering a game server. */
+class QueryNetworkGameSocketHandler : public ZeroedMemoryAllocator, public NetworkGameSocketHandler {
+private:
+	static std::vector<std::unique_ptr<QueryNetworkGameSocketHandler>> queries; ///< Pending queries.
+	std::string connection_string; ///< Address we are connected to.
+
+protected:
+	NetworkRecvStatus Receive_SERVER_FULL(Packet *p) override;
+	NetworkRecvStatus Receive_SERVER_BANNED(Packet *p) override;
+	NetworkRecvStatus Receive_SERVER_ERROR(Packet *p) override;
+	NetworkRecvStatus Receive_SERVER_GAME_INFO(Packet *p) override;
+
+	NetworkRecvStatus SendGameInfo();
+
+	bool CheckConnection();
+	void Send();
+	bool Receive();
+
+public:
+	/**
+	 * Create a new socket for the client side of quering game server.
+	 * @param s The socket to connect with.
+	 * @param connection_string The connection string of the server.
+	 */
+	QueryNetworkGameSocketHandler(SOCKET s, const std::string &connection_string) : NetworkGameSocketHandler(s), connection_string(connection_string) {}
+
+	/**
+	 * Start to query a server based on an open socket.
+	 * @param s The socket to connect with.
+	 * @param connection_string The connection string of the server.
+	 */
+	static void QueryServer(SOCKET s, const std::string &connection_string)
+	{
+		auto query = std::make_unique<QueryNetworkGameSocketHandler>(s, connection_string);
+		query->SendGameInfo();
+
+		QueryNetworkGameSocketHandler::queries.push_back(std::move(query));
+	}
+
+	static void SendReceive();
+
+	NetworkRecvStatus CloseConnection(NetworkRecvStatus status) override;
+};
+
+#endif /* NETWORK_QUERY_H */