cpp/openttd-patchpack/source Changeset - r24274:24301225307d

Changeset - r24274:24301225307d

Parent rev.

Child rev.

[Not reviewed]

master

0 6 0

Jonathan G Rennison - 4 years ago 2020-05-06 22:23:03
j.g.rennison@gmail.com

Fix: Thread unsafe use of NetworkAddress::GetAddressAsString

Remove static buffer form of NetworkAddress::GetAddressAsString.
This is used in multiple threads concurrently, and is not thread-safe.

Replace it with a form returning std::string.

6 files changed with 28 insertions and 23 deletions:

src/network/core/address.cpp

src/network/core/address.h

src/network/core/tcp_content.cpp

src/network/core/udp.cpp

src/network/network_gui.cpp

src/network/network_udp.cpp

0 comments (0 inline, 0 general)

src/network/core/address.cpp

➞

Show inline comments

@@ @@ -96,12 +96,11 @@ void NetworkAddress::GetAddressAsString( @@
  * Get the address as a string, e.g. 127.0.0.1:12345.
  * @param with_family whether to add the family (e.g. IPvX).
  * @return the address
  * @note NOT thread safe
  */
-const char *NetworkAddress::GetAddressAsString(bool with_family)
+std::string NetworkAddress::GetAddressAsString(bool with_family)
+{
 	/* 6 = for the : and 5 for the decimal port number */
-	static char buf[NETWORK_HOSTNAME_LENGTH + 6 + 7];
 	char buf[NETWORK_HOSTNAME_LENGTH + 6 + 7];
 	this->GetAddressAsString(buf, lastof(buf), with_family);
 	return buf;
+}
@@ @@ -289,7 +288,8 @@ static SOCKET ConnectLoopProc(addrinfo * @@
+{
 	const char *type = NetworkAddress::SocketTypeAsString(runp->ai_socktype);
 	const char *family = NetworkAddress::AddressFamilyAsString(runp->ai_family);
 	const char *address = NetworkAddress(runp->ai_addr, (int)runp->ai_addrlen).GetAddressAsString();
 	char address[NETWORK_HOSTNAME_LENGTH + 6 + 7];
 	NetworkAddress(runp->ai_addr, (int)runp->ai_addrlen).GetAddressAsString(address, lastof(address));
 	SOCKET sock = socket(runp->ai_family, runp->ai_socktype, runp->ai_protocol);
 	if (sock == INVALID_SOCKET) {
@@ @@ -319,7 +319,7 @@ static SOCKET ConnectLoopProc(addrinfo * @@
  */
 SOCKET NetworkAddress::Connect()
+{
 	DEBUG(net, 1, "Connecting to %s", this->GetAddressAsString());
+	DEBUG(net, 1, "Connecting to %s", this->GetAddressAsString().c_str());
 	return this->Resolve(AF_UNSPEC, SOCK_STREAM, AI_ADDRCONFIG, nullptr, ConnectLoopProc);
+}
@@ @@ -333,7 +333,8 @@ static SOCKET ListenLoopProc(addrinfo *r @@
+{
 	const char *type = NetworkAddress::SocketTypeAsString(runp->ai_socktype);
 	const char *family = NetworkAddress::AddressFamilyAsString(runp->ai_family);
 	const char *address = NetworkAddress(runp->ai_addr, (int)runp->ai_addrlen).GetAddressAsString();
 	char address[NETWORK_HOSTNAME_LENGTH + 6 + 7];
 	NetworkAddress(runp->ai_addr, (int)runp->ai_addrlen).GetAddressAsString(address, lastof(address));
 	SOCKET sock = socket(runp->ai_family, runp->ai_socktype, runp->ai_protocol);
 	if (sock == INVALID_SOCKET) {

src/network/core/address.h

➞

Show inline comments

@@ @@ -15,6 +15,8 @@ @@
 #include "../../string_func.h"
 #include "../../core/smallmap_type.hpp"
 #include <string>
 class NetworkAddress;
 typedef std::vector<NetworkAddress> NetworkAddressList; ///< Type for a list of addresses.
 typedef SmallMap<NetworkAddress, SOCKET> SocketList;    ///< Type for a mapping between address and socket.
@@ @@ -91,7 +93,7 @@ public: @@
 	const char *GetHostname();
 	void GetAddressAsString(char *buffer, const char *last, bool with_family = true);
-	const char *GetAddressAsString(bool with_family = true);
+	std::string GetAddressAsString(bool with_family = true);
 	const sockaddr_storage *GetAddress();
 	/**

src/network/core/tcp_content.cpp

➞

Show inline comments

@@ @@ -171,9 +171,9 @@ bool NetworkContentSocketHandler::Handle @@
 		default:
 			if (this->HasClientQuit()) {
 				DEBUG(net, 0, "[tcp/content] received invalid packet type %d from %s", type, this->client_addr.GetAddressAsString());
+				DEBUG(net, 0, "[tcp/content] received invalid packet type %d from %s", type, this->client_addr.GetAddressAsString().c_str());
 			} else {
 				DEBUG(net, 0, "[tcp/content] received illegal packet from %s", this->client_addr.GetAddressAsString());
+				DEBUG(net, 0, "[tcp/content] received illegal packet from %s", this->client_addr.GetAddressAsString().c_str());
+			}
 			return false;
+	}
@@ @@ -224,7 +224,7 @@ bool NetworkContentSocketHandler::Receiv @@
  */
 bool NetworkContentSocketHandler::ReceiveInvalidPacket(PacketContentType type)
+{
 	DEBUG(net, 0, "[tcp/content] received illegal packet type %d from %s", type, this->client_addr.GetAddressAsString());
+	DEBUG(net, 0, "[tcp/content] received illegal packet type %d from %s", type, this->client_addr.GetAddressAsString().c_str());
 	return false;
+}

src/network/core/udp.cpp

➞

Show inline comments

@@ @@ -100,10 +100,10 @@ void NetworkUDPSocketHandler::SendPacket @@
 		/* Send the buffer */
 		int res = sendto(s.second, (const char*)p->buffer, p->size, 0, (const struct sockaddr *)send.GetAddress(), send.GetAddressLength());
 		DEBUG(net, 7, "[udp] sendto(%s)", send.GetAddressAsString());
+		DEBUG(net, 7, "[udp] sendto(%s)", send.GetAddressAsString().c_str());
 		/* Check for any errors, but ignore it otherwise */
 		if (res == -1) DEBUG(net, 1, "[udp] sendto(%s) failed with: %i", send.GetAddressAsString(), GET_LAST_ERROR());
+		if (res == -1) DEBUG(net, 1, "[udp] sendto(%s) failed with: %i", send.GetAddressAsString().c_str(), GET_LAST_ERROR());
 		if (!all) break;
+	}
@@ @@ -136,7 +136,7 @@ void NetworkUDPSocketHandler::ReceivePac @@
 			/* If the size does not match the packet must be corrupted.
 			 * Otherwise it will be marked as corrupted later on. */
 			if (nbytes != p.size) {
 				DEBUG(net, 1, "received a packet with mismatching size from %s", address.GetAddressAsString());
+				DEBUG(net, 1, "received a packet with mismatching size from %s", address.GetAddressAsString().c_str());
 				continue;
+			}
@@ @@ -313,9 +313,9 @@ void NetworkUDPSocketHandler::HandleUDPP @@
 		default:
 			if (this->HasClientQuit()) {
 				DEBUG(net, 0, "[udp] received invalid packet type %d from %s", type, client_addr->GetAddressAsString());
+				DEBUG(net, 0, "[udp] received invalid packet type %d from %s", type, client_addr->GetAddressAsString().c_str());
 			} else {
 				DEBUG(net, 0, "[udp] received illegal packet from %s", client_addr->GetAddressAsString());
+				DEBUG(net, 0, "[udp] received illegal packet from %s", client_addr->GetAddressAsString().c_str());
+			}
 			break;
+	}
@@ @@ -328,7 +328,7 @@ void NetworkUDPSocketHandler::HandleUDPP @@
  */
 void NetworkUDPSocketHandler::ReceiveInvalidPacket(PacketUDPType type, NetworkAddress *client_addr)
+{
 	DEBUG(net, 0, "[udp] received packet type %d on wrong port from %s", type, client_addr->GetAddressAsString());
+	DEBUG(net, 0, "[udp] received packet type %d on wrong port from %s", type, client_addr->GetAddressAsString().c_str());
+}
 void NetworkUDPSocketHandler::Receive_CLIENT_FIND_SERVER(Packet *p, NetworkAddress *client_addr) { this->ReceiveInvalidPacket(PACKET_UDP_CLIENT_FIND_SERVER, client_addr); }

src/network/network_gui.cpp

➞

Show inline comments

@@ @@ -664,7 +664,9 @@ public: @@
 			DrawString(r.left + WD_FRAMERECT_LEFT, r.right - WD_FRAMERECT_RIGHT, y, STR_NETWORK_SERVER_LIST_SERVER_VERSION); // server version
 			y += FONT_HEIGHT_NORMAL;
 			SetDParamStr(0, sel->address.GetAddressAsString());
 			char network_addr_buffer[NETWORK_HOSTNAME_LENGTH + 6 + 7];
 			sel->address.GetAddressAsString(network_addr_buffer, lastof(network_addr_buffer));
 			SetDParamStr(0, network_addr_buffer);
 			DrawString(r.left + WD_FRAMERECT_LEFT, r.right - WD_FRAMERECT_RIGHT, y, STR_NETWORK_SERVER_LIST_SERVER_ADDRESS); // server address
 			y += FONT_HEIGHT_NORMAL;

src/network/network_udp.cpp

➞

Show inline comments

@@ @@ -244,7 +244,7 @@ void ServerNetworkUDPSocketHandler::Rece @@
 	uint8 in_reply_count = 0;
 	size_t packet_len = 0;
 	DEBUG(net, 6, "[udp] newgrf data request from %s", client_addr->GetAddressAsString());
+	DEBUG(net, 6, "[udp] newgrf data request from %s", client_addr->GetAddressAsString().c_str());
 	num_grfs = p->Recv_uint8 ();
 	if (num_grfs > NETWORK_MAX_GRF_COUNT) return;
@@ @@ -307,7 +307,7 @@ void ClientNetworkUDPSocketHandler::Rece @@
 	/* Just a fail-safe.. should never happen */
 	if (_network_udp_server) return;
 	DEBUG(net, 4, "[udp] server response from %s", client_addr->GetAddressAsString());
+	DEBUG(net, 4, "[udp] server response from %s", client_addr->GetAddressAsString().c_str());
 	/* Find next item */
 	item = NetworkGameListAddItem(*client_addr);
@@ @@ -407,7 +407,7 @@ void ClientNetworkUDPSocketHandler::Rece @@
 	uint8 num_grfs;
 	uint i;
 	DEBUG(net, 6, "[udp] newgrf data reply from %s", client_addr->GetAddressAsString());
+	DEBUG(net, 6, "[udp] newgrf data reply from %s", client_addr->GetAddressAsString().c_str());
 	num_grfs = p->Recv_uint8 ();
 	if (num_grfs > NETWORK_MAX_GRF_COUNT) return;
@@ @@ -477,7 +477,7 @@ void NetworkUDPQueryMasterServer() @@
 	_udp_client_socket->SendPacket(&p, &out_addr, true);
 	DEBUG(net, 2, "[udp] master server queried at %s", out_addr.GetAddressAsString());
+	DEBUG(net, 2, "[udp] master server queried at %s", out_addr.GetAddressAsString().c_str());
+}
 /** Find all servers */
@@ @@ -541,8 +541,8 @@ static void NetworkUDPAdvertiseThread() @@
 	if (_session_key == 0 && session_key_retries++ == 2) {
 		DEBUG(net, 0, "[udp] advertising to the master server is failing");
 		DEBUG(net, 0, "[udp]   we are not receiving the session key from the server");
 		DEBUG(net, 0, "[udp]   please allow udp packets from %s to you to be delivered", out_addr.GetAddressAsString(false));
 		DEBUG(net, 0, "[udp]   please allow udp packets from you to %s to be delivered", out_addr.GetAddressAsString(false));
 		DEBUG(net, 0, "[udp]   please allow udp packets from %s to you to be delivered", out_addr.GetAddressAsString(false).c_str());
 		DEBUG(net, 0, "[udp]   please allow udp packets from you to %s to be delivered", out_addr.GetAddressAsString(false).c_str());
+	}
 	if (_session_key != 0 && _network_advertise_retries == 0) {
 		DEBUG(net, 0, "[udp] advertising to the master server is failing");

0 comments (0 inline, 0 general)