Use std::string for CommandContainer text instead of giant static buffer

Use move semantics for CommandContainer instance where feasible

src/command.cpp

@@ -630,7 +630,7 @@ static int _docommand_recursive = 0;
  */
 CommandCost DoCommand(const CommandContainer *container, DoCommandFlag flags)
 {
-	return DoCommand(container->tile, container->p1, container->p2, flags, container->cmd & CMD_ID_MASK, container->text);
+	return DoCommand(container->tile, container->p1, container->p2, flags, container->cmd & CMD_ID_MASK, container->text.c_str());
 }
 
 /*!
@@ -724,7 +724,7 @@ Money GetAvailableMoneyForCommand()
  */
 bool DoCommandP(const CommandContainer *container, bool my_cmd)
 {
-	return DoCommandP(container->tile, container->p1, container->p2, container->cmd, container->callback, container->text, my_cmd);
+	return DoCommandP(container->tile, container->p1, container->p2, container->cmd, container->callback, container->text.c_str(), my_cmd);
 }
 
 /*!

src/command_type.h

@@ -15,6 +15,7 @@
 #include "economy_type.h"
 #include "strings_type.h"
 #include "tile_type.h"
+#include <string>
 
 struct GRFFile;
 
@@ -550,7 +551,7 @@ struct CommandContainer {
 	uint32 cmd;                      ///< command being executed.
 	CommandCallback *callback;       ///< any callback function executed upon successful completion of the command.
 	uint32 binary_length;            ///< in case text contains binary data, this describes its length.
-	char text[MAX_CMD_TEXT_LENGTH];  ///< possible text sent for name changes etc, in bytes including '\0'.
+	std::string text;                ///< possible text sent for name changes etc.
 };
 
 #endif /* COMMAND_TYPE_H */

src/network/core/packet.cpp

@@ -330,10 +330,26 @@ void Packet::Recv_string(char *buffer, size_t size, StringValidationSettings set
 	str_validate(bufp, last, settings);
 }
 
+/**
+ * Reads a string till it finds a '\0' in the stream.
+ * @param buffer The buffer to put the data into.
+ * @param settings The string validation settings.
+ */
+void Packet::Recv_string(std::string &buffer, StringValidationSettings settings)
+{
+	/* Don't allow reading from a closed socket */
+	if (cs->HasClientQuit()) return;
+
+	size_t length = ttd_strnlen((const char *)(this->buffer + this->pos), this->size - this->pos - 1);
+	buffer.assign((const char *)(this->buffer + this->pos), length);
+	this->pos += length + 1;
+	str_validate(const_cast<char *>(buffer.c_str()), buffer.c_str() + buffer.size(), settings);
+}
+
 /**
  * Reads binary data.
  * @param buffer The buffer to put the data into.
- * @param size   The size of the buffer.
+ * @param size   The size of the data.
  */
 void Packet::Recv_binary(char *buffer, size_t size)
 {
@@ -344,4 +360,18 @@ void Packet::Recv_binary(char *buffer, size_t size)
 	this->pos += (PacketSize) size;
 }
 
+/**
+ * Reads binary data.
+ * @param buffer The buffer to put the data into.
+ * @param size   The size of the data.
+ */
+void Packet::Recv_binary(std::string &buffer, size_t size)
+{
+	/* Don't allow reading from a closed socket */
+	if (cs->HasClientQuit()) return;
+
+	buffer.assign((const char *) &this->buffer[this->pos], size);
+	this->pos += (PacketSize) size;
+}
+
 #endif /* ENABLE_NETWORK */

src/network/core/packet.h

@@ -17,6 +17,7 @@
 #include "config.h"
 #include "core.h"
 #include "../../string_type.h"
+#include <string>
 
 #ifdef ENABLE_NETWORK
 
@@ -88,7 +89,9 @@ public:
 	uint32 Recv_uint32();
 	uint64 Recv_uint64();
 	void   Recv_string(char *buffer, size_t size, StringValidationSettings settings = SVS_REPLACE_WITH_QUESTION_MARK);
+	void   Recv_string(std::string &buffer, StringValidationSettings settings = SVS_REPLACE_WITH_QUESTION_MARK);
 	void   Recv_binary(char *buffer, size_t size);
+	void   Recv_binary(std::string &buffer, size_t size);
 };
 
 #endif /* ENABLE_NETWORK */

src/network/core/tcp_game.h

@@ -18,6 +18,7 @@
 #include "tcp.h"
 #include "../network_type.h"
 #include "../../core/pool_type.hpp"
+#include <memory>
 
 #ifdef ENABLE_NETWORK
 
@@ -135,13 +136,16 @@ class CommandQueue {
 	CommandPacket *last;  ///< The last packet in the queue; only valid when first != NULL.
 	uint count;           ///< The number of items in the queue.
 
+	void Append(CommandPacket *p, bool move);
+
 public:
 	/** Initialise the command queue. */
 	CommandQueue() : first(NULL), last(NULL), count(0) {}
 	/** Clear the command queue. */
 	~CommandQueue() { this->Free(); }
-	void Append(CommandPacket *p);
-	CommandPacket *Pop(bool ignore_paused = false);
+	void Append(CommandPacket &p) { this->Append(&p, false); }
+	void Append(CommandPacket &&p) { this->Append(&p, true); }
+	std::unique_ptr<CommandPacket> Pop(bool ignore_paused = false);
 	CommandPacket *Peek(bool ignore_paused = false);
 	void Free();
 	/** Get the number of items in the queue. */

src/network/network.cpp

@@ -906,7 +906,7 @@ void NetworkGameLoop()
 		static Date next_date = 0;
 		static uint32 next_date_fract;
 		static uint8 next_tick_skip_counter;
-		static CommandPacket *cp = NULL;
+		static std::unique_ptr<CommandPacket> cp;
 		static bool check_sync_state = false;
 		static uint32 sync_state[2];
 		if (f == NULL && next_date == 0) {
@@ -917,10 +917,9 @@ void NetworkGameLoop()
 		while (f != NULL && !feof(f)) {
 			if (_date == next_date && _date_fract == next_date_fract) {
 				if (cp != NULL) {
-					NetworkSendCommand(cp->tile, cp->p1, cp->p2, cp->cmd & ~CMD_FLAGS_MASK, NULL, cp->text, cp->company);
-					DEBUG(net, 0, "injecting: date{%08x; %02x; %02x}; %02x; %06x; %08x; %08x; %08x; \"%s\" (%s)", _date, _date_fract, _tick_skip_counter, (int)_current_company, cp->tile, cp->p1, cp->p2, cp->cmd, cp->text, GetCommandName(cp->cmd));
-					free(cp);
-					cp = NULL;
+					NetworkSendCommand(cp->tile, cp->p1, cp->p2, cp->cmd & ~CMD_FLAGS_MASK, NULL, cp->text.c_str(), cp->company, cp->binary_length);
+					DEBUG(net, 0, "injecting: date{%08x; %02x; %02x}; %02x; %06x; %08x; %08x; %08x; \"%s\" (%x) (%s)", _date, _date_fract, _tick_skip_counter, (int)_current_company, cp->tile, cp->p1, cp->p2, cp->cmd, cp->text.c_str(), cp->binary_length, GetCommandName(cp->cmd));
+					cp.reset();
 				}
 				if (check_sync_state) {
 					if (sync_state[0] == _random.state[0] && sync_state[1] == _random.state[1]) {
@@ -954,24 +953,29 @@ void NetworkGameLoop()
 				) {
 				p += 5;
 				if (*p == ' ') p++;
-				cp = CallocT<CommandPacket>(1);
+				cp.reset(new CommandPacket());
 				int company;
-				int ret = sscanf(p, "date{%x; %x; %x}; %x; %x; %x; %x; %x; \"%[^\"]\"", &next_date, &next_date_fract, &next_tick_skip_counter, &company, &cp->tile, &cp->p1, &cp->p2, &cp->cmd, cp->text);
+				cp->text.resize(MAX_CMD_TEXT_LENGTH);
+				int ret = sscanf(p, "date{%x; %x; %x}; %x; %x; %x; %x; %x; \"%[^\"]\"", &next_date, &next_date_fract, &next_tick_skip_counter, &company, &cp->tile, &cp->p1, &cp->p2, &cp->cmd, const_cast<char *>(cp->text.c_str()));
 				/* There are 9 pieces of data to read, however the last is a
 				 * string that might or might not exist. Ignore it if that
 				 * string misses because in 99% of the time it's not used. */
 				assert(ret == 9 || ret == 8);
 				cp->company = (CompanyID)company;
+				cp->binary_length = 0;
 			} else if (strncmp(p, "join: ", 6) == 0) {
 				/* Manually insert a pause when joining; this way the client can join at the exact right time. */
 				int ret = sscanf(p + 6, "date{%x; %x; %x}", &next_date, &next_date_fract, &next_tick_skip_counter);
 				assert(ret == 3);
 				DEBUG(net, 0, "injecting pause for join at date{%08x; %02x; %02x}; please join when paused", next_date, next_date_fract, next_tick_skip_counter);
-				cp = CallocT<CommandPacket>(1);
+				cp.reset(new CommandPacket());
+				cp->tile = 0;
 				cp->company = COMPANY_SPECTATOR;
 				cp->cmd = CMD_PAUSE;
 				cp->p1 = PM_PAUSED_NORMAL;
 				cp->p2 = 1;
+				cp->callback = NULL;
+				cp->binary_length = 0;
 				_ddc_fastforward = false;
 			} else if (strncmp(p, "sync: ", 6) == 0) {
 				int ret = sscanf(p + 6, "date{%x; %x; %x}; %x; %x", &next_date, &next_date_fract, &next_tick_skip_counter, &sync_state[0], &sync_state[1]);

src/network/network_admin.cpp

@@ -655,7 +655,7 @@ NetworkRecvStatus ServerNetworkAdminSocketHandler::SendCmdLogging(ClientID clien
 	p->Send_uint32(cp->p1);
 	p->Send_uint32(cp->p2);
 	p->Send_uint32(cp->tile);
-	p->Send_string(cp->text);
+	p->Send_string(cp->text.c_str());
 	p->Send_uint32(cp->frame);
 
 	this->SendPacket(p);

src/network/network_client.cpp

@@ -955,7 +955,7 @@ NetworkRecvStatus ClientNetworkGameSocketHandler::Receive_SERVER_COMMAND(Packet
 		return NETWORK_RECV_STATUS_MALFORMED_PACKET;
 	}
 
-	this->incoming_queue.Append(&cp);
+	this->incoming_queue.Append(std::move(cp));
 
 	return NETWORK_RECV_STATUS_OKAY;
 }

src/network/network_command.cpp

@@ -63,10 +63,14 @@ static CommandCallback * const _callback_table[] = {
  * @param p The packet to append to the queue.
  * @note A new instance of the CommandPacket will be made.
  */
-void CommandQueue::Append(CommandPacket *p)
+void CommandQueue::Append(CommandPacket *p, bool move)
 {
-	CommandPacket *add = MallocT<CommandPacket>(1);
-	*add = *p;
+	CommandPacket *add = new CommandPacket();
+	if (move) {
+		*add = std::move(*p);
+	} else {
+		*add = *p;
+	}
 	add->next = NULL;
 	if (this->first == NULL) {
 		this->first = add;
@@ -82,8 +86,9 @@ void CommandQueue::Append(CommandPacket *p)
  * @param ignore_paused Whether to ignore commands that may not be executed while paused.
  * @return the first item in the queue.
  */
-CommandPacket *CommandQueue::Pop(bool ignore_paused)
+std::unique_ptr<CommandPacket> CommandQueue::Pop(bool ignore_paused)
 {
+
 	CommandPacket **prev = &this->first;
 	CommandPacket *ret = this->first;
 	CommandPacket *prev_item = NULL;
@@ -99,7 +104,7 @@ CommandPacket *CommandQueue::Pop(bool ignore_paused)
 		*prev = ret->next;
 		this->count--;
 	}
-	return ret;
+	return std::unique_ptr<CommandPacket>(ret);
 }
 
 /**
@@ -120,10 +125,8 @@ CommandPacket *CommandQueue::Peek(bool ignore_paused)
 /** Free everything that is in the queue. */
 void CommandQueue::Free()
 {
-	CommandPacket *cp;
-	while ((cp = this->Pop()) != NULL) {
-		free(cp);
-	}
+	std::unique_ptr<CommandPacket> cp;
+	while ((cp = this->Pop()) != NULL) {}
 	assert(this->count == 0);
 }
 
@@ -157,9 +160,13 @@ void NetworkSendCommand(TileIndex tile, uint32 p1, uint32 p2, uint32 cmd, Comman
 
 	c.binary_length = binary_length;
 	if (binary_length == 0) {
-		strecpy(c.text, (text != NULL) ? text : "", lastof(c.text));
+		if (text != NULL) {
+			c.text.assign(text);
+		} else {
+			c.text.clear();
+		}
 	} else {
-		memcpy(c.text, text, binary_length);
+		c.text.assign(text, binary_length);
 	}
 
 	if (_network_server) {
@@ -172,7 +179,7 @@ void NetworkSendCommand(TileIndex tile, uint32 p1, uint32 p2, uint32 cmd, Comman
 		c.frame = _frame_counter_max + 1;
 		c.my_cmd = true;
 
-		_local_wait_queue.Append(&c);
+		_local_wait_queue.Append(std::move(c));
 		return;
 	}
 
@@ -196,7 +203,7 @@ void NetworkSyncCommandQueue(NetworkClientSocket *cs)
 	for (CommandPacket *p = _local_execution_queue.Peek(); p != NULL; p = p->next) {
 		CommandPacket c = *p;
 		c.callback = 0;
-		cs->outgoing_queue.Append(&c);
+		cs->outgoing_queue.Append(std::move(c));
 	}
 }
 
@@ -227,7 +234,6 @@ void NetworkExecuteLocalCommandQueue()
 		DoCommandP(cp, cp->my_cmd);
 
 		queue.Pop();
-		free(cp);
 	}
 
 	/* Local company may have changed, so we should not restore the old value */
@@ -260,13 +266,13 @@ static void DistributeCommandPacket(CommandPacket &cp, const NetworkClientSocket
 			 *  first place. This filters that out. */
 			cp.callback = (cs != owner) ? NULL : callback;
 			cp.my_cmd = (cs == owner);
-			cs->outgoing_queue.Append(&cp);
+			cs->outgoing_queue.Append(cp);
 		}
 	}
 
 	cp.callback = (cs != owner) ? NULL : callback;
 	cp.my_cmd = (cs == owner);
-	_local_execution_queue.Append(&cp);
+	_local_execution_queue.Append(cp);
 }
 
 /**
@@ -283,11 +289,10 @@ static void DistributeQueue(CommandQueue *queue, const NetworkClientSocket *owne
 	int to_go = _settings_client.network.commands_per_frame;
 #endif
 
-	CommandPacket *cp;
+	std::unique_ptr<CommandPacket> cp;
 	while (--to_go >= 0 && (cp = queue->Pop(true)) != NULL) {
 		DistributeCommandPacket(*cp, owner);
-		NetworkAdminCmdLogging(owner, cp);
-		free(cp);
+		NetworkAdminCmdLogging(owner, cp.get());
 	}
 }
 
@@ -323,9 +328,10 @@ const char *NetworkGameSocketHandler::ReceiveCommand(Packet *p, CommandPacket *c
 	cp->tile    = p->Recv_uint32();
 	cp->binary_length = p->Recv_uint32();
 	if (cp->binary_length == 0) {
-		p->Recv_string(cp->text, lengthof(cp->text), (!_network_server && GetCommandFlags(cp->cmd) & CMD_STR_CTRL) != 0 ? SVS_ALLOW_CONTROL_CODE | SVS_REPLACE_WITH_QUESTION_MARK : SVS_REPLACE_WITH_QUESTION_MARK);
+		p->Recv_string(cp->text, (!_network_server && GetCommandFlags(cp->cmd) & CMD_STR_CTRL) != 0 ? SVS_ALLOW_CONTROL_CODE | SVS_REPLACE_WITH_QUESTION_MARK : SVS_REPLACE_WITH_QUESTION_MARK);
 	} else {
 		if ((p->pos + (PacketSize) cp->binary_length + /* callback index */ 1) > p->size) return "invalid binary data length";
+		if (cp->binary_length > MAX_CMD_TEXT_LENGTH) return "over-size binary data length";
 		p->Recv_binary(cp->text, cp->binary_length);
 	}
 
@@ -350,9 +356,10 @@ void NetworkGameSocketHandler::SendCommand(Packet *p, const CommandPacket *cp)
 	p->Send_uint32(cp->tile);
 	p->Send_uint32(cp->binary_length);
 	if (cp->binary_length == 0) {
-		p->Send_string(cp->text);
+		p->Send_string(cp->text.c_str());
 	} else {
-		p->Send_binary(cp->text, cp->binary_length);
+		assert(cp->text.size() >= cp->binary_length);
+		p->Send_binary(cp->text.c_str(), cp->binary_length);
 	}
 
 	byte callback = 0;

src/network/network_server.cpp

@@ -1144,7 +1144,7 @@ NetworkRecvStatus ServerNetworkGameSocketHandler::Receive_CLIENT_COMMAND(Packet
 
 	if (GetCommandFlags(cp.cmd) & CMD_CLIENT_ID) cp.p2 = this->client_id;
 
-	this->incoming_queue.Append(&cp);
+	this->incoming_queue.Append(std::move(cp));
 	return NETWORK_RECV_STATUS_OKAY;
 }
 
@@ -1799,10 +1799,9 @@ void NetworkServerSetCompanyPassword(CompanyID company_id, const char *password,
  */
 static void NetworkHandleCommandQueue(NetworkClientSocket *cs)
 {
-	CommandPacket *cp;
+	std::unique_ptr<CommandPacket> cp;
 	while ((cp = cs->outgoing_queue.Pop()) != NULL) {
-		cs->SendCommand(cp);
-		free(cp);
+		cs->SendCommand(cp.get());
 	}
 }