From 802ca4e72231895e9f043a7b380c59dfbba366cd Mon Sep 17 00:00:00 2001 From: Tyler Trahan Date: Sun, 28 Nov 2021 07:16:42 -0700 Subject: [PATCH 1/4] Fix: Don't try to rename OWNER_DEITY signs in-game (#9716) --- src/signs.cpp | 12 ++++++++++++ src/signs_cmd.cpp | 2 +- src/signs_func.h | 1 + src/signs_gui.cpp | 4 ++++ 4 files changed, 18 insertions(+), 1 deletion(-) diff --git a/src/signs.cpp b/src/signs.cpp index 3e0e7a7a33..0f1d7a78f0 100644 --- a/src/signs.cpp +++ b/src/signs.cpp @@ -9,6 +9,7 @@ #include "stdafx.h" #include "landscape.h" +#include "company_func.h" #include "signs_base.h" #include "signs_func.h" #include "strings_func.h" @@ -61,3 +62,14 @@ void UpdateAllSignVirtCoords() si->UpdateVirtCoord(); } } + +/** + * Check if the current company can rename a given sign. + * @param *si The sign in question. + * @return true if the sign can be renamed, else false. + */ +bool CompanyCanRenameSign(const Sign *si) +{ + if (si->owner == OWNER_DEITY && _current_company != OWNER_DEITY && _game_mode != GM_EDITOR) return false; + return true; +} diff --git a/src/signs_cmd.cpp b/src/signs_cmd.cpp index 0dd821157e..78bbb8b4b5 100644 --- a/src/signs_cmd.cpp +++ b/src/signs_cmd.cpp @@ -79,7 +79,7 @@ CommandCost CmdRenameSign(TileIndex tile, DoCommandFlag flags, uint32 p1, uint32 { Sign *si = Sign::GetIfValid(p1); if (si == nullptr) return CMD_ERROR; - if (si->owner == OWNER_DEITY && _current_company != OWNER_DEITY && _game_mode != GM_EDITOR) return CMD_ERROR; + if (!CompanyCanRenameSign(si)) return CMD_ERROR; /* Rename the signs when empty, otherwise remove it */ if (!text.empty()) { diff --git a/src/signs_func.h b/src/signs_func.h index 55e831fdca..af677201c1 100644 --- a/src/signs_func.h +++ b/src/signs_func.h @@ -18,6 +18,7 @@ extern SignID _new_sign_id; void UpdateAllSignVirtCoords(); void PlaceProc_Sign(TileIndex tile); +bool CompanyCanRenameSign(const Sign *si); /* signs_gui.cpp */ void ShowRenameSignWindow(const Sign *si); diff --git a/src/signs_gui.cpp b/src/signs_gui.cpp index 0240a6a257..5169064625 100644 --- a/src/signs_gui.cpp +++ b/src/signs_gui.cpp @@ -565,10 +565,14 @@ static WindowDesc _query_sign_edit_desc( */ void HandleClickOnSign(const Sign *si) { + /* If we can't rename the sign, don't even open the rename GUI. */ + if (!CompanyCanRenameSign(si)) return; + if (_ctrl_pressed && (si->owner == _local_company || (si->owner == OWNER_DEITY && _game_mode == GM_EDITOR))) { RenameSign(si->index, ""); return; } + ShowRenameSignWindow(si); } From 9c36c12c85ede5a187263d3dda1ed067a6875852 Mon Sep 17 00:00:00 2001 From: Patric Stout Date: Sat, 4 Dec 2021 18:32:06 +0100 Subject: [PATCH 2/4] Codechange: ensure OnConnect() always gets called with a valid socket (#9729) This should already be the case, but now assert()s will tell us if this isn't. --- src/network/core/tcp.cpp | 2 ++ src/network/core/tcp_connect.cpp | 2 ++ src/network/network_coordinator.cpp | 2 ++ src/network/network_turn.cpp | 2 +- 4 files changed, 7 insertions(+), 1 deletion(-) diff --git a/src/network/core/tcp.cpp b/src/network/core/tcp.cpp index a2e31b53d2..3dc2a3fac8 100644 --- a/src/network/core/tcp.cpp +++ b/src/network/core/tcp.cpp @@ -217,6 +217,8 @@ Packet *NetworkTCPSocketHandler::ReceivePacket() */ bool NetworkTCPSocketHandler::CanSendReceive() { + assert(this->sock != INVALID_SOCKET); + fd_set read_fd, write_fd; struct timeval tv; diff --git a/src/network/core/tcp_connect.cpp b/src/network/core/tcp_connect.cpp index 73c6aa90d3..f9c40c2026 100644 --- a/src/network/core/tcp_connect.cpp +++ b/src/network/core/tcp_connect.cpp @@ -451,6 +451,8 @@ bool TCPServerConnecter::CheckActivity() */ void TCPServerConnecter::SetConnected(SOCKET sock) { + assert(sock != INVALID_SOCKET); + this->socket = sock; this->status = Status::CONNECTED; } diff --git a/src/network/network_coordinator.cpp b/src/network/network_coordinator.cpp index c3fa2b5ce5..a52ac638ea 100644 --- a/src/network/network_coordinator.cpp +++ b/src/network/network_coordinator.cpp @@ -566,6 +566,8 @@ void ClientNetworkCoordinatorSocketHandler::ConnectFailure(const std::string &to */ void ClientNetworkCoordinatorSocketHandler::ConnectSuccess(const std::string &token, SOCKET sock, NetworkAddress &address) { + assert(sock != INVALID_SOCKET); + /* Connecter will destroy itself. */ this->game_connecter = nullptr; diff --git a/src/network/network_turn.cpp b/src/network/network_turn.cpp index ae82f3094d..bfc4919e59 100644 --- a/src/network/network_turn.cpp +++ b/src/network/network_turn.cpp @@ -41,7 +41,7 @@ public: { this->handler->connecter = nullptr; - handler->sock = s; + this->handler->sock = s; } }; From ea4f6bb8b2a4e1298d1166febaaeebb3c9e583ed Mon Sep 17 00:00:00 2001 From: Patric Stout Date: Sat, 4 Dec 2021 20:56:05 +0100 Subject: [PATCH 3/4] Fix #9730: [Network] connections can use an invalid socket due to a race condition A race condition happens when an IPv6 connection takes more than 250ms to report an error, but does return before the IPv4 connection is established. In result, an invalid socket might be used for that connection. --- src/network/core/tcp_connect.cpp | 37 +++++++++++++++----------------- 1 file changed, 17 insertions(+), 20 deletions(-) diff --git a/src/network/core/tcp_connect.cpp b/src/network/core/tcp_connect.cpp index f9c40c2026..8ef41ebf00 100644 --- a/src/network/core/tcp_connect.cpp +++ b/src/network/core/tcp_connect.cpp @@ -363,7 +363,10 @@ bool TCPConnecter::CheckActivity() return true; } - /* Check for errors on any of the sockets. */ + /* If a socket is writeable, it is either in error-state or connected. + * Remove all sockets that are in error-state and mark the first that is + * not in error-state as the socket we will use for our connection. */ + SOCKET connected_socket = INVALID_SOCKET; for (auto it = this->sockets.begin(); it != this->sockets.end(); /* nothing */) { NetworkError socket_error = GetSocketError(*it); if (socket_error.HasError()) { @@ -371,34 +374,28 @@ bool TCPConnecter::CheckActivity() closesocket(*it); this->sock_to_address.erase(*it); it = this->sockets.erase(it); - } else { - it++; + continue; } - } - /* In case all sockets had an error, queue a new one. */ - if (this->sockets.empty()) { - if (!this->TryNextAddress()) { - /* There were no more addresses to try, so we failed. */ - this->OnFailure(); - return true; - } - return false; - } - - /* At least one socket is connected. The first one that does is the one - * we will be using, and we close all other sockets. */ - SOCKET connected_socket = INVALID_SOCKET; - for (auto it = this->sockets.begin(); it != this->sockets.end(); /* nothing */) { + /* No error but writeable means connected. */ if (connected_socket == INVALID_SOCKET && FD_ISSET(*it, &write_fd)) { connected_socket = *it; - } else { + } + + it++; + } + + /* All the writable sockets were in error state. So nothing is connected yet. */ + if (connected_socket == INVALID_SOCKET) return false; + + /* Close all sockets except the one we picked for our connection. */ + for (auto it = this->sockets.begin(); it != this->sockets.end(); /* nothing */) { + if (connected_socket != *it) { closesocket(*it); } this->sock_to_address.erase(*it); it = this->sockets.erase(it); } - assert(connected_socket != INVALID_SOCKET); Debug(net, 3, "Connected to {}", this->connection_string); if (_debug_net_level >= 5) { From ad89601c49bc5f4e996d2afea58db17fb1b01324 Mon Sep 17 00:00:00 2001 From: Rubidium Date: Sun, 5 Dec 2021 16:15:27 +0100 Subject: [PATCH 4/4] Codechange: do not use all upper case enumerators in a scoped enum --- src/industry_gui.cpp | 16 ++++++++-------- src/network/core/tcp.h | 12 ++++++------ src/network/core/tcp_connect.cpp | 28 ++++++++++++++-------------- 3 files changed, 28 insertions(+), 28 deletions(-) diff --git a/src/industry_gui.cpp b/src/industry_gui.cpp index 1b63191f39..bfdb308b66 100644 --- a/src/industry_gui.cpp +++ b/src/industry_gui.cpp @@ -1324,10 +1324,10 @@ protected: static CargoID produced_cargo_filter; enum class SorterType : uint8 { - IDW_SORT_BY_NAME, ///< Sorter type to sort by name - IDW_SORT_BY_TYPE, ///< Sorter type to sort by type - IDW_SORT_BY_PRODUCTION, ///< Sorter type to sort by production amount - IDW_SORT_BY_TRANSPORTED, ///< Sorter type to sort by transported percentage + ByName, ///< Sorter type to sort by name + ByType, ///< Sorter type to sort by type + ByProduction, ///< Sorter type to sort by production amount + ByTransported, ///< Sorter type to sort by transported percentage }; /** @@ -1552,9 +1552,9 @@ protected: } switch (static_cast(this->industries.SortType())) { - case IndustryDirectoryWindow::SorterType::IDW_SORT_BY_NAME: - case IndustryDirectoryWindow::SorterType::IDW_SORT_BY_TYPE: - case IndustryDirectoryWindow::SorterType::IDW_SORT_BY_PRODUCTION: + case IndustryDirectoryWindow::SorterType::ByName: + case IndustryDirectoryWindow::SorterType::ByType: + case IndustryDirectoryWindow::SorterType::ByProduction: /* Sort by descending production, then descending transported */ std::sort(cargos.begin(), cargos.end(), [](const CargoInfo &a, const CargoInfo &b) { if (a.production != b.production) return a.production > b.production; @@ -1562,7 +1562,7 @@ protected: }); break; - case IndustryDirectoryWindow::SorterType::IDW_SORT_BY_TRANSPORTED: + case IndustryDirectoryWindow::SorterType::ByTransported: /* Sort by descending transported, then descending production */ std::sort(cargos.begin(), cargos.end(), [](const CargoInfo &a, const CargoInfo &b) { if (a.transported != b.transported) return a.transported > b.transported; diff --git a/src/network/core/tcp.h b/src/network/core/tcp.h index 52d9cfddb3..7bce8f6c0a 100644 --- a/src/network/core/tcp.h +++ b/src/network/core/tcp.h @@ -78,15 +78,15 @@ private: * lock on the game-state. */ enum class Status { - INIT, ///< TCPConnecter is created but resolving hasn't started. - RESOLVING, ///< The hostname is being resolved (threaded). - FAILURE, ///< Resolving failed. - CONNECTING, ///< We are currently connecting. - CONNECTED, ///< The connection is established. + Init, ///< TCPConnecter is created but resolving hasn't started. + Resolving, ///< The hostname is being resolved (threaded). + Failure, ///< Resolving failed. + Connecting, ///< We are currently connecting. + Connected, ///< The connection is established. }; std::thread resolve_thread; ///< Thread used during resolving. - std::atomic status = Status::INIT; ///< The current status of the connecter. + std::atomic status = Status::Init; ///< The current status of the connecter. std::atomic killed = false; ///< Whether this connecter is marked as killed. addrinfo *ai = nullptr; ///< getaddrinfo() allocated linked-list of resolved addresses. diff --git a/src/network/core/tcp_connect.cpp b/src/network/core/tcp_connect.cpp index 8ef41ebf00..a9cc77934e 100644 --- a/src/network/core/tcp_connect.cpp +++ b/src/network/core/tcp_connect.cpp @@ -52,7 +52,7 @@ TCPServerConnecter::TCPServerConnecter(const std::string &connection_string, uin break; case SERVER_ADDRESS_INVITE_CODE: - this->status = Status::CONNECTING; + this->status = Status::Connecting; _network_coordinator_client.ConnectToServer(this->server_address.connection_string, this); break; @@ -254,14 +254,14 @@ void TCPConnecter::Resolve() if (error != 0) { Debug(net, 0, "Failed to resolve DNS for {}", this->connection_string); - this->status = Status::FAILURE; + this->status = Status::Failure; return; } this->ai = ai; this->OnResolved(ai); - this->status = Status::CONNECTING; + this->status = Status::Connecting; } /** @@ -281,11 +281,11 @@ bool TCPConnecter::CheckActivity() if (this->killed) return true; switch (this->status) { - case Status::INIT: + case Status::Init: /* Start the thread delayed, so the vtable is loaded. This allows classes * to overload functions used by Resolve() (in case threading is disabled). */ if (StartNewThread(&this->resolve_thread, "ottd:resolve", &TCPConnecter::ResolveThunk, this)) { - this->status = Status::RESOLVING; + this->status = Status::Resolving; return false; } @@ -296,18 +296,18 @@ bool TCPConnecter::CheckActivity() * connection. The rest of this function handles exactly that. */ break; - case Status::RESOLVING: + case Status::Resolving: /* Wait till Resolve() comes back with an answer (in case it runs threaded). */ return false; - case Status::FAILURE: + case Status::Failure: /* Ensure the OnFailure() is called from the game-thread instead of the * resolve-thread, as otherwise we can get into some threading issues. */ this->OnFailure(); return true; - case Status::CONNECTING: - case Status::CONNECTED: + case Status::Connecting: + case Status::Connected: break; } @@ -403,7 +403,7 @@ bool TCPConnecter::CheckActivity() } this->OnConnect(connected_socket); - this->status = Status::CONNECTED; + this->status = Status::Connected; return true; } @@ -422,11 +422,11 @@ bool TCPServerConnecter::CheckActivity() case SERVER_ADDRESS_INVITE_CODE: /* Check if a result has come in. */ switch (this->status) { - case Status::FAILURE: + case Status::Failure: this->OnFailure(); return true; - case Status::CONNECTED: + case Status::Connected: this->OnConnect(this->socket); return true; @@ -451,7 +451,7 @@ void TCPServerConnecter::SetConnected(SOCKET sock) assert(sock != INVALID_SOCKET); this->socket = sock; - this->status = Status::CONNECTED; + this->status = Status::Connected; } /** @@ -459,7 +459,7 @@ void TCPServerConnecter::SetConnected(SOCKET sock) */ void TCPServerConnecter::SetFailure() { - this->status = Status::FAILURE; + this->status = Status::Failure; } /**