The International Simutrans Forum

Development => Patches & Projects => Incorporated Patches and Solved Bug Reports => Topic started by: jamespetts on September 30, 2017, 10:25:00 AM

Title: Server crashes when MOTD has a double quote?
Post by: jamespetts on September 30, 2017, 10:25:00 AM
I am not entirely sure that this affects the latest version of Standard, as I have not tested this fully (this is a very time consuming issue to test), but the latest version of Extended on the master branch appears to have a bug that causes the server to crash on saving a game when the MOTD contains a double quote (") character.

Because the code for saving the MOTD is unchanged between Standard and Extended and, I believe, unchanged recently, it seems likely that this is also a bug in Standard, although I have not had time to test this.

A workaround is to use single quotes (').
Title: Re: Server crashes when MOTD has a double quote?
Post by: Dwachs on October 01, 2017, 09:25:16 AM
The crash happens during saving at the server?

Edit: From looking at the code, I think there is a problem: motd[len]=0 writes out of bounds, as motd has length len.......
Title: Re: Server crashes when MOTD has a double quote?
Post by: jamespetts on October 01, 2017, 09:31:02 AM
Yes, I think so - it crashes, then the saved game is corrupted (and always the same file size for any given saved game to start with).
Title: Re: Server crashes when MOTD has a double quote?
Post by: Dwachs on October 01, 2017, 09:31:53 AM
From looking at the code, I think there is a problem: motd[len]=0 writes out of bounds, as motd has length len.......

I cannot test right now.
Title: Re: Server crashes when MOTD has a double quote?
Post by: Ters on October 01, 2017, 11:49:10 AM
Where in the code is this motd[len]=0? It certainly looks suspicious, but that depends on what len is supposed to be, string length or buffer size.
Title: Re: Server crashes when MOTD has a double quote?
Post by: Dwachs on October 01, 2017, 04:14:10 PM
Should be fixed in r8298.

James, thanks for reporting.
Title: Re: Server crashes when MOTD has a double quote?
Post by: DrSuperGood on October 05, 2017, 11:16:24 AM
char *motd = (char *)malloc( len );Code like this makes Simutrans look like it has an identity crisis. I thought Simutrans was meant to be C++ but that is clearly C code...

I would imagine the C++ form should look like...char *const motd = new char[len];
...
delete[] motd;
An argument for this is that it removes a typecast and explicitly shows that an array is being allocated so is more readable (malloc allocates general memory which may or may not end up being used as an array). Ideally std:string would be used as that automatically manages memory but that requires at least C++11 to be compatible in this use case (earlier C++ versions do not permit the direct modification of c_str).
Title: Re: Server crashes when MOTD has a double quote?
Post by: Ters on October 05, 2017, 05:33:42 PM
Quote from: DrSuperGood on October 05, 2017, 11:16:24 AM
earlier C++ versions do not permit the direct modification of c_str

Arguably, no C++ version should permit direct modification of c_str. If safety is the goal, any kind of modification should be disallowed.
Title: Re: Server crashes when MOTD has a double quote?
Post by: prissi on October 06, 2017, 02:39:42 AM
I would prefer "char *const motd = new char[len];" It was avoided in the past, because new with zero used to crash MSVC compiled programs, even though it should not (according to C++ standard.)
Title: Re: Server crashes when MOTD has a double quote?
Post by: DrSuperGood on October 06, 2017, 03:40:41 AM
QuoteArguably, no C++ version should permit direct modification of c_str. If safety is the goal, any kind of modification should be disallowed.
I see where that could be a problem now. When looking up UTF-8 with UTF-16 I read this (https://msdn.microsoft.com/en-us/magazine/mt763237.aspx) article which after giving a second read does some pretty unsafe stuff. It takes the address of the first operator[] element in a std::string and uses that as a char array, however I do not think std::string guarantees that each sequential element returned by operator[] is address wise sequential and hence an array despite the operator being array like.
QuoteI would prefer "char *const motd = new char[len];" It was avoided in the past, because new with zero used to crash MSVC compiled programs, even though it should not (according to C++ standard.)
I cannot find an internet reference to that so possibly it was a short lived bug? Not like it could happen in this case as it looks like a minimum value of 1 is enforced.