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

@@ @@ -93,18 +93,17 @@ 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;
+}
 /**
  * Helper function to resolve without opening a socket.
@@ @@ -286,13 +285,14 @@ SOCKET NetworkAddress::Resolve(int famil @@
  * @return the opened socket or INVALID_SOCKET
  */
 static SOCKET ConnectLoopProc(addrinfo *runp)
+{
 	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) {
 		DEBUG(net, 1, "[%s] could not create %s socket: %s", type, family, strerror(errno));
 		return INVALID_SOCKET;
+	}
@@ @@ -316,13 +316,13 @@ static SOCKET ConnectLoopProc(addrinfo * @@
 /**
  * Connect to the given address.
  * @return the connected socket or INVALID_SOCKET.
  */
 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);
+}
 /**
  * Helper function to resolve a listening.
@@ @@ -330,13 +330,14 @@ SOCKET NetworkAddress::Connect() @@
  * @return the opened socket or INVALID_SOCKET
  */
 static SOCKET ListenLoopProc(addrinfo *runp)
+{
 	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) {
 		DEBUG(net, 0, "[%s] could not create %s socket on port %s: %s", type, family, address, strerror(errno));
 		return INVALID_SOCKET;
+	}

src/network/core/address.h

➞

Show inline comments

@@ @@ -12,12 +12,14 @@ @@
 #include "os_abstraction.h"
 #include "config.h"
 #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.
 /**
  * Wrapper for (un)resolved network addresses; there's no reason to transform
@@ @@ -88,13 +90,13 @@ public: @@
 		this->address.ss_family = family;
 		this->SetPort(port);
+	}
 	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();
 	/**
 	 * Get the (valid) length of the address.
 	 * @return the length
 	 */

src/network/core/tcp_content.cpp

➞

Show inline comments

@@ @@ -168,15 +168,15 @@ bool NetworkContentSocketHandler::Handle @@
 		case PACKET_CONTENT_SERVER_INFO:           return this->Receive_SERVER_INFO(p);
 		case PACKET_CONTENT_CLIENT_CONTENT:        return this->Receive_CLIENT_CONTENT(p);
 		case PACKET_CONTENT_SERVER_CONTENT:        return this->Receive_SERVER_CONTENT(p);
 		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;
+	}
+}
 /**
@@ @@ -221,13 +221,13 @@ bool NetworkContentSocketHandler::Receiv @@
  * Helper for logging receiving invalid packets.
  * @param type The received packet type.
  * @return Always false, as it's an error.
  */
 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;
+}
 bool NetworkContentSocketHandler::Receive_CLIENT_INFO_LIST(Packet *p) { return this->ReceiveInvalidPacket(PACKET_CONTENT_CLIENT_INFO_LIST); }
 bool NetworkContentSocketHandler::Receive_CLIENT_INFO_ID(Packet *p) { return this->ReceiveInvalidPacket(PACKET_CONTENT_CLIENT_INFO_ID); }
 bool NetworkContentSocketHandler::Receive_CLIENT_INFO_EXTID(Packet *p) { return this->ReceiveInvalidPacket(PACKET_CONTENT_CLIENT_INFO_EXTID); }

src/network/core/udp.cpp

➞

Show inline comments

@@ @@ -97,16 +97,16 @@ void NetworkUDPSocketHandler::SendPacket @@
 				DEBUG(net, 1, "[udp] setting broadcast failed with: %i", GET_LAST_ERROR());
+			}
+		}
 		/* 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;
+	}
+}
 /**
@@ @@ -133,13 +133,13 @@ void NetworkUDPSocketHandler::ReceivePac @@
 			NetworkAddress address(client_addr, client_len);
 			p.PrepareToRead();
 			/* 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;
+			}
 			/* Handle the packet */
 			this->HandleUDPPacket(&p, &address);
+		}
@@ @@ -310,28 +310,28 @@ void NetworkUDPSocketHandler::HandleUDPP @@
 		case PACKET_UDP_CLIENT_GET_NEWGRFS:   this->Receive_CLIENT_GET_NEWGRFS(p, client_addr);   break;
 		case PACKET_UDP_SERVER_NEWGRFS:       this->Receive_SERVER_NEWGRFS(p, client_addr);       break;
 		case PACKET_UDP_MASTER_SESSION_KEY:   this->Receive_MASTER_SESSION_KEY(p, client_addr);   break;
 		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;
+	}
+}
 /**
  * Helper for logging receiving invalid packets.
  * @param type The received packet type.
  * @param client_addr The address we received the packet from.
  */
 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); }
 void NetworkUDPSocketHandler::Receive_SERVER_RESPONSE(Packet *p, NetworkAddress *client_addr) { this->ReceiveInvalidPacket(PACKET_UDP_SERVER_RESPONSE, client_addr); }
 void NetworkUDPSocketHandler::Receive_CLIENT_DETAIL_INFO(Packet *p, NetworkAddress *client_addr) { this->ReceiveInvalidPacket(PACKET_UDP_CLIENT_DETAIL_INFO, client_addr); }
 void NetworkUDPSocketHandler::Receive_SERVER_DETAIL_INFO(Packet *p, NetworkAddress *client_addr) { this->ReceiveInvalidPacket(PACKET_UDP_SERVER_DETAIL_INFO, client_addr); }

src/network/network_gui.cpp

➞

Show inline comments

@@ @@ -661,13 +661,15 @@ public: @@
 			y += FONT_HEIGHT_NORMAL;
 			SetDParamStr(0, sel->info.server_revision);
 			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;
 			SetDParam(0, sel->info.start_date);
 			DrawString(r.left + WD_FRAMERECT_LEFT, r.right - WD_FRAMERECT_RIGHT, y, STR_NETWORK_SERVER_LIST_START_DATE); // start date
 			y += FONT_HEIGHT_NORMAL;

src/network/network_udp.cpp

➞

Show inline comments

@@ @@ -241,13 +241,13 @@ void ServerNetworkUDPSocketHandler::Rece @@
 	uint i;
 	const GRFConfig *in_reply[NETWORK_MAX_GRF_COUNT];
 	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;
 	for (i = 0; i < num_grfs; i++) {
 		GRFIdentifier c;
@@ @@ -304,13 +304,13 @@ void ClientNetworkUDPSocketHandler::Rece @@
+{
 	NetworkGameList *item;
 	/* 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);
 	ClearGRFConfigList(&item->info.grfconfig);
 	this->ReceiveNetworkGameInfo(p, &item->info);
@@ @@ -404,13 +404,13 @@ void ClientNetworkUDPSocketHandler::Rece @@
 /** The return of the client's request of the names of some NewGRFs */
 void ClientNetworkUDPSocketHandler::Receive_SERVER_NEWGRFS(Packet *p, NetworkAddress *client_addr)
+{
 	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;
 	for (i = 0; i < num_grfs; i++) {
 		char name[NETWORK_GRF_NAME_LENGTH];
@@ @@ -474,13 +474,13 @@ void NetworkUDPQueryMasterServer() @@
 	/* packet only contains protocol version */
 	p.Send_uint8(NETWORK_MASTER_SERVER_VERSION);
 	p.Send_uint8(SLT_AUTODETECT);
 	_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 */
 void NetworkUDPSearchGame()
+{
 	/* We are still searching.. */
@@ @@ -538,14 +538,14 @@ static void NetworkUDPAdvertiseThread() @@
 	/* Add a bit more messaging when we cannot get a session key */
 	static byte session_key_retries = 0;
 	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");
 		DEBUG(net, 0, "[udp]   we are not receiving the acknowledgement from the server");
 		DEBUG(net, 0, "[udp]   this usually means that the master server cannot reach us");
 		DEBUG(net, 0, "[udp]   please allow udp and tcp packets to port %u to be delivered", _settings_client.network.server_port);

0 comments (0 inline, 0 general)