From c545cc9d7039a89e23de160b1c93adc959eabda5 Mon Sep 17 00:00:00 2001
From: Rubidium <rubidium@openttd.org>
Date: Sun, 18 Apr 2021 09:01:27 +0200
Subject: [PATCH 01/10] Codechange: move more logic about packet size validity
 and reading into Packet

---
 src/network/core/packet.cpp | 25 +++++++++++++++++++++----
 src/network/core/packet.h   |  3 ++-
 src/network/core/tcp.cpp    | 10 ++++------
 src/network/core/udp.cpp    |  4 ++--
 4 files changed, 29 insertions(+), 13 deletions(-)

diff --git a/src/network/core/packet.cpp b/src/network/core/packet.cpp
index 94ffcc5584..4eb0e929ee 100644
--- a/src/network/core/packet.cpp
+++ b/src/network/core/packet.cpp
@@ -181,13 +181,32 @@ bool Packet::CanReadFromPacket(uint bytes_to_read)
 }
 
 /**
- * Reads the packet size from the raw packet and stores it in the packet->size
+ * Check whether the packet, given the position of the "write" pointer, has read
+ * enough of the packet to contain its size.
+ * @return True iff there is enough data in the packet to contain the packet's size.
  */
-void Packet::ReadRawPacketSize()
+bool Packet::HasPacketSizeData() const
+{
+	return this->pos >= sizeof(PacketSize);
+}
+
+/**
+ * Reads the packet size from the raw packet and stores it in the packet->size
+ * @return True iff the packet size seems plausible.
+ */
+bool Packet::ParsePacketSize()
 {
 	assert(this->cs != nullptr && this->next == nullptr);
 	this->size  = (PacketSize)this->buffer[0];
 	this->size += (PacketSize)this->buffer[1] << 8;
+
+	/* If the size of the packet is less than the bytes required for the size and type of
+	 * the packet, or more than the allowed limit, then something is wrong with the packet.
+	 * In those cases the packet can generally be regarded as containing garbage data. */
+	if (this->size < sizeof(PacketSize) + sizeof(PacketType) || this->size > SEND_MTU) return false;
+
+	this->pos = sizeof(PacketSize);
+	return true;
 }
 
 /**
@@ -195,8 +214,6 @@ void Packet::ReadRawPacketSize()
  */
 void Packet::PrepareToRead()
 {
-	this->ReadRawPacketSize();
-
 	/* Put the position on the right place */
 	this->pos = sizeof(PacketSize);
 }
diff --git a/src/network/core/packet.h b/src/network/core/packet.h
index c9be4eeb53..6e5c5509ce 100644
--- a/src/network/core/packet.h
+++ b/src/network/core/packet.h
@@ -71,7 +71,8 @@ public:
 	void Send_string(const char *data);
 
 	/* Reading/receiving of packets */
-	void ReadRawPacketSize();
+	bool HasPacketSizeData() const;
+	bool ParsePacketSize();
 	void PrepareToRead();
 
 	bool   CanReadFromPacket (uint bytes_to_read);
diff --git a/src/network/core/tcp.cpp b/src/network/core/tcp.cpp
index a51913d843..1461a92981 100644
--- a/src/network/core/tcp.cpp
+++ b/src/network/core/tcp.cpp
@@ -155,8 +155,8 @@ Packet *NetworkTCPSocketHandler::ReceivePacket()
 	Packet *p = this->packet_recv;
 
 	/* Read packet size */
-	if (p->pos < sizeof(PacketSize)) {
-		while (p->pos < sizeof(PacketSize)) {
+	if (!p->HasPacketSizeData()) {
+		while (!p->HasPacketSizeData()) {
 		/* Read the size of the packet */
 			res = recv(this->sock, (char*)p->buffer + p->pos, sizeof(PacketSize) - p->pos, 0);
 			if (res == -1) {
@@ -178,10 +178,8 @@ Packet *NetworkTCPSocketHandler::ReceivePacket()
 			p->pos += res;
 		}
 
-		/* Read the packet size from the received packet */
-		p->ReadRawPacketSize();
-
-		if (p->size > SEND_MTU) {
+		/* Parse the size in the received packet and if not valid, close the connection. */
+		if (!p->ParsePacketSize()) {
 			this->CloseConnection();
 			return nullptr;
 		}
diff --git a/src/network/core/udp.cpp b/src/network/core/udp.cpp
index 72fec49e1e..614ae8c3ba 100644
--- a/src/network/core/udp.cpp
+++ b/src/network/core/udp.cpp
@@ -134,14 +134,14 @@ void NetworkUDPSocketHandler::ReceivePackets()
 #endif
 
 			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) {
+			if (!p.ParsePacketSize() || nbytes != p.size) {
 				DEBUG(net, 1, "received a packet with mismatching size from %s", address.GetAddressAsString().c_str());
 				continue;
 			}
+			p.PrepareToRead();
 
 			/* Handle the packet */
 			this->HandleUDPPacket(&p, &address);

From a2051bad503618f37e941aca3e4a5d53af1b0fbe Mon Sep 17 00:00:00 2001
From: Rubidium <rubidium@openttd.org>
Date: Sun, 18 Apr 2021 09:26:06 +0200
Subject: [PATCH 02/10] Codechange: move logic whether there is enough space in
 a packet to write data into the Packet

---
 src/network/core/packet.cpp    | 41 ++++++++++++++++++++++------------
 src/network/core/packet.h      |  3 ++-
 src/network/network_admin.cpp  |  2 +-
 src/network/network_client.cpp |  2 +-
 src/network/network_udp.cpp    | 12 +++++-----
 5 files changed, 37 insertions(+), 23 deletions(-)

diff --git a/src/network/core/packet.cpp b/src/network/core/packet.cpp
index 4eb0e929ee..54f5a79e16 100644
--- a/src/network/core/packet.cpp
+++ b/src/network/core/packet.cpp
@@ -68,6 +68,16 @@ void Packet::PrepareToSend()
 	this->pos  = 0; // We start reading from here
 }
 
+/**
+ * Is it safe to write to the packet, i.e. didn't we run over the buffer?
+ * @param bytes_to_write The amount of bytes we want to try to write.
+ * @return True iff the given amount of bytes can be written to the packet.
+ */
+bool Packet::CanWriteToPacket(size_t bytes_to_write)
+{
+	return this->size + bytes_to_write < SEND_MTU;
+}
+
 /*
  * The next couple of functions make sure we can send
  *  uint8, uint16, uint32 and uint64 endian-safe
@@ -95,7 +105,7 @@ void Packet::Send_bool(bool data)
  */
 void Packet::Send_uint8(uint8 data)
 {
-	assert(this->size < SEND_MTU - sizeof(data));
+	assert(this->CanWriteToPacket(sizeof(data)));
 	this->buffer[this->size++] = data;
 }
 
@@ -105,7 +115,7 @@ void Packet::Send_uint8(uint8 data)
  */
 void Packet::Send_uint16(uint16 data)
 {
-	assert(this->size < SEND_MTU - sizeof(data));
+	assert(this->CanWriteToPacket(sizeof(data)));
 	this->buffer[this->size++] = GB(data, 0, 8);
 	this->buffer[this->size++] = GB(data, 8, 8);
 }
@@ -116,7 +126,7 @@ void Packet::Send_uint16(uint16 data)
  */
 void Packet::Send_uint32(uint32 data)
 {
-	assert(this->size < SEND_MTU - sizeof(data));
+	assert(this->CanWriteToPacket(sizeof(data)));
 	this->buffer[this->size++] = GB(data,  0, 8);
 	this->buffer[this->size++] = GB(data,  8, 8);
 	this->buffer[this->size++] = GB(data, 16, 8);
@@ -129,7 +139,7 @@ void Packet::Send_uint32(uint32 data)
  */
 void Packet::Send_uint64(uint64 data)
 {
-	assert(this->size < SEND_MTU - sizeof(data));
+	assert(this->CanWriteToPacket(sizeof(data)));
 	this->buffer[this->size++] = GB(data,  0, 8);
 	this->buffer[this->size++] = GB(data,  8, 8);
 	this->buffer[this->size++] = GB(data, 16, 8);
@@ -148,8 +158,8 @@ void Packet::Send_uint64(uint64 data)
 void Packet::Send_string(const char *data)
 {
 	assert(data != nullptr);
-	/* The <= *is* valid due to the fact that we are comparing sizes and not the index. */
-	assert(this->size + strlen(data) + 1 <= SEND_MTU);
+	/* Length of the string + 1 for the '\0' termination. */
+	assert(this->CanWriteToPacket(strlen(data) + 1));
 	while ((this->buffer[this->size++] = *data++) != '\0') {}
 }
 
@@ -162,18 +172,21 @@ void Packet::Send_string(const char *data)
 
 
 /**
- * Is it safe to read from the packet, i.e. didn't we run over the buffer ?
- * @param bytes_to_read The amount of bytes we want to try to read.
+ * Is it safe to read from the packet, i.e. didn't we run over the buffer?
+ * In case \c close_connection is true, the connection will be closed when one would
+ * overrun the buffer. When it is false, the connection remains untouched.
+ * @param bytes_to_read    The amount of bytes we want to try to read.
+ * @param close_connection Whether to close the connection if one cannot read that amount.
  * @return True if that is safe, otherwise false.
  */
-bool Packet::CanReadFromPacket(uint bytes_to_read)
+bool Packet::CanReadFromPacket(size_t bytes_to_read, bool close_connection)
 {
 	/* Don't allow reading from a quit client/client who send bad data */
 	if (this->cs->HasClientQuit()) return false;
 
 	/* Check if variable is within packet-size */
 	if (this->pos + bytes_to_read > this->size) {
-		this->cs->NetworkSocketHandler::CloseConnection();
+		if (close_connection) this->cs->NetworkSocketHandler::CloseConnection();
 		return false;
 	}
 
@@ -235,7 +248,7 @@ uint8 Packet::Recv_uint8()
 {
 	uint8 n;
 
-	if (!this->CanReadFromPacket(sizeof(n))) return 0;
+	if (!this->CanReadFromPacket(sizeof(n), true)) return 0;
 
 	n = this->buffer[this->pos++];
 	return n;
@@ -249,7 +262,7 @@ uint16 Packet::Recv_uint16()
 {
 	uint16 n;
 
-	if (!this->CanReadFromPacket(sizeof(n))) return 0;
+	if (!this->CanReadFromPacket(sizeof(n), true)) return 0;
 
 	n  = (uint16)this->buffer[this->pos++];
 	n += (uint16)this->buffer[this->pos++] << 8;
@@ -264,7 +277,7 @@ uint32 Packet::Recv_uint32()
 {
 	uint32 n;
 
-	if (!this->CanReadFromPacket(sizeof(n))) return 0;
+	if (!this->CanReadFromPacket(sizeof(n), true)) return 0;
 
 	n  = (uint32)this->buffer[this->pos++];
 	n += (uint32)this->buffer[this->pos++] << 8;
@@ -281,7 +294,7 @@ uint64 Packet::Recv_uint64()
 {
 	uint64 n;
 
-	if (!this->CanReadFromPacket(sizeof(n))) return 0;
+	if (!this->CanReadFromPacket(sizeof(n), true)) return 0;
 
 	n  = (uint64)this->buffer[this->pos++];
 	n += (uint64)this->buffer[this->pos++] << 8;
diff --git a/src/network/core/packet.h b/src/network/core/packet.h
index 6e5c5509ce..901d3f593b 100644
--- a/src/network/core/packet.h
+++ b/src/network/core/packet.h
@@ -63,6 +63,7 @@ public:
 	/* Sending/writing of packets */
 	void PrepareToSend();
 
+	bool CanWriteToPacket(size_t bytes_to_write);
 	void Send_bool  (bool   data);
 	void Send_uint8 (uint8  data);
 	void Send_uint16(uint16 data);
@@ -75,7 +76,7 @@ public:
 	bool ParsePacketSize();
 	void PrepareToRead();
 
-	bool   CanReadFromPacket (uint bytes_to_read);
+	bool   CanReadFromPacket(size_t bytes_to_read, bool close_connection = false);
 	bool   Recv_bool  ();
 	uint8  Recv_uint8 ();
 	uint16 Recv_uint16();
diff --git a/src/network/network_admin.cpp b/src/network/network_admin.cpp
index fa97b7e578..057ad59883 100644
--- a/src/network/network_admin.cpp
+++ b/src/network/network_admin.cpp
@@ -613,7 +613,7 @@ NetworkRecvStatus ServerNetworkAdminSocketHandler::SendCmdNames()
 		/* Should SEND_MTU be exceeded, start a new packet
 		 * (magic 5: 1 bool "more data" and one uint16 "command id", one
 		 * byte for string '\0' termination and 1 bool "no more data" */
-		if (p->size + strlen(cmdname) + 5 >= SEND_MTU) {
+		if (p->CanWriteToPacket(strlen(cmdname) + 5)) {
 			p->Send_bool(false);
 			this->SendPacket(p);
 
diff --git a/src/network/network_client.cpp b/src/network/network_client.cpp
index 72f69f99f7..10b4fd1411 100644
--- a/src/network/network_client.cpp
+++ b/src/network/network_client.cpp
@@ -933,7 +933,7 @@ NetworkRecvStatus ClientNetworkGameSocketHandler::Receive_SERVER_FRAME(Packet *p
 	}
 #endif
 	/* Receive the token. */
-	if (p->pos != p->size) this->token = p->Recv_uint8();
+	if (p->CanReadFromPacket(sizeof(uint8))) this->token = p->Recv_uint8();
 
 	DEBUG(net, 5, "Received FRAME %d", _frame_counter_server);
 
diff --git a/src/network/network_udp.cpp b/src/network/network_udp.cpp
index 46a21fc87d..aa34515bdd 100644
--- a/src/network/network_udp.cpp
+++ b/src/network/network_udp.cpp
@@ -220,23 +220,23 @@ void ServerNetworkUDPSocketHandler::Receive_CLIENT_DETAIL_INFO(Packet *p, Networ
 	static const uint MIN_CI_SIZE = 54;
 	uint max_cname_length = NETWORK_COMPANY_NAME_LENGTH;
 
-	if (Company::GetNumItems() * (MIN_CI_SIZE + NETWORK_COMPANY_NAME_LENGTH) >= (uint)SEND_MTU - packet.size) {
+	if (!packet.CanWriteToPacket(Company::GetNumItems() * (MIN_CI_SIZE + NETWORK_COMPANY_NAME_LENGTH))) {
 		/* Assume we can at least put the company information in the packets. */
-		assert(Company::GetNumItems() * MIN_CI_SIZE < (uint)SEND_MTU - packet.size);
+		assert(packet.CanWriteToPacket(Company::GetNumItems() * MIN_CI_SIZE));
 
 		/* At this moment the company names might not fit in the
 		 * packet. Check whether that is really the case. */
 
 		for (;;) {
-			int free = SEND_MTU - packet.size;
+			size_t required = 0;
 			for (const Company *company : Company::Iterate()) {
 				char company_name[NETWORK_COMPANY_NAME_LENGTH];
 				SetDParam(0, company->index);
 				GetString(company_name, STR_COMPANY_NAME, company_name + max_cname_length - 1);
-				free -= MIN_CI_SIZE;
-				free -= (int)strlen(company_name);
+				required += MIN_CI_SIZE;
+				required += strlen(company_name);
 			}
-			if (free >= 0) break;
+			if (packet.CanWriteToPacket(required)) break;
 
 			/* Try again, with slightly shorter strings. */
 			assert(max_cname_length > 0);

From 98aa561cf759f75971afd5dfc4d42e6921a8ab1a Mon Sep 17 00:00:00 2001
From: Rubidium <rubidium@openttd.org>
Date: Sun, 18 Apr 2021 09:55:00 +0200
Subject: [PATCH 03/10] Codechange: encapsulate reading data from sockets into
 Packets to prevent packet state modifications outside of the Packet

---
 src/network/core/packet.cpp | 24 +++++++++++++++---
 src/network/core/packet.h   | 50 ++++++++++++++++++++++++++++++++++++-
 src/network/core/tcp.cpp    | 12 +++------
 src/network/core/udp.cpp    |  4 +--
 4 files changed, 75 insertions(+), 15 deletions(-)

diff --git a/src/network/core/packet.cpp b/src/network/core/packet.cpp
index 54f5a79e16..5cb6716bca 100644
--- a/src/network/core/packet.cpp
+++ b/src/network/core/packet.cpp
@@ -17,17 +17,24 @@
 #include "../../safeguards.h"
 
 /**
- * Create a packet that is used to read from a network socket
- * @param cs the socket handler associated with the socket we are reading from
+ * Create a packet that is used to read from a network socket.
+ * @param cs                The socket handler associated with the socket we are reading from.
+ * @param initial_read_size The initial amount of data to transfer from the socket into the
+ *                          packet. This defaults to just the required bytes to determine the
+ *                          packet's size. That default is the wanted for streams such as TCP
+ *                          as you do not want to read data of the next packet yet. For UDP
+ *                          you need to read the whole packet at once otherwise you might
+ *                          loose some the data of the packet, so there you pass the maximum
+ *                          size for the packet you expect from the network.
  */
-Packet::Packet(NetworkSocketHandler *cs)
+Packet::Packet(NetworkSocketHandler *cs, size_t initial_read_size)
 {
 	assert(cs != nullptr);
 
 	this->cs     = cs;
 	this->next   = nullptr;
 	this->pos    = 0; // We start reading from here
-	this->size   = 0;
+	this->size   = static_cast<int>(initial_read_size);
 	this->buffer = MallocT<byte>(SEND_MTU);
 }
 
@@ -336,3 +343,12 @@ void Packet::Recv_string(char *buffer, size_t size, StringValidationSettings set
 
 	str_validate(bufp, last, settings);
 }
+
+/**
+ * Get the amount of bytes that are still available for the Transfer functions.
+ * @return The number of bytes that still have to be transfered.
+ */
+size_t Packet::RemainingBytesToTransfer() const
+{
+	return this->size - this->pos;
+}
diff --git a/src/network/core/packet.h b/src/network/core/packet.h
index 901d3f593b..3eee0522ff 100644
--- a/src/network/core/packet.h
+++ b/src/network/core/packet.h
@@ -12,9 +12,11 @@
 #ifndef NETWORK_CORE_PACKET_H
 #define NETWORK_CORE_PACKET_H
 
+#include "os_abstraction.h"
 #include "config.h"
 #include "core.h"
 #include "../../string_type.h"
+#include <functional>
 
 typedef uint16 PacketSize; ///< Size of the whole packet.
 typedef uint8  PacketType; ///< Identifier for the packet
@@ -56,7 +58,7 @@ private:
 	NetworkSocketHandler *cs;
 
 public:
-	Packet(NetworkSocketHandler *cs);
+	Packet(NetworkSocketHandler *cs, size_t initial_read_size = sizeof(PacketSize));
 	Packet(PacketType type);
 	~Packet();
 
@@ -83,6 +85,52 @@ public:
 	uint32 Recv_uint32();
 	uint64 Recv_uint64();
 	void   Recv_string(char *buffer, size_t size, StringValidationSettings settings = SVS_REPLACE_WITH_QUESTION_MARK);
+
+	size_t RemainingBytesToTransfer() const;
+
+	/**
+	 * Transfer data from the given function into the packet. It starts writing at the
+	 * position the last transfer stopped.
+	 *
+	 * Examples of functions that can be used to transfer data into a packet are TCP's
+	 * recv and UDP's recvfrom functions. They will directly write their data into the
+	 * packet without an intermediate buffer.
+	 * Examples of functions that can be used to transfer data from a packet are TCP's
+	 * send and UDP's sendto functions. They will directly read the data from the packet's
+	 * buffer without an intermediate buffer.
+	 * These are functions are special in a sense as even though the packet can send or
+	 * receive an amount of data, those functions can say they only processed a smaller
+	 * amount, so special handling is required to keep the position pointers correct.
+	 * Most of these transfer functions are in the form function(source, buffer, amount, ...),
+	 * so the template of this function will assume that as the base parameter order.
+	 *
+	 * This will attempt to write all the remaining bytes into the packet. It updates the
+	 * position based on how many bytes were actually written by the called transfer_function.
+	 * @param transfer_function The function to pass the buffer as second parameter and the
+	 *                          amount to read as third parameter. It returns the amount that
+	 *                          was read or -1 upon errors.
+	 * @param source            The first parameter of the transfer function.
+	 * @param args              The fourth and further parameters to the transfer function, if any.
+	 * @tparam A    The type for the amount to be passed, so it can be cast to the right type.
+	 * @tparam F    The type of the transfer_function.
+	 * @tparam S    The type of the source.
+	 * @tparam Args The types of the remaining arguments to the function.
+	 * @return The return value of the transfer_function.
+	 */
+	template <typename A = size_t, typename F, typename S, typename ... Args>
+	ssize_t TransferIn(F transfer_function, S source, Args&& ... args)
+	{
+		size_t amount = this->RemainingBytesToTransfer();
+		if (amount == 0) return 0;
+
+		assert(this->pos < this->buffer.size());
+		assert(this->pos + amount <= this->buffer.size());
+		/* Making buffer a char means casting a lot in the Recv/Send functions. */
+		char *input_buffer = reinterpret_cast<char*>(this->buffer + this->pos);
+		ssize_t bytes = transfer_function(source, input_buffer, static_cast<A>(amount), std::forward<Args>(args)...);
+		if (bytes > 0) this->pos += bytes;
+		return bytes;
+	}
 };
 
 #endif /* NETWORK_CORE_PACKET_H */
diff --git a/src/network/core/tcp.cpp b/src/network/core/tcp.cpp
index 1461a92981..aa1e1cbeda 100644
--- a/src/network/core/tcp.cpp
+++ b/src/network/core/tcp.cpp
@@ -156,9 +156,8 @@ Packet *NetworkTCPSocketHandler::ReceivePacket()
 
 	/* Read packet size */
 	if (!p->HasPacketSizeData()) {
-		while (!p->HasPacketSizeData()) {
-		/* Read the size of the packet */
-			res = recv(this->sock, (char*)p->buffer + p->pos, sizeof(PacketSize) - p->pos, 0);
+		while (p->RemainingBytesToTransfer() != 0) {
+			res = p->TransferIn<int>(recv, this->sock, 0);
 			if (res == -1) {
 				int err = GET_LAST_ERROR();
 				if (err != EWOULDBLOCK) {
@@ -175,7 +174,6 @@ Packet *NetworkTCPSocketHandler::ReceivePacket()
 				this->CloseConnection();
 				return nullptr;
 			}
-			p->pos += res;
 		}
 
 		/* Parse the size in the received packet and if not valid, close the connection. */
@@ -186,8 +184,8 @@ Packet *NetworkTCPSocketHandler::ReceivePacket()
 	}
 
 	/* Read rest of packet */
-	while (p->pos < p->size) {
-		res = recv(this->sock, (char*)p->buffer + p->pos, p->size - p->pos, 0);
+	while (p->RemainingBytesToTransfer() != 0) {
+		res = p->TransferIn<int>(recv, this->sock, 0);
 		if (res == -1) {
 			int err = GET_LAST_ERROR();
 			if (err != EWOULDBLOCK) {
@@ -204,8 +202,6 @@ Packet *NetworkTCPSocketHandler::ReceivePacket()
 			this->CloseConnection();
 			return nullptr;
 		}
-
-		p->pos += res;
 	}
 
 	/* Prepare for receiving a new packet */
diff --git a/src/network/core/udp.cpp b/src/network/core/udp.cpp
index 614ae8c3ba..398f53142b 100644
--- a/src/network/core/udp.cpp
+++ b/src/network/core/udp.cpp
@@ -119,12 +119,12 @@ void NetworkUDPSocketHandler::ReceivePackets()
 			struct sockaddr_storage client_addr;
 			memset(&client_addr, 0, sizeof(client_addr));
 
-			Packet p(this);
+			Packet p(this, SEND_MTU);
 			socklen_t client_len = sizeof(client_addr);
 
 			/* Try to receive anything */
 			SetNonBlocking(s.second); // Some OSes seem to lose the non-blocking status of the socket
-			int nbytes = recvfrom(s.second, (char*)p.buffer, SEND_MTU, 0, (struct sockaddr *)&client_addr, &client_len);
+			ssize_t nbytes = p.TransferIn<int>(recvfrom, s.second, 0, (struct sockaddr *)&client_addr, &client_len);
 
 			/* Did we get the bytes for the base header of the packet? */
 			if (nbytes <= 0) break;    // No data, i.e. no packet

From d4f027c03bfc5f632b7d63c125dfa57a7ba89926 Mon Sep 17 00:00:00 2001
From: Rubidium <rubidium@openttd.org>
Date: Sun, 18 Apr 2021 10:23:41 +0200
Subject: [PATCH 04/10] Codechange: encapsulate writing data from Packets into
 sockets/files/buffers to prevent packet state modifications outside of the
 Packet

---
 src/network/core/packet.h       | 52 +++++++++++++++++++++++++++++++++
 src/network/core/tcp.cpp        |  6 ++--
 src/network/core/tcp_listen.h   |  4 +--
 src/network/core/udp.cpp        |  2 +-
 src/network/network_client.cpp  | 36 ++++++++++++-----------
 src/network/network_content.cpp | 16 ++++++++--
 6 files changed, 90 insertions(+), 26 deletions(-)

diff --git a/src/network/core/packet.h b/src/network/core/packet.h
index 3eee0522ff..b091d8a7ec 100644
--- a/src/network/core/packet.h
+++ b/src/network/core/packet.h
@@ -88,6 +88,58 @@ public:
 
 	size_t RemainingBytesToTransfer() const;
 
+	/**
+	 * Transfer data from the packet to the given function. It starts reading at the
+	 * position the last transfer stopped.
+	 * See Packet::TransferIn for more information about transferring data to functions.
+	 * @param transfer_function The function to pass the buffer as second parameter and the
+	 *                          amount to write as third parameter. It returns the amount that
+	 *                          was written or -1 upon errors.
+	 * @param limit             The maximum amount of bytes to transfer.
+	 * @param destination       The first parameter of the transfer function.
+	 * @param args              The fourth and further parameters to the transfer function, if any.
+	 * @return The return value of the transfer_function.
+	 */
+	template <
+		typename A = size_t, ///< The type for the amount to be passed, so it can be cast to the right type.
+		typename F,          ///< The type of the function.
+		typename D,          ///< The type of the destination.
+		typename ... Args>   ///< The types of the remaining arguments to the function.
+	ssize_t TransferOutWithLimit(F transfer_function, size_t limit, D destination, Args&& ... args)
+	{
+		size_t amount = std::min(this->RemainingBytesToTransfer(), limit);
+		if (amount == 0) return 0;
+
+		assert(this->pos < this->buffer.size());
+		assert(this->pos + amount <= this->buffer.size());
+		/* Making buffer a char means casting a lot in the Recv/Send functions. */
+		const char *output_buffer = reinterpret_cast<const char*>(this->buffer + this->pos);
+		ssize_t bytes = transfer_function(destination, output_buffer, static_cast<A>(amount), std::forward<Args>(args)...);
+		if (bytes > 0) this->pos += bytes;
+		return bytes;
+	}
+
+	/**
+	 * Transfer data from the packet to the given function. It starts reading at the
+	 * position the last transfer stopped.
+	 * See Packet::TransferIn for more information about transferring data to functions.
+	 * @param transfer_function The function to pass the buffer as second parameter and the
+	 *                          amount to write as third parameter. It returns the amount that
+	 *                          was written or -1 upon errors.
+	 * @param destination       The first parameter of the transfer function.
+	 * @param args              The fourth and further parameters to the transfer function, if any.
+	 * @tparam A    The type for the amount to be passed, so it can be cast to the right type.
+	 * @tparam F    The type of the transfer_function.
+	 * @tparam D    The type of the destination.
+	 * @tparam Args The types of the remaining arguments to the function.
+	 * @return The return value of the transfer_function.
+	 */
+	template <typename A = size_t, typename F, typename D, typename ... Args>
+	ssize_t TransferOut(F transfer_function, D destination, Args&& ... args)
+	{
+		return TransferOutWithLimit<A>(transfer_function, std::numeric_limits<size_t>::max(), destination, std::forward<Args>(args)...);
+	}
+
 	/**
 	 * Transfer data from the given function into the packet. It starts writing at the
 	 * position the last transfer stopped.
diff --git a/src/network/core/tcp.cpp b/src/network/core/tcp.cpp
index aa1e1cbeda..ab18f47a87 100644
--- a/src/network/core/tcp.cpp
+++ b/src/network/core/tcp.cpp
@@ -103,7 +103,7 @@ SendPacketsState NetworkTCPSocketHandler::SendPackets(bool closing_down)
 
 	p = this->packet_queue;
 	while (p != nullptr) {
-		res = send(this->sock, (const char*)p->buffer + p->pos, p->size - p->pos, 0);
+		res = p->TransferOut<int>(send, this->sock, 0);
 		if (res == -1) {
 			int err = GET_LAST_ERROR();
 			if (err != EWOULDBLOCK) {
@@ -122,10 +122,8 @@ SendPacketsState NetworkTCPSocketHandler::SendPackets(bool closing_down)
 			return SPS_CLOSED;
 		}
 
-		p->pos += res;
-
 		/* Is this packet sent? */
-		if (p->pos == p->size) {
+		if (p->RemainingBytesToTransfer() == 0) {
 			/* Go to the next packet */
 			this->packet_queue = p->next;
 			delete p;
diff --git a/src/network/core/tcp_listen.h b/src/network/core/tcp_listen.h
index 1f073aa735..53a3d57cc9 100644
--- a/src/network/core/tcp_listen.h
+++ b/src/network/core/tcp_listen.h
@@ -63,7 +63,7 @@ public:
 
 					DEBUG(net, 1, "[%s] Banned ip tried to join (%s), refused", Tsocket::GetName(), entry.c_str());
 
-					if (send(s, (const char*)p.buffer, p.size, 0) < 0) {
+					if (p.TransferOut<int>(send, s, 0) < 0) {
 						DEBUG(net, 0, "send failed with error %d", GET_LAST_ERROR());
 					}
 					closesocket(s);
@@ -80,7 +80,7 @@ public:
 				Packet p(Tfull_packet);
 				p.PrepareToSend();
 
-				if (send(s, (const char*)p.buffer, p.size, 0) < 0) {
+				if (p.TransferOut<int>(send, s, 0) < 0) {
 					DEBUG(net, 0, "send failed with error %d", GET_LAST_ERROR());
 				}
 				closesocket(s);
diff --git a/src/network/core/udp.cpp b/src/network/core/udp.cpp
index 398f53142b..8e476f4e2b 100644
--- a/src/network/core/udp.cpp
+++ b/src/network/core/udp.cpp
@@ -99,7 +99,7 @@ void NetworkUDPSocketHandler::SendPacket(Packet *p, NetworkAddress *recv, bool a
 		}
 
 		/* Send the buffer */
-		int res = sendto(s.second, (const char*)p->buffer, p->size, 0, (const struct sockaddr *)send.GetAddress(), send.GetAddressLength());
+		ssize_t res = p->TransferOut<int>(sendto, s.second, 0, (const struct sockaddr *)send.GetAddress(), send.GetAddressLength());
 		DEBUG(net, 7, "[udp] sendto(%s)", send.GetAddressAsString().c_str());
 
 		/* Check for any errors, but ignore it otherwise */
diff --git a/src/network/network_client.cpp b/src/network/network_client.cpp
index 10b4fd1411..6156dc4863 100644
--- a/src/network/network_client.cpp
+++ b/src/network/network_client.cpp
@@ -35,7 +35,6 @@
 
 /* This file handles all the client-commands */
 
-
 /** Read some packets, and when do use that data as initial load filter. */
 struct PacketReader : LoadFilter {
 	static const size_t CHUNK = 32 * 1024;  ///< 32 KiB chunks of memory.
@@ -59,35 +58,38 @@ struct PacketReader : LoadFilter {
 		}
 	}
 
+	/**
+	 * Simple wrapper around fwrite to be able to pass it to Packet's TransferOut.
+	 * @param destination The reader to add the data to.
+	 * @param source      The buffer to read data from.
+	 * @param amount      The number of bytes to copy.
+	 * @return The number of bytes that were copied.
+	 */
+	static inline ssize_t TransferOutMemCopy(PacketReader *destination, const char *source, size_t amount)
+	{
+		memcpy(destination->buf, source, amount);
+		destination->buf += amount;
+		destination->written_bytes += amount;
+		return amount;
+	}
+
 	/**
 	 * Add a packet to this buffer.
 	 * @param p The packet to add.
 	 */
-	void AddPacket(const Packet *p)
+	void AddPacket(Packet *p)
 	{
 		assert(this->read_bytes == 0);
-
-		size_t in_packet = p->size - p->pos;
-		size_t to_write  = std::min<size_t>(this->bufe - this->buf, in_packet);
-		const byte *pbuf = p->buffer + p->pos;
-
-		this->written_bytes += in_packet;
-		if (to_write != 0) {
-			memcpy(this->buf, pbuf, to_write);
-			this->buf += to_write;
-		}
+		p->TransferOutWithLimit(TransferOutMemCopy, this->bufe - this->buf, this);
 
 		/* Did everything fit in the current chunk, then we're done. */
-		if (to_write == in_packet) return;
+		if (p->RemainingBytesToTransfer() == 0) return;
 
 		/* Allocate a new chunk and add the remaining data. */
-		pbuf += to_write;
-		to_write   = in_packet - to_write;
 		this->blocks.push_back(this->buf = CallocT<byte>(CHUNK));
 		this->bufe = this->buf + CHUNK;
 
-		memcpy(this->buf, pbuf, to_write);
-		this->buf += to_write;
+		p->TransferOutWithLimit(TransferOutMemCopy, this->bufe - this->buf, this);
 	}
 
 	size_t Read(byte *rbuf, size_t size) override
diff --git a/src/network/network_content.cpp b/src/network/network_content.cpp
index 0220f890b3..5292252354 100644
--- a/src/network/network_content.cpp
+++ b/src/network/network_content.cpp
@@ -459,6 +459,18 @@ static bool GunzipFile(const ContentInfo *ci)
 #endif /* defined(WITH_ZLIB) */
 }
 
+/**
+ * Simple wrapper around fwrite to be able to pass it to Packet's TransferOut.
+ * @param file   The file to write data to.
+ * @param buffer The buffer to write to the file.
+ * @param amount The number of bytes to write.
+ * @return The number of bytes that were written.
+ */
+static inline ssize_t TransferOutFWrite(FILE *file, const char *buffer, size_t amount)
+{
+	return fwrite(buffer, 1, amount, file);
+}
+
 bool ClientNetworkContentSocketHandler::Receive_SERVER_CONTENT(Packet *p)
 {
 	if (this->curFile == nullptr) {
@@ -476,8 +488,8 @@ bool ClientNetworkContentSocketHandler::Receive_SERVER_CONTENT(Packet *p)
 		}
 	} else {
 		/* We have a file opened, thus are downloading internal content */
-		size_t toRead = (size_t)(p->size - p->pos);
-		if (fwrite(p->buffer + p->pos, 1, toRead, this->curFile) != toRead) {
+		size_t toRead = p->RemainingBytesToTransfer();
+		if (toRead != 0 && (size_t)p->TransferOut(TransferOutFWrite, this->curFile) != toRead) {
 			DeleteWindowById(WC_NETWORK_STATUS_WINDOW, WN_NETWORK_STATUS_WINDOW_CONTENT_DOWNLOAD);
 			ShowErrorMessage(STR_CONTENT_ERROR_COULD_NOT_DOWNLOAD, STR_CONTENT_ERROR_COULD_NOT_DOWNLOAD_FILE_NOT_WRITABLE, WL_ERROR);
 			this->Close();

From 38d15fc9b788e2c904705d2ba8de4d5f1ff7988d Mon Sep 17 00:00:00 2001
From: Rubidium <rubidium@openttd.org>
Date: Sun, 18 Apr 2021 10:27:04 +0200
Subject: [PATCH 05/10] Codechange: move the logic shrinking of the packets
 into the Packet itself

---
 src/network/core/packet.cpp | 5 +++++
 src/network/core/tcp.cpp    | 5 -----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/network/core/packet.cpp b/src/network/core/packet.cpp
index 5cb6716bca..6e6bb51c0a 100644
--- a/src/network/core/packet.cpp
+++ b/src/network/core/packet.cpp
@@ -73,6 +73,11 @@ void Packet::PrepareToSend()
 	this->buffer[1] = GB(this->size, 8, 8);
 
 	this->pos  = 0; // We start reading from here
+
+	/* Reallocate the packet as in 99+% of the times we send at most 25 bytes and
+	 * keeping the other 1400+ bytes wastes memory, especially when someone tries
+	 * to do a denial of service attack! */
+	this->buffer = ReallocT(this->buffer, this->size);
 }
 
 /**
diff --git a/src/network/core/tcp.cpp b/src/network/core/tcp.cpp
index ab18f47a87..c779beb966 100644
--- a/src/network/core/tcp.cpp
+++ b/src/network/core/tcp.cpp
@@ -65,11 +65,6 @@ void NetworkTCPSocketHandler::SendPacket(Packet *packet)
 
 	packet->PrepareToSend();
 
-	/* Reallocate the packet as in 99+% of the times we send at most 25 bytes and
-	 * keeping the other 1400+ bytes wastes memory, especially when someone tries
-	 * to do a denial of service attack! */
-	packet->buffer = ReallocT(packet->buffer, packet->size);
-
 	/* Locate last packet buffered for the client */
 	p = this->packet_queue;
 	if (p == nullptr) {

From 6f161f655942f2ca0091a75cdab8e3260e31bb5f Mon Sep 17 00:00:00 2001
From: Rubidium <rubidium@openttd.org>
Date: Sun, 18 Apr 2021 10:49:12 +0200
Subject: [PATCH 06/10] Codechange: encapsulate the logic about how many bytes
 can be sent from a buffer in to a Packet

---
 src/network/core/packet.cpp    | 15 +++++++++++++++
 src/network/core/packet.h      | 15 ++++++++-------
 src/network/network_server.cpp |  8 +++-----
 3 files changed, 26 insertions(+), 12 deletions(-)

diff --git a/src/network/core/packet.cpp b/src/network/core/packet.cpp
index 6e6bb51c0a..c033aec98a 100644
--- a/src/network/core/packet.cpp
+++ b/src/network/core/packet.cpp
@@ -175,6 +175,21 @@ void Packet::Send_string(const char *data)
 	while ((this->buffer[this->size++] = *data++) != '\0') {}
 }
 
+/**
+ * Send as many of the bytes as possible in the packet. This can mean
+ * that it is possible that not all bytes are sent. To cope with this
+ * the function returns the amount of bytes that were actually sent.
+ * @param begin The begin of the buffer to send.
+ * @param end   The end of the buffer to send.
+ * @return The number of bytes that were added to this packet.
+ */
+size_t Packet::Send_bytes(const byte *begin, const byte *end)
+{
+	size_t amount = std::min<size_t>(end - begin, SEND_MTU - this->size);
+	memcpy(this->buffer + this->size, begin, amount);
+	this->size += static_cast<PacketSize>(amount);
+	return amount;
+}
 
 /*
  * Receiving commands
diff --git a/src/network/core/packet.h b/src/network/core/packet.h
index b091d8a7ec..4eb4703c19 100644
--- a/src/network/core/packet.h
+++ b/src/network/core/packet.h
@@ -65,13 +65,14 @@ public:
 	/* Sending/writing of packets */
 	void PrepareToSend();
 
-	bool CanWriteToPacket(size_t bytes_to_write);
-	void Send_bool  (bool   data);
-	void Send_uint8 (uint8  data);
-	void Send_uint16(uint16 data);
-	void Send_uint32(uint32 data);
-	void Send_uint64(uint64 data);
-	void Send_string(const char *data);
+	bool   CanWriteToPacket(size_t bytes_to_write);
+	void   Send_bool  (bool   data);
+	void   Send_uint8 (uint8  data);
+	void   Send_uint16(uint16 data);
+	void   Send_uint32(uint32 data);
+	void   Send_uint64(uint64 data);
+	void   Send_string(const char *data);
+	size_t Send_bytes (const byte *begin, const byte *end);
 
 	/* Reading/receiving of packets */
 	bool HasPacketSizeData() const;
diff --git a/src/network/network_server.cpp b/src/network/network_server.cpp
index fbde713eb4..9b77a57afe 100644
--- a/src/network/network_server.cpp
+++ b/src/network/network_server.cpp
@@ -174,12 +174,10 @@ struct PacketWriter : SaveFilter {
 
 		byte *bufe = buf + size;
 		while (buf != bufe) {
-			size_t to_write = std::min<size_t>(SEND_MTU - this->current->size, bufe - buf);
-			memcpy(this->current->buffer + this->current->size, buf, to_write);
-			this->current->size += (PacketSize)to_write;
-			buf += to_write;
+			size_t written = this->current->Send_bytes(buf, bufe);
+			buf += written;
 
-			if (this->current->size == SEND_MTU) {
+			if (!this->current->CanWriteToPacket(1)) {
 				this->AppendQueue();
 				if (buf != bufe) this->current = new Packet(PACKET_SERVER_MAP_DATA);
 			}

From f71fb0f54af443cee37d3c41a0d4f39a24617741 Mon Sep 17 00:00:00 2001
From: Rubidium <rubidium@openttd.org>
Date: Sun, 18 Apr 2021 12:29:34 +0200
Subject: [PATCH 07/10] Codechange: encapsulate reading the size of a Packet

---
 src/network/core/packet.cpp    | 12 ++++++++++++
 src/network/core/packet.h      |  1 +
 src/network/core/udp.cpp       |  2 +-
 src/network/network_server.cpp |  4 ++--
 src/network/network_server.h   |  2 +-
 5 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/src/network/core/packet.cpp b/src/network/core/packet.cpp
index c033aec98a..e32b7fad81 100644
--- a/src/network/core/packet.cpp
+++ b/src/network/core/packet.cpp
@@ -230,6 +230,18 @@ bool Packet::HasPacketSizeData() const
 	return this->pos >= sizeof(PacketSize);
 }
 
+/**
+ * Get the number of bytes in the packet.
+ * When sending a packet this is the size of the data up to that moment.
+ * When receiving a packet (before PrepareToRead) this is the allocated size for the data to be read.
+ * When reading a packet (after PrepareToRead) this is the full size of the packet.
+ * @return The packet's size.
+ */
+size_t Packet::Size() const
+{
+	return this->size;
+}
+
 /**
  * Reads the packet size from the raw packet and stores it in the packet->size
  * @return True iff the packet size seems plausible.
diff --git a/src/network/core/packet.h b/src/network/core/packet.h
index 4eb4703c19..b6a7ff5d31 100644
--- a/src/network/core/packet.h
+++ b/src/network/core/packet.h
@@ -77,6 +77,7 @@ public:
 	/* Reading/receiving of packets */
 	bool HasPacketSizeData() const;
 	bool ParsePacketSize();
+	size_t Size() const;
 	void PrepareToRead();
 
 	bool   CanReadFromPacket(size_t bytes_to_read, bool close_connection = false);
diff --git a/src/network/core/udp.cpp b/src/network/core/udp.cpp
index 8e476f4e2b..3bd2151fe1 100644
--- a/src/network/core/udp.cpp
+++ b/src/network/core/udp.cpp
@@ -137,7 +137,7 @@ void NetworkUDPSocketHandler::ReceivePackets()
 
 			/* If the size does not match the packet must be corrupted.
 			 * Otherwise it will be marked as corrupted later on. */
-			if (!p.ParsePacketSize() || nbytes != p.size) {
+			if (!p.ParsePacketSize() || (size_t)nbytes != p.Size()) {
 				DEBUG(net, 1, "received a packet with mismatching size from %s", address.GetAddressAsString().c_str());
 				continue;
 			}
diff --git a/src/network/network_server.cpp b/src/network/network_server.cpp
index 9b77a57afe..5301bd084c 100644
--- a/src/network/network_server.cpp
+++ b/src/network/network_server.cpp
@@ -247,7 +247,7 @@ Packet *ServerNetworkGameSocketHandler::ReceivePacket()
 	/* We can receive a packet, so try that and if needed account for
 	 * the amount of received data. */
 	Packet *p = this->NetworkTCPSocketHandler::ReceivePacket();
-	if (p != nullptr) this->receive_limit -= p->size;
+	if (p != nullptr) this->receive_limit -= p->Size();
 	return p;
 }
 
@@ -1832,7 +1832,7 @@ void NetworkServer_Tick(bool send_frame)
 	for (NetworkClientSocket *cs : NetworkClientSocket::Iterate()) {
 		/* We allow a number of bytes per frame, but only to the burst amount
 		 * to be available for packet receiving at any particular time. */
-		cs->receive_limit = std::min<int>(cs->receive_limit + _settings_client.network.bytes_per_frame,
+		cs->receive_limit = std::min<size_t>(cs->receive_limit + _settings_client.network.bytes_per_frame,
 				_settings_client.network.bytes_per_frame_burst);
 
 		/* Check if the speed of the client is what we can expect from a client */
diff --git a/src/network/network_server.h b/src/network/network_server.h
index 77612fdc8c..4f6033fab1 100644
--- a/src/network/network_server.h
+++ b/src/network/network_server.h
@@ -67,7 +67,7 @@ public:
 	uint32 last_token_frame;     ///< The last frame we received the right token
 	ClientStatus status;         ///< Status of this client
 	CommandQueue outgoing_queue; ///< The command-queue awaiting delivery
-	int receive_limit;           ///< Amount of bytes that we can receive at this moment
+	size_t receive_limit;        ///< Amount of bytes that we can receive at this moment
 
 	struct PacketWriter *savegame; ///< Writer used to write the savegame.
 	NetworkAddress client_address; ///< IP-address of the client (so he can be banned)

From 3abefdf56190ef55d8680acb1aeab9f1b2fc8108 Mon Sep 17 00:00:00 2001
From: Rubidium <rubidium@openttd.org>
Date: Tue, 20 Apr 2021 18:50:46 +0200
Subject: [PATCH 08/10] Codechange: remove public access to the next pointer in
 Packet

---
 src/network/core/packet.cpp    | 26 ++++++++++++++++++++++++++
 src/network/core/packet.h      |  3 +++
 src/network/core/tcp.cpp       | 24 ++++--------------------
 src/network/network_server.cpp | 20 +++++---------------
 4 files changed, 38 insertions(+), 35 deletions(-)

diff --git a/src/network/core/packet.cpp b/src/network/core/packet.cpp
index e32b7fad81..9e9ce69012 100644
--- a/src/network/core/packet.cpp
+++ b/src/network/core/packet.cpp
@@ -62,6 +62,32 @@ Packet::~Packet()
 	free(this->buffer);
 }
 
+/**
+ * Add the given Packet to the end of the queue of packets.
+ * @param queue  The pointer to the begin of the queue.
+ * @param packet The packet to append to the queue.
+ */
+/* static */ void Packet::AddToQueue(Packet **queue, Packet *packet)
+{
+	while (*queue != nullptr) queue = &(*queue)->next;
+	*queue = packet;
+}
+
+/**
+ * Pop the packet from the begin of the queue and set the
+ * begin of the queue to the second element in the queue.
+ * @param queue  The pointer to the begin of the queue.
+ * @return The Packet that used to be a the begin of the queue.
+ */
+/* static */ Packet *Packet::PopFromQueue(Packet **queue)
+{
+	Packet *p = *queue;
+	*queue = p->next;
+	p->next = nullptr;
+	return p;
+}
+
+
 /**
  * Writes the packet size from the raw packet from packet->size
  */
diff --git a/src/network/core/packet.h b/src/network/core/packet.h
index b6a7ff5d31..d7ab7fee63 100644
--- a/src/network/core/packet.h
+++ b/src/network/core/packet.h
@@ -62,6 +62,9 @@ public:
 	Packet(PacketType type);
 	~Packet();
 
+	static void AddToQueue(Packet **queue, Packet *packet);
+	static Packet *PopFromQueue(Packet **queue);
+
 	/* Sending/writing of packets */
 	void PrepareToSend();
 
diff --git a/src/network/core/tcp.cpp b/src/network/core/tcp.cpp
index c779beb966..a749b6195b 100644
--- a/src/network/core/tcp.cpp
+++ b/src/network/core/tcp.cpp
@@ -42,9 +42,7 @@ NetworkRecvStatus NetworkTCPSocketHandler::CloseConnection(bool error)
 
 	/* Free all pending and partially received packets */
 	while (this->packet_queue != nullptr) {
-		Packet *p = this->packet_queue->next;
-		delete this->packet_queue;
-		this->packet_queue = p;
+		delete Packet::PopFromQueue(&this->packet_queue);
 	}
 	delete this->packet_recv;
 	this->packet_recv = nullptr;
@@ -60,21 +58,10 @@ NetworkRecvStatus NetworkTCPSocketHandler::CloseConnection(bool error)
  */
 void NetworkTCPSocketHandler::SendPacket(Packet *packet)
 {
-	Packet *p;
 	assert(packet != nullptr);
 
 	packet->PrepareToSend();
-
-	/* Locate last packet buffered for the client */
-	p = this->packet_queue;
-	if (p == nullptr) {
-		/* No packets yet */
-		this->packet_queue = packet;
-	} else {
-		/* Skip to the last packet */
-		while (p->next != nullptr) p = p->next;
-		p->next = packet;
-	}
+	Packet::AddToQueue(&this->packet_queue, packet);
 }
 
 /**
@@ -96,8 +83,7 @@ SendPacketsState NetworkTCPSocketHandler::SendPackets(bool closing_down)
 	if (!this->writable) return SPS_NONE_SENT;
 	if (!this->IsConnected()) return SPS_CLOSED;
 
-	p = this->packet_queue;
-	while (p != nullptr) {
+	while ((p = this->packet_queue) != nullptr) {
 		res = p->TransferOut<int>(send, this->sock, 0);
 		if (res == -1) {
 			int err = GET_LAST_ERROR();
@@ -120,9 +106,7 @@ SendPacketsState NetworkTCPSocketHandler::SendPackets(bool closing_down)
 		/* Is this packet sent? */
 		if (p->RemainingBytesToTransfer() == 0) {
 			/* Go to the next packet */
-			this->packet_queue = p->next;
-			delete p;
-			p = this->packet_queue;
+			delete Packet::PopFromQueue(&this->packet_queue);
 		} else {
 			return SPS_PARTLY_SENT;
 		}
diff --git a/src/network/network_server.cpp b/src/network/network_server.cpp
index 5301bd084c..80a9c56a0c 100644
--- a/src/network/network_server.cpp
+++ b/src/network/network_server.cpp
@@ -79,9 +79,7 @@ struct PacketWriter : SaveFilter {
 		/* This must all wait until the Destroy function is called. */
 
 		while (this->packets != nullptr) {
-			Packet *p = this->packets->next;
-			delete this->packets;
-			this->packets = p;
+			delete Packet::PopFromQueue(&this->packets);
 		}
 
 		delete this->current;
@@ -132,11 +130,7 @@ struct PacketWriter : SaveFilter {
 	{
 		std::lock_guard<std::mutex> lock(this->mutex);
 
-		Packet *p = this->packets;
-		this->packets = p->next;
-		p->next = nullptr;
-
-		return p;
+		return Packet::PopFromQueue(&this->packets);
 	}
 
 	/** Append the current packet to the queue. */
@@ -144,12 +138,7 @@ struct PacketWriter : SaveFilter {
 	{
 		if (this->current == nullptr) return;
 
-		Packet **p = &this->packets;
-		while (*p != nullptr) {
-			p = &(*p)->next;
-		}
-		*p = this->current;
-
+		Packet::AddToQueue(&this->packets, this->current);
 		this->current = nullptr;
 	}
 
@@ -158,7 +147,8 @@ struct PacketWriter : SaveFilter {
 	{
 		if (this->current == nullptr) return;
 
-		this->current->next = this->packets;
+		/* Reversed from AppendQueue so the queue gets added to the current one. */
+		Packet::AddToQueue(&this->current, this->packets);
 		this->packets = this->current;
 		this->current = nullptr;
 	}

From 450178d780eb885717c53a2dad62587332efc0f4 Mon Sep 17 00:00:00 2001
From: Rubidium <rubidium@openttd.org>
Date: Wed, 21 Apr 2021 07:10:09 +0200
Subject: [PATCH 09/10] Codechange: add accessor for the packet type to Packet
 and make the internal state of Packet private

---
 src/network/core/packet.cpp    | 10 ++++++++++
 src/network/core/packet.h      |  4 ++--
 src/network/network_server.cpp |  2 +-
 3 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/src/network/core/packet.cpp b/src/network/core/packet.cpp
index 9e9ce69012..d534df4d0d 100644
--- a/src/network/core/packet.cpp
+++ b/src/network/core/packet.cpp
@@ -296,6 +296,16 @@ void Packet::PrepareToRead()
 	this->pos = sizeof(PacketSize);
 }
 
+/**
+ * Get the \c PacketType from this packet.
+ * @return The packet type.
+ */
+PacketType Packet::GetPacketType() const
+{
+	assert(this->Size() >= sizeof(PacketSize) + sizeof(PacketType));
+	return static_cast<PacketType>(buffer[sizeof(PacketSize)]);
+}
+
 /**
  * Read a boolean from the packet.
  * @return The read data.
diff --git a/src/network/core/packet.h b/src/network/core/packet.h
index d7ab7fee63..1a9c9faea6 100644
--- a/src/network/core/packet.h
+++ b/src/network/core/packet.h
@@ -40,6 +40,7 @@ typedef uint8  PacketType; ///< Identifier for the packet
  *     (year % 4 == 0) and ((year % 100 != 0) or (year % 400 == 0))
  */
 struct Packet {
+private:
 	/** The next packet. Used for queueing packets before sending. */
 	Packet *next;
 	/**
@@ -52,8 +53,6 @@ struct Packet {
 	PacketSize pos;
 	/** The buffer of this packet, of basically variable length up to SEND_MTU. */
 	byte *buffer;
-
-private:
 	/** Socket we're associated with. */
 	NetworkSocketHandler *cs;
 
@@ -82,6 +81,7 @@ public:
 	bool ParsePacketSize();
 	size_t Size() const;
 	void PrepareToRead();
+	PacketType GetPacketType() const;
 
 	bool   CanReadFromPacket(size_t bytes_to_read, bool close_connection = false);
 	bool   Recv_bool  ();
diff --git a/src/network/network_server.cpp b/src/network/network_server.cpp
index 80a9c56a0c..a0d1a00666 100644
--- a/src/network/network_server.cpp
+++ b/src/network/network_server.cpp
@@ -626,7 +626,7 @@ NetworkRecvStatus ServerNetworkGameSocketHandler::SendMap()
 
 		for (uint i = 0; (has_packets = this->savegame->HasPackets()) && i < sent_packets; i++) {
 			Packet *p = this->savegame->PopPacket();
-			last_packet = p->buffer[2] == PACKET_SERVER_MAP_DONE;
+			last_packet = p->GetPacketType() == PACKET_SERVER_MAP_DONE;
 
 			this->SendPacket(p);
 

From 75386873b77df4f3493edd5bbba92b33007bdd21 Mon Sep 17 00:00:00 2001
From: Rubidium <rubidium@openttd.org>
Date: Sun, 18 Apr 2021 12:36:19 +0200
Subject: [PATCH 10/10] Codechange: use std::vector instead of a fixed size
 array for Packets

---
 src/network/core/packet.cpp | 98 +++++++++++++++----------------------
 src/network/core/packet.h   | 16 ++----
 2 files changed, 44 insertions(+), 70 deletions(-)

diff --git a/src/network/core/packet.cpp b/src/network/core/packet.cpp
index d534df4d0d..428bc65ba4 100644
--- a/src/network/core/packet.cpp
+++ b/src/network/core/packet.cpp
@@ -27,39 +27,23 @@
  *                          loose some the data of the packet, so there you pass the maximum
  *                          size for the packet you expect from the network.
  */
-Packet::Packet(NetworkSocketHandler *cs, size_t initial_read_size)
+Packet::Packet(NetworkSocketHandler *cs, size_t initial_read_size) : next(nullptr), pos(0)
 {
 	assert(cs != nullptr);
 
-	this->cs     = cs;
-	this->next   = nullptr;
-	this->pos    = 0; // We start reading from here
-	this->size   = static_cast<int>(initial_read_size);
-	this->buffer = MallocT<byte>(SEND_MTU);
+	this->cs = cs;
+	this->buffer.resize(initial_read_size);
 }
 
 /**
  * Creates a packet to send
  * @param type of the packet to send
  */
-Packet::Packet(PacketType type)
+Packet::Packet(PacketType type) : next(nullptr), pos(0), cs(nullptr)
 {
-	this->cs                   = nullptr;
-	this->next                 = nullptr;
-
-	/* Skip the size so we can write that in before sending the packet */
-	this->pos                  = 0;
-	this->size                 = sizeof(PacketSize);
-	this->buffer               = MallocT<byte>(SEND_MTU);
-	this->buffer[this->size++] = type;
-}
-
-/**
- * Free the buffer of this packet.
- */
-Packet::~Packet()
-{
-	free(this->buffer);
+	/* Allocate space for the the size so we can write that in just before sending the packet. */
+	this->Send_uint16(0);
+	this->Send_uint8(type);
 }
 
 /**
@@ -95,15 +79,11 @@ void Packet::PrepareToSend()
 {
 	assert(this->cs == nullptr && this->next == nullptr);
 
-	this->buffer[0] = GB(this->size, 0, 8);
-	this->buffer[1] = GB(this->size, 8, 8);
+	this->buffer[0] = GB(this->Size(), 0, 8);
+	this->buffer[1] = GB(this->Size(), 8, 8);
 
 	this->pos  = 0; // We start reading from here
-
-	/* Reallocate the packet as in 99+% of the times we send at most 25 bytes and
-	 * keeping the other 1400+ bytes wastes memory, especially when someone tries
-	 * to do a denial of service attack! */
-	this->buffer = ReallocT(this->buffer, this->size);
+	this->buffer.shrink_to_fit();
 }
 
 /**
@@ -113,7 +93,7 @@ void Packet::PrepareToSend()
  */
 bool Packet::CanWriteToPacket(size_t bytes_to_write)
 {
-	return this->size + bytes_to_write < SEND_MTU;
+	return this->Size() + bytes_to_write < SEND_MTU;
 }
 
 /*
@@ -144,7 +124,7 @@ void Packet::Send_bool(bool data)
 void Packet::Send_uint8(uint8 data)
 {
 	assert(this->CanWriteToPacket(sizeof(data)));
-	this->buffer[this->size++] = data;
+	this->buffer.emplace_back(data);
 }
 
 /**
@@ -154,8 +134,8 @@ void Packet::Send_uint8(uint8 data)
 void Packet::Send_uint16(uint16 data)
 {
 	assert(this->CanWriteToPacket(sizeof(data)));
-	this->buffer[this->size++] = GB(data, 0, 8);
-	this->buffer[this->size++] = GB(data, 8, 8);
+	this->buffer.emplace_back(GB(data, 0, 8));
+	this->buffer.emplace_back(GB(data, 8, 8));
 }
 
 /**
@@ -165,10 +145,10 @@ void Packet::Send_uint16(uint16 data)
 void Packet::Send_uint32(uint32 data)
 {
 	assert(this->CanWriteToPacket(sizeof(data)));
-	this->buffer[this->size++] = GB(data,  0, 8);
-	this->buffer[this->size++] = GB(data,  8, 8);
-	this->buffer[this->size++] = GB(data, 16, 8);
-	this->buffer[this->size++] = GB(data, 24, 8);
+	this->buffer.emplace_back(GB(data,  0, 8));
+	this->buffer.emplace_back(GB(data,  8, 8));
+	this->buffer.emplace_back(GB(data, 16, 8));
+	this->buffer.emplace_back(GB(data, 24, 8));
 }
 
 /**
@@ -178,14 +158,14 @@ void Packet::Send_uint32(uint32 data)
 void Packet::Send_uint64(uint64 data)
 {
 	assert(this->CanWriteToPacket(sizeof(data)));
-	this->buffer[this->size++] = GB(data,  0, 8);
-	this->buffer[this->size++] = GB(data,  8, 8);
-	this->buffer[this->size++] = GB(data, 16, 8);
-	this->buffer[this->size++] = GB(data, 24, 8);
-	this->buffer[this->size++] = GB(data, 32, 8);
-	this->buffer[this->size++] = GB(data, 40, 8);
-	this->buffer[this->size++] = GB(data, 48, 8);
-	this->buffer[this->size++] = GB(data, 56, 8);
+	this->buffer.emplace_back(GB(data,  0, 8));
+	this->buffer.emplace_back(GB(data,  8, 8));
+	this->buffer.emplace_back(GB(data, 16, 8));
+	this->buffer.emplace_back(GB(data, 24, 8));
+	this->buffer.emplace_back(GB(data, 32, 8));
+	this->buffer.emplace_back(GB(data, 40, 8));
+	this->buffer.emplace_back(GB(data, 48, 8));
+	this->buffer.emplace_back(GB(data, 56, 8));
 }
 
 /**
@@ -198,7 +178,7 @@ void Packet::Send_string(const char *data)
 	assert(data != nullptr);
 	/* Length of the string + 1 for the '\0' termination. */
 	assert(this->CanWriteToPacket(strlen(data) + 1));
-	while ((this->buffer[this->size++] = *data++) != '\0') {}
+	while (this->buffer.emplace_back(*data++) != '\0') {}
 }
 
 /**
@@ -211,9 +191,8 @@ void Packet::Send_string(const char *data)
  */
 size_t Packet::Send_bytes(const byte *begin, const byte *end)
 {
-	size_t amount = std::min<size_t>(end - begin, SEND_MTU - this->size);
-	memcpy(this->buffer + this->size, begin, amount);
-	this->size += static_cast<PacketSize>(amount);
+	size_t amount = std::min<size_t>(end - begin, SEND_MTU - this->Size());
+	this->buffer.insert(this->buffer.end(), begin, begin + amount);
 	return amount;
 }
 
@@ -238,7 +217,7 @@ bool Packet::CanReadFromPacket(size_t bytes_to_read, bool close_connection)
 	if (this->cs->HasClientQuit()) return false;
 
 	/* Check if variable is within packet-size */
-	if (this->pos + bytes_to_read > this->size) {
+	if (this->pos + bytes_to_read > this->Size()) {
 		if (close_connection) this->cs->NetworkSocketHandler::CloseConnection();
 		return false;
 	}
@@ -265,7 +244,7 @@ bool Packet::HasPacketSizeData() const
  */
 size_t Packet::Size() const
 {
-	return this->size;
+	return this->buffer.size();
 }
 
 /**
@@ -275,14 +254,15 @@ size_t Packet::Size() const
 bool Packet::ParsePacketSize()
 {
 	assert(this->cs != nullptr && this->next == nullptr);
-	this->size  = (PacketSize)this->buffer[0];
-	this->size += (PacketSize)this->buffer[1] << 8;
+	size_t size = (size_t)this->buffer[0];
+	size       += (size_t)this->buffer[1] << 8;
 
 	/* If the size of the packet is less than the bytes required for the size and type of
 	 * the packet, or more than the allowed limit, then something is wrong with the packet.
 	 * In those cases the packet can generally be regarded as containing garbage data. */
-	if (this->size < sizeof(PacketSize) + sizeof(PacketType) || this->size > SEND_MTU) return false;
+	if (size < sizeof(PacketSize) + sizeof(PacketType) || size > SEND_MTU) return false;
 
+	this->buffer.resize(size);
 	this->pos = sizeof(PacketSize);
 	return true;
 }
@@ -398,13 +378,13 @@ void Packet::Recv_string(char *buffer, size_t size, StringValidationSettings set
 	if (cs->HasClientQuit()) return;
 
 	pos = this->pos;
-	while (--size > 0 && pos < this->size && (*buffer++ = this->buffer[pos++]) != '\0') {}
+	while (--size > 0 && pos < this->Size() && (*buffer++ = this->buffer[pos++]) != '\0') {}
 
-	if (size == 0 || pos == this->size) {
+	if (size == 0 || pos == this->Size()) {
 		*buffer = '\0';
 		/* If size was sooner to zero then the string in the stream
 		 *  skip till the \0, so than packet can be read out correctly for the rest */
-		while (pos < this->size && this->buffer[pos] != '\0') pos++;
+		while (pos < this->Size() && this->buffer[pos] != '\0') pos++;
 		pos++;
 	}
 	this->pos = pos;
@@ -418,5 +398,5 @@ void Packet::Recv_string(char *buffer, size_t size, StringValidationSettings set
  */
 size_t Packet::RemainingBytesToTransfer() const
 {
-	return this->size - this->pos;
+	return this->Size() - this->pos;
 }
diff --git a/src/network/core/packet.h b/src/network/core/packet.h
index 1a9c9faea6..1ac7f18a18 100644
--- a/src/network/core/packet.h
+++ b/src/network/core/packet.h
@@ -43,23 +43,17 @@ struct Packet {
 private:
 	/** The next packet. Used for queueing packets before sending. */
 	Packet *next;
-	/**
-	 * The size of the whole packet for received packets. For packets
-	 * that will be sent, the value is filled in just before the
-	 * actual transmission.
-	 */
-	PacketSize size;
 	/** The current read/write position in the packet */
 	PacketSize pos;
-	/** The buffer of this packet, of basically variable length up to SEND_MTU. */
-	byte *buffer;
+	/** The buffer of this packet. */
+	std::vector<byte> buffer;
+
 	/** Socket we're associated with. */
 	NetworkSocketHandler *cs;
 
 public:
 	Packet(NetworkSocketHandler *cs, size_t initial_read_size = sizeof(PacketSize));
 	Packet(PacketType type);
-	~Packet();
 
 	static void AddToQueue(Packet **queue, Packet *packet);
 	static Packet *PopFromQueue(Packet **queue);
@@ -118,7 +112,7 @@ public:
 		assert(this->pos < this->buffer.size());
 		assert(this->pos + amount <= this->buffer.size());
 		/* Making buffer a char means casting a lot in the Recv/Send functions. */
-		const char *output_buffer = reinterpret_cast<const char*>(this->buffer + this->pos);
+		const char *output_buffer = reinterpret_cast<const char*>(this->buffer.data() + this->pos);
 		ssize_t bytes = transfer_function(destination, output_buffer, static_cast<A>(amount), std::forward<Args>(args)...);
 		if (bytes > 0) this->pos += bytes;
 		return bytes;
@@ -183,7 +177,7 @@ public:
 		assert(this->pos < this->buffer.size());
 		assert(this->pos + amount <= this->buffer.size());
 		/* Making buffer a char means casting a lot in the Recv/Send functions. */
-		char *input_buffer = reinterpret_cast<char*>(this->buffer + this->pos);
+		char *input_buffer = reinterpret_cast<char*>(this->buffer.data() + this->pos);
 		ssize_t bytes = transfer_function(source, input_buffer, static_cast<A>(amount), std::forward<Args>(args)...);
 		if (bytes > 0) this->pos += bytes;
 		return bytes;