Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Version names truncated after move to git #7021

Closed
nielsmh opened this issue Jan 6, 2019 · 6 comments · Fixed by #7121
Closed

Version names truncated after move to git #7021

nielsmh opened this issue Jan 6, 2019 · 6 comments · Fixed by #7121
Labels
bug Something isn't working regression It used to work, and now it's broken.
Milestone

Comments

@nielsmh
Copy link
Contributor

nielsmh commented Jan 6, 2019

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.

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

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.

/** 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
Copy link
Contributor

Eddi-z commented Jan 6, 2019

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
Copy link
Contributor Author

nielsmh commented Jan 6, 2019

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.

@andythenorth andythenorth added bug Something isn't working regression It used to work, and now it's broken. labels Jan 7, 2019
@LordAro LordAro added this to the 1.9.0 milestone Jan 23, 2019
@nielsmh
Copy link
Contributor Author

nielsmh commented Jan 27, 2019

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
Copy link
Contributor Author

nielsmh commented Jan 27, 2019

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
Copy link
Contributor

glx22 commented Jan 27, 2019

Game log is stored in the savegame IIRC

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

may be a problem

@nielsmh
Copy link
Contributor Author

nielsmh commented Jan 27, 2019

/** 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working regression It used to work, and now it's broken.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants