From 08ebc1ab4140c465904748d0f82dcf724487d714 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=CE=B6eh=20Matt?= Date: Sun, 9 May 2021 09:56:20 +0300 Subject: [PATCH] Fix #14587: Send queued packets before disconnecting the client (#14596) * Fix #14587: Send queued packets before disconnecting the client * Log better information during network authentication --- src/openrct2/network/NetworkBase.cpp | 85 +++++++++++----------- src/openrct2/network/NetworkConnection.cpp | 10 +++ src/openrct2/network/NetworkConnection.h | 7 +- src/openrct2/network/Socket.cpp | 1 + 4 files changed, 60 insertions(+), 43 deletions(-) diff --git a/src/openrct2/network/NetworkBase.cpp b/src/openrct2/network/NetworkBase.cpp index eabe5ac4bb..2ffb098c20 100644 --- a/src/openrct2/network/NetworkBase.cpp +++ b/src/openrct2/network/NetworkBase.cpp @@ -504,12 +504,12 @@ void NetworkBase::UpdateServer() for (auto& connection : client_connection_list) { // This can be called multiple times before the connection is removed. - if (connection->IsDisconnected) + if (!connection->IsValid()) continue; if (!ProcessConnection(*connection)) { - connection->IsDisconnected = true; + connection->Disconnect(); } else { @@ -716,12 +716,6 @@ void NetworkBase::SendPacketToClients(const NetworkPacket& packet, bool front, b { for (auto& client_connection : client_connection_list) { - if (client_connection->IsDisconnected) - { - // Client will be removed at the end of the tick, don't bother. - continue; - } - if (gameCmd) { // If marked as game command we can not send the packet to connections that are not fully connected. @@ -826,7 +820,7 @@ void NetworkBase::KickPlayer(int32_t playerId) char str_disconnect_msg[256]; format_string(str_disconnect_msg, 256, STR_MULTIPLAYER_KICKED_REASON, nullptr); Server_Send_SETDISCONNECTMSG(*client_connection, str_disconnect_msg); - client_connection->Socket->Disconnect(); + client_connection->Disconnect(); break; } } @@ -841,7 +835,7 @@ void NetworkBase::ServerClientDisconnected() { if (GetMode() == NETWORK_MODE_CLIENT) { - _serverConnection->Socket->Disconnect(); + _serverConnection->Disconnect(); } } @@ -1356,7 +1350,7 @@ void NetworkBase::Server_Send_AUTH(NetworkConnection& connection) connection.QueuePacket(std::move(packet)); if (connection.AuthStatus != NetworkAuth::Ok && connection.AuthStatus != NetworkAuth::RequirePassword) { - connection.Socket->Disconnect(); + connection.Disconnect(); } } @@ -1382,7 +1376,7 @@ void NetworkBase::Server_Send_MAP(NetworkConnection* connection) if (connection) { connection->SetLastDisconnectReason(STR_MULTIPLAYER_CONNECTION_CLOSED); - connection->Socket->Disconnect(); + connection->Disconnect(); } return; } @@ -1461,7 +1455,7 @@ void NetworkBase::Server_Send_CHAT(const char* text, const std::vector& for (auto playerId : playerIds) { auto conn = GetPlayerConnection(playerId); - if (conn != nullptr && !conn->IsDisconnected) + if (conn != nullptr) { conn->QueuePacket(packet); } @@ -1674,7 +1668,7 @@ bool NetworkBase::ProcessConnection(NetworkConnection& connection) case NetworkReadPacket::Success: // done reading in packet ProcessPacket(connection, connection.InboundPacket); - if (connection.Socket == nullptr) + if (!connection.IsValid()) { return false; } @@ -1688,8 +1682,6 @@ bool NetworkBase::ProcessConnection(NetworkConnection& connection) } } while (packetStatus == NetworkReadPacket::Success); - connection.SendQueuedPackets(); - if (!connection.ReceivedPacketRecently()) { if (!connection.GetLastDisconnectReason()) @@ -1920,17 +1912,21 @@ void NetworkBase::ProcessDisconnectedClients() for (auto it = client_connection_list.begin(); it != client_connection_list.end();) { auto& connection = *it; - if (connection->IsDisconnected) - { - ServerClientDisconnected(connection); - RemovePlayer(connection); - it = client_connection_list.erase(it); - } - else + if (!connection->ShouldDisconnect) { it++; + continue; } + + // Make sure to send all remaining packets out before disconnecting. + connection->SendQueuedPackets(); + connection->Socket->Disconnect(); + + ServerClientDisconnected(connection); + RemovePlayer(connection); + + it = client_connection_list.erase(it); } } @@ -2133,7 +2129,7 @@ void NetworkBase::Client_Handle_TOKEN(NetworkConnection& connection, NetworkPack { log_error("Failed to load key %s", keyPath); connection.SetLastDisconnectReason(STR_MULTIPLAYER_VERIFICATION_FAILURE); - connection.Socket->Disconnect(); + connection.Disconnect(); return; } @@ -2150,7 +2146,7 @@ void NetworkBase::Client_Handle_TOKEN(NetworkConnection& connection, NetworkPack { log_error("Failed to sign server's challenge."); connection.SetLastDisconnectReason(STR_MULTIPLAYER_VERIFICATION_FAILURE); - connection.Socket->Disconnect(); + connection.Disconnect(); return; } // Don't keep private key in memory. There's no need and it may get leaked @@ -2221,37 +2217,37 @@ void NetworkBase::Client_Handle_AUTH(NetworkConnection& connection, NetworkPacke break; case NetworkAuth::BadName: connection.SetLastDisconnectReason(STR_MULTIPLAYER_BAD_PLAYER_NAME); - connection.Socket->Disconnect(); + connection.Disconnect(); break; case NetworkAuth::BadVersion: { const char* version = packet.ReadString(); connection.SetLastDisconnectReason(STR_MULTIPLAYER_INCORRECT_SOFTWARE_VERSION, &version); - connection.Socket->Disconnect(); + connection.Disconnect(); break; } case NetworkAuth::BadPassword: connection.SetLastDisconnectReason(STR_MULTIPLAYER_BAD_PASSWORD); - connection.Socket->Disconnect(); + connection.Disconnect(); break; case NetworkAuth::VerificationFailure: connection.SetLastDisconnectReason(STR_MULTIPLAYER_VERIFICATION_FAILURE); - connection.Socket->Disconnect(); + connection.Disconnect(); break; case NetworkAuth::Full: connection.SetLastDisconnectReason(STR_MULTIPLAYER_SERVER_FULL); - connection.Socket->Disconnect(); + connection.Disconnect(); break; case NetworkAuth::RequirePassword: context_open_window_view(WV_NETWORK_PASSWORD); break; case NetworkAuth::UnknownKeyDisallowed: connection.SetLastDisconnectReason(STR_MULTIPLAYER_UNKNOWN_KEY_DISALLOWED); - connection.Socket->Disconnect(); + connection.Disconnect(); break; default: connection.SetLastDisconnectReason(STR_MULTIPLAYER_RECEIVED_INVALID_DATA); - connection.Socket->Disconnect(); + connection.Disconnect(); break; } } @@ -2311,7 +2307,7 @@ void NetworkBase::Client_Handle_OBJECTS_LIST(NetworkConnection& connection, Netw if (totalObjects > OBJECT_ENTRY_COUNT) { connection.SetLastDisconnectReason(STR_MULTIPLAYER_SERVER_INVALID_REQUEST); - connection.Socket->Disconnect(); + connection.Disconnect(); log_warning("Server sent invalid amount of objects"); return; } @@ -2460,7 +2456,7 @@ void NetworkBase::Server_Handle_MAPREQUEST(NetworkConnection& connection, Networ if (size > OBJECT_ENTRY_COUNT) { connection.SetLastDisconnectReason(STR_MULTIPLAYER_CLIENT_INVALID_REQUEST); - connection.Socket->Disconnect(); + connection.Disconnect(); std::string playerName = "(unknown)"; if (connection.Player) { @@ -2500,6 +2496,8 @@ void NetworkBase::Server_Handle_AUTH(NetworkConnection& connection, NetworkPacke { if (connection.AuthStatus != NetworkAuth::Ok) { + const char* hostName = connection.Socket->GetHostName(); + const char* gameversion = packet.ReadString(); const char* name = packet.ReadString(); const char* password = packet.ReadString(); @@ -2535,10 +2533,10 @@ void NetworkBase::Server_Handle_AUTH(NetworkConnection& connection, NetworkPacke const std::string hash = connection.Key.PublicKeyHash(); if (verified) { - log_verbose("Signature verification ok. Hash %s", hash.c_str()); + log_verbose("Connection %s: Signature verification ok. Hash %s", hostName, hash.c_str()); if (gConfigNetwork.known_keys_only && _userManager.GetUserByHash(hash) == nullptr) { - log_verbose("Hash %s, not known", hash.c_str()); + log_verbose("Connection %s: Hash %s, not known", hostName, hash.c_str()); connection.AuthStatus = NetworkAuth::UnknownKeyDisallowed; } else @@ -2549,13 +2547,13 @@ void NetworkBase::Server_Handle_AUTH(NetworkConnection& connection, NetworkPacke else { connection.AuthStatus = NetworkAuth::VerificationFailure; - log_verbose("Signature verification failed!"); + log_verbose("Connection %s: Signature verification failed!", hostName); } } catch (const std::exception&) { connection.AuthStatus = NetworkAuth::VerificationFailure; - log_verbose("Signature verification failed, invalid data!"); + log_verbose("Connection %s: Signature verification failed, invalid data!", hostName); } } @@ -2568,26 +2566,31 @@ void NetworkBase::Server_Handle_AUTH(NetworkConnection& connection, NetworkPacke if (!gameversion || network_get_version() != gameversion) { connection.AuthStatus = NetworkAuth::BadVersion; + log_info("Connection %s: Bad version.", hostName); } else if (!name) { connection.AuthStatus = NetworkAuth::BadName; + log_info("Connection %s: Bad name.", connection.Socket->GetHostName()); } else if (!passwordless) { if ((!password || strlen(password) == 0) && !_password.empty()) { connection.AuthStatus = NetworkAuth::RequirePassword; + log_info("Connection %s: Requires password.", hostName); } else if (password && _password != password) { connection.AuthStatus = NetworkAuth::BadPassword; + log_info("Connection %s: Bad password.", hostName); } } if (static_cast(gConfigNetwork.maxplayers) <= player_list.size()) { connection.AuthStatus = NetworkAuth::Full; + log_info("Connection %s: Server is full.", hostName); } else if (connection.AuthStatus == NetworkAuth::Verified) { @@ -2600,12 +2603,10 @@ void NetworkBase::Server_Handle_AUTH(NetworkConnection& connection, NetworkPacke else { connection.AuthStatus = NetworkAuth::VerificationFailure; + log_info("Connection %s: Denied by plugin.", hostName); } } - else if (connection.AuthStatus != NetworkAuth::RequirePassword) - { - log_error("Unknown failure (%d) while authenticating client", connection.AuthStatus); - } + Server_Send_AUTH(connection); } } diff --git a/src/openrct2/network/NetworkConnection.cpp b/src/openrct2/network/NetworkConnection.cpp index fe6087a6c1..ffb0e46e1d 100644 --- a/src/openrct2/network/NetworkConnection.cpp +++ b/src/openrct2/network/NetworkConnection.cpp @@ -155,6 +155,16 @@ void NetworkConnection::QueuePacket(NetworkPacket&& packet, bool front) } } +void NetworkConnection::Disconnect() +{ + ShouldDisconnect = true; +} + +bool NetworkConnection::IsValid() const +{ + return !ShouldDisconnect && Socket->GetStatus() == SocketStatus::Connected; +} + void NetworkConnection::SendQueuedPackets() { while (!_outboundPackets.empty() && SendPacket(_outboundPackets.front())) diff --git a/src/openrct2/network/NetworkConnection.h b/src/openrct2/network/NetworkConnection.h index 293f01f5db..d5a94d5056 100644 --- a/src/openrct2/network/NetworkConnection.h +++ b/src/openrct2/network/NetworkConnection.h @@ -35,7 +35,7 @@ public: NetworkKey Key; std::vector Challenge; std::vector RequestedObjects; - bool IsDisconnected = false; + bool ShouldDisconnect = false; NetworkConnection(); ~NetworkConnection(); @@ -48,6 +48,11 @@ public: return QueuePacket(std::move(copy), front); } + // This will not immediately disconnect the client. The disconnect + // will happen post-tick. + void Disconnect(); + + bool IsValid() const; void SendQueuedPackets(); void ResetLastPacketTime(); bool ReceivedPacketRecently(); diff --git a/src/openrct2/network/Socket.cpp b/src/openrct2/network/Socket.cpp index 55035a77d4..2bf4d8dd30 100644 --- a/src/openrct2/network/Socket.cpp +++ b/src/openrct2/network/Socket.cpp @@ -541,6 +541,7 @@ public: { shutdown(_socket, SHUT_RDWR); } + _status = SocketStatus::Closed; } size_t SendData(const void* buffer, size_t size) override