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

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

Closed
DorpsGek opened this issue Oct 9, 2012 · 12 comments

Comments

@DorpsGek
Copy link

@DorpsGek DorpsGek commented Oct 9, 2012

Eagle_rainbow opened the ticket and wrote:

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

Reported version: trunk
Operating system: All


This issue was imported from FlySpray: https://bugs.openttd.org/task/5326
@DorpsGek

This comment has been minimized.

Copy link
Author

@DorpsGek DorpsGek commented Oct 10, 2012

planetmaker wrote:

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.


This comment was imported from FlySpray: https://bugs.openttd.org/task/5326#comment11578
@DorpsGek

This comment has been minimized.

Copy link
Author

@DorpsGek DorpsGek commented Oct 10, 2012

Eagle_rainbow wrote:

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


This comment was imported from FlySpray: https://bugs.openttd.org/task/5326#comment11581
@DorpsGek

This comment has been minimized.

Copy link
Author

@DorpsGek DorpsGek commented Oct 10, 2012

Alberth wrote:

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.


This comment was imported from FlySpray: https://bugs.openttd.org/task/5326#comment11582
@DorpsGek

This comment has been minimized.

Copy link
Author

@DorpsGek DorpsGek commented Oct 11, 2012

frosch wrote:

Extending the name in the gamelog sounds reasonable. Telling the exact version from a savegame can be very useful.


This comment was imported from FlySpray: https://bugs.openttd.org/task/5326#comment11585
@DorpsGek

This comment has been minimized.

Copy link
Author

@DorpsGek DorpsGek commented Oct 14, 2012

frosch wrote:

Attached diff removes the limit for the revision text in the gamelog.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/5326#comment11588
@DorpsGek

This comment has been minimized.

Copy link
Author

@DorpsGek DorpsGek commented Oct 14, 2012

Eagle_rainbow wrote:

Just checked the diff by frosch. It would fix my problem.


This comment was imported from FlySpray: https://bugs.openttd.org/task/5326#comment11590
@DorpsGek

This comment has been minimized.

Copy link
Author

@DorpsGek DorpsGek commented Oct 16, 2012

dihedral wrote:

could you also please try this with network server + connecting a client using the multiplayer lobby?


This comment was imported from FlySpray: https://bugs.openttd.org/task/5326#comment11592
@DorpsGek

This comment has been minimized.

Copy link
Author

@DorpsGek DorpsGek commented Oct 16, 2012

frosch wrote:

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.


This comment was imported from FlySpray: https://bugs.openttd.org/task/5326#comment11594
@DorpsGek

This comment has been minimized.

Copy link
Author

@DorpsGek DorpsGek commented Oct 19, 2012

Eagle_rainbow wrote:

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.


This comment was imported from FlySpray: https://bugs.openttd.org/task/5326#comment11603
@nielsmh

This comment has been minimized.

Copy link
Contributor

@nielsmh nielsmh commented Nov 5, 2018

This is still an issue, and I imagine it can cause undetected compatibility issues with network games.

@nielsmh nielsmh added this to Critical bugs in Release 1.9.0 Nov 5, 2018
@nielsmh

This comment has been minimized.

Copy link
Contributor

@nielsmh nielsmh commented Nov 5, 2018

After discussing this on IRC, I think we've agreed that the network server_revision field being limited to 15 bytes (of which one is consumed as string terminator) is too short for this brave new git world.

I think there is also enough room in the UDP packet to expand the server_revision field a bit, perhaps to 25 or 30 bytes, which should protect against most matched-mismatch issues, without lowering the max number of NewGRFs allowed. Checking the code, it looks like expanding the field is even possible without bumping the game_info_version number, as older versions will just truncate the string on receiving a too long one.

@LordAro

This comment has been minimized.

Copy link
Member

@LordAro LordAro commented Jan 6, 2019

Closed in favour of #7021

@LordAro LordAro closed this Jan 6, 2019
Release 1.9.0 automation moved this from Critical bugs to Done Jan 6, 2019
@LordAro LordAro removed this from Done in Release 1.9.0 Jan 23, 2019
@LordAro LordAro added this to the 1.9.0 milestone Jan 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.