New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix #9501: [Network] crash when more than one game-info query was pending #9502
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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(); | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 */ | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the
ZeroedMemoryAllocatorreally needed here? Only theClientNetworkGameSocketHandleruses it but not theServerNetworkGameSocketHandlerso it is not something theNetworkGameSocketHandlerintrinsicly needs. Since this has only one instance variable that gets in the constructor, I think there is no use for it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ServerNetworkGameSocketHandleralso zero's the object, but this is done by the pool (Tzerois set totrue). So there tooCallocis used. In other words: it currently is consistent in zero'ing the object on creation.Of course these days we can just set the initial values of variables, and possibly get ride of this whole
ZeroedMemoryAllocator, but that might be best for another PR. For now, I suggest we leave it consistent, and zero the object for all children usingNetworkGameSocketHandler.