Version names truncated after move to git

[nielsmh]

Builds of most non-master branches will currently print dbg: [misc] String too long for destination buffer messages to the console. This is a symptom of the version strings generated being longer than the 15 byte buffer allocated.

This becomes relevant especially for network games. The server's version name is truncated to 15 bytes, which can lead to unrelated versions being indistinguishable, with a large risk of desyncs.

OpenTTD/src/network/core/game.h

Line 44 in 5ff6053

char server_revision[NETWORK_REVISION_LENGTH]; ///< The version number the server is using (e.g.: 'r304' or 0.5.0)

OpenTTD/src/network/core/config.h

Line 40 in 5ff6053

NETWORK_REVISION_LENGTH = 15, ///< The maximum length of the revision, in bytes including '\0'

As far as I can tell, it should be possible to increase NETWORK_REVISION_LENGTH without breaking compatibility with older clients. The Recv_string function does handle data longer than the supplied buffer, and will truncate strings longer than the buffer.

OpenTTD/src/network/core/packet.cpp

Lines 242 to 265 in 5ff6053

	/** Reads a string till it finds a '\0' in the stream */
	void Packet::Recv_string(char *buffer, size_t size, bool allow_newlines)
	{
	PacketSize pos;
	char *bufp = buffer;
	const char *last = buffer + size - 1;
	/* Don't allow reading from a closed socket */
	if (cs->HasClientQuit()) return;
	pos = this->pos;
	while (--size > 0 && pos < this->size && (*buffer++ = this->buffer[pos++]) != '\0') {}
	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++;
	pos++;
	}
	this->pos = pos;
	str_validate(bufp, last, allow_newlines);
	}

The main thing to watch out for with increasing NETWORK_REVISION_LENGTH is the MTU for UDP server discovery packets. Increasing it by 20 will cost one possible NewGRF for network games, but should cover most if not all cases.

[Eddi-z]

i'd rather make sure the string is cut in a way that the end is always included, as that tends to include the revision hash and makes the versions distinguishable. or make a hash of the complete string and transmit that.

[nielsmh]

Possibly transmit just the revision hash (that should normally be enough), perhaps encoded in a way so more of the hash fits in 15 bytes, perhaps with some indicator bits for modified/non-modified checkout and release build or not.

[nielsmh]

I tried summing up the packet structure as it currently is:

field	size	repeats	total
packet size	2	1	2
packet type	1	1	1
network game info version	1	1	1
newgrf count	1	1	1
newgrf data	20	62	1240
game date	4	1	4
start date	4	1	4
companies max	1	1	1
companies on	1	1	1
spectators max	1	1	1
server name	80	1	80
server revision	15	1	15
server lang	1	1	1
use password	1	1	1
clients max	1	1	1
clients on	1	1	1
spectators on	1	1	1
map name	80	1	80
map width	2	1	2
map height	2	1	2
map set	1	1	1
dedicated	1	1	1
Total			1442

The MTU we work with is 1460 bytes, meaning there should be room for 18 bytes expansion. I suggest expanding server revision by 17 bytes to 32 bytes.

[nielsmh]

A separate issue is the gamelog. It also uses the NETWORK_REVISION_LENGTH constant for the size of revision string it writes to the log, I'm not sure how safe it would be to increase the size here, is this ever written to a file as a binary format? Ideally the field should probably be sufficiently long to contain a full untruncated "display revision string" i.e. the one used in the title screen window.

[glx22]

Game log is stored in the savegame IIRC

OpenTTD/src/saveload/gamelog_sl.cpp

Line 32 in b60b193

SLE_ARR(LoggedChange, revision.text, SLE_UINT8, NETWORK_REVISION_LENGTH),

may be a problem

[nielsmh]

OpenTTD/src/gamelog_internal.h

Lines 36 to 49 in 82fc28f

	/** Contains information about one logged change */
	struct LoggedChange {
	GamelogChangeType ct; ///< Type of change logged in this struct
	union {
	struct {
	byte mode; ///< new game mode - Editor x Game
	byte landscape; ///< landscape (temperate, arctic, ...)
	} mode;
	struct {
	char text[NETWORK_REVISION_LENGTH]; ///< revision string, _openttd_revision
	uint32 newgrf; ///< _openttd_newgrf_version
	uint16 slver; ///< _sl_version
	byte modified; ///< _openttd_revision_modified
	} revision;

This union definitely makes it difficult to change. Should probably just write a prefix of the revision hash to this.