OpenTTD

Tasklist

FS#5326 - NETWORK_REVISION_LENGTH causing "String too long for destination buffer" git branch with length > 8

Attached to Project: OpenTTD
Opened by Nico Schmoigl (Eagle_rainbow) - Tuesday, 09 October 2012, 20:14 GMT
Last edited by andythenorth (andythenorth) - Thursday, 31 August 2017, 20:55 GMT
Type Feature Request
Category Network
Status With patch
Assigned To No-one
Operating System All
Severity Very Low
Priority Normal
Reported Version trunk
Due in Version Undecided
Due Date Undecided
Percent Complete 0%
Votes 0
Private No

Details

Hi folks,

doing my first steps on playing around with the source code of my beloved OpenTTD, I stumbled over the following issue:

How to reproduce:
1. Clone the git repository
2. create a new branch with a long name (at least 6 chars) such as "longbranchname"
3. checkout that branch
4. do a configure such that rev.cpp is written
5. do a tiny change
6. add and submit your change
7. do a make distclean
8. do a configure again
9. compile it

Now, in variable _openttd_revision available via rev.h there will be an eight char long hash value of your git commit plus the hyphen plus the long name of your branch (see also ./findversion.sh). Note in function GamelogRevision() of file gamelog.cpp there is a strecpy into a string with fixed length set maximal to length 15. This will result in multiple following error message on startup of the program (in debug mode):

dbg: [misc] String too long for destination buffer

For me, it also caused that the linked program did not start anymore.

Obviously, a workaround is to use a branch name that has less than 8 chars.
The problem with it, is the fact that this field is typed as

char text[NETWORK_REVISION_LENGTH];

whereas NETWORK_REVISION_LENGTH resolves to the length of 15 before. I just replaced the length of this array with - example - 25 and retried it. It did the trick.

What do you think? Is it possible to increase the length there? 8 chars sometimes is really short...

Cheers,
Nico
This task depends upon

Comment by Ingo von Borstel (planetmaker) - Wednesday, 10 October 2012, 11:03 GMT
Personally I haven't touched network code yet, for a reason, so take my words with a grain of salt:
Just changing that means touching network compatibility; likely you'll have to introduce a new network protocol version:
You'll have to look at the master server that it treats it correctly, depending on the client version it communicates with. You'll also have to check that such change doesn't increase the size of any network packet beyond its allowed limits.
Comment by Nico Schmoigl (Eagle_rainbow) - Wednesday, 10 October 2012, 18:33 GMT
Granted to all what you said. However, I see also another option to not affect the network protocol: Apparently, the problem only occurs with the gamelog. The gamelog just "reuses" the constant for the length of the revision attribute (see gamelog_internal.h, line 45 as of r24576). Why not just extending the field length manually there. The constant for the length is called "NETWORK_REVISION_LENGTH". Thus, why not having a "GAMELOG_REVISION_LENGTH"... This could be a quick fix to it, but on the other hand introduces a little inconsistency.
BTW: That's also the reason why I don't understand that you have put the problem to category "Network"... but, as I am a newbie - you'll know what you are doing :)
Comment by Alberth (Alberth) - Wednesday, 10 October 2012, 19:40 GMT
Afaik, the revision (which includes the branch name), gets transmitted over the network for game version identification. There is also other data in that packet, and the text got the space that was left, which is unfortunately not much.
Making it longer is not possible, since the network packet may not arrive, or not arrive in one piece, in that case.

The issue is however not serious, the message is just a notice that not all text is copied. Since this will happen at every machine the game identification still works. Also, since it is a debug notice for versions that are built from a branch, only developers ever see the message.

Fixing the issue looks like overkill to me tbh, extending the network protocol just for experimental builds to prevent an information message is just tmwftlb. There are bigger problems in OpenTTD.
Comment by frosch (frosch) - Thursday, 11 October 2012, 16:10 GMT
Extending the name in the gamelog sounds reasonable. Telling the exact version from a savegame can be very useful.
Comment by frosch (frosch) - Sunday, 14 October 2012, 14:37 GMT
Attached diff removes the limit for the revision text in the gamelog.
Comment by Nico Schmoigl (Eagle_rainbow) - Sunday, 14 October 2012, 19:15 GMT
Just checked the diff by frosch. It would fix my problem.
Comment by Nathanael Rebsch (dihedral) - Tuesday, 16 October 2012, 10:15 GMT
could you also please try this with network server + connecting a client using the multiplayer lobby?
Comment by frosch (frosch) - Tuesday, 16 October 2012, 16:31 GMT
Who does your question address?

The patch does not change the version which is sent across the network. The packet has to truncate the string. However, the receiving side also does only compare the truncated versions.

So:
* OpenTTD titlebar and intro GUI show the revision with a higher limit on the length.
* With the patch the gamelog also stores the revision with a much higher limit.
* Revisions sent over the network remain truncated.
** The client and openttd.org only display the truncated version.
** The compatibility check only compares the truncated versions.
Comment by Nico Schmoigl (Eagle_rainbow) - Friday, 19 October 2012, 17:41 GMT
Though I agree to frosch's statement, I still did a brief test with the patch in a multiplayer environment:
* git version on trunk with patch from above applied; one server via LAN + one additional local client --> works
* official release 1.2.2 source code, patch applied (has problems due to savegame version, but the rest was applied successfully), connect to public server on the list with compatible version --> works

Thus, I'd say, that this patch does not introduce an obvious side effect to the multiplayer code.

Loading...