News:

Simutrans Sites
Know our official sites. Find tools and resources for Simutrans.

Server crashes when MOTD has a double quote?

Started by jamespetts, September 30, 2017, 10:25:00 AM

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

jamespetts

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 (').
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

Dwachs

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.......
Parsley, sage, rosemary, and maggikraut.

jamespetts

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).
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

Dwachs

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.
Parsley, sage, rosemary, and maggikraut.

Ters

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.

Dwachs

Should be fixed in r8298.

James, thanks for reporting.
Parsley, sage, rosemary, and maggikraut.

DrSuperGood

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).

Ters

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.

prissi

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.)

DrSuperGood

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