Serverinfo string overflow

[ThomasBrierley]

This is a strange one, had me scratching my head for a while.

If sv_hostname is longer than 25 chars (including hidden colour code chars) the game type is forced to FFA. Note that the server will still advertise whatever g_gametype was set to and rcon g_gametype will still return whatever it was set to while running, yet the actual game mode used will be FFA.

This behaviour doesn't seem to occur with Barbatos's repo.

set  sv_hostname "12345678901234567890123456" // 26 char name

[edit]

Thanks for everyones invaluable input on this. Rough plan from discussion bellow:

• Shorten version cvar (specifically git rev) to reduce chance of hitting limit (Pointed out by Karnute).
• Ommit cvars with default values (Default server.cfg causes all defaults to be sent unecessarily).
• Reorder cvars to ensure functionally important ones appear at the start (Suggested by Kronik).
• Determine superficial cvars and truncate their values when limit exceeded in order of importance.
• Add 2nd serverinfo string containing untruncated cvars (without interfering with strict q3 protocol).

Exclude truncation based option since reviewing content of infostring (almost all cvars are not strings).
Exluded omitting cvars option because the optional ones are mostly auth cvars.
Kronik has detailed relevant functions here #30 (comment)

[ThomasBrierley]

Actually it doesn't seem to be a constant limit, the maximum length appears to depend on the value of sv_hostname but I can't infer what the relationship is... non-alphanumeric chars seem to severely shorten the max length e.g spaces. Trying to figure out an name that doesn't break is quite maddening.

[danielepantaleone]

To me this looks as a serverinfo string overflow issue. The gametype on the server is set correctly but when the serverinfo string is transmitted to the client it gets messed up (overflowing the max length eventually) so on clients the gametype is parsed erroneously and the UI reacts accordingly (just print the serverinfo string on the client to debug it, +set developer 1 would do it if I recall correctly).

[ThomasBrierley]

Thanks @danielepantaleone I'll give that a go another day (I wasted all my time today trying to find a server name that didn't break it :D)

[edit]

It also depends on which map is loaded!

[karnute]

To me this looks as a serverinfo string overflow issue.

Yes, be aware that the "version" cvar in the infostring on Mickael9 builds can be longer than in the official builds, as they include git rev etc (search for all "VERSION" appearances in Makefile).
Check the length of the line starting with "InitGame:" in the server log.

[The-spiki]

Hi Thomas, thank you for opening this issue and troubleshooting, because now we are closer to it being fixed eventually. I had my fair share of problems with wrong gametype being set, spent some time testing etc...
But didn't wanted to come out stupid reporting it before being 100% sure that long server name was really the reason for wrong gametype.

Being lazy sys admin, I've always reused my configs (not just for games), but eventually it came to my mind that if others are not experiencing and reporting same issues the error is probably on my side.
So I did at some point start from scratch, but plainly copied over settings from old config.

I never did proper advanced troubleshooting, but just by paying attention to logs, the only error that was consistently popping up was server info too long, so I shortened the name of server.
For example, our server in 4.1/4.2 used to be [b00bs] USA Uptown CTF (b00bs-clan.com) [UAA] (46 char)
In 4.3 the problems on that particular server stopped after I renamed it "[b00bs] USA Uptown CTF" (23 char)

Another server that had similar issue occasionally was the map rotating server, usually named
[b00bs] USA CTF all maps appended with either [new-ioq3] or [m9]
(Biddle suggested to add ioq3/m9 in the name so that we would advertise testing)
After some testing, I've found out that this worked
[b00bs] USA CTF all maps (25 char)
but, it again effed up occasionally, though rarely, so I finally shortened it to
[b00bs] USA CTF all map (24 char)
which I believe never ended up with wrong gametype

[danielepantaleone]

The map name is part of the serverinfo string, so if it's long it can overflow. I agree that adding extra info to the infostring (such as git revision, build number, etc) is not helping

[ThomasBrierley]

Yes, be aware that the "version" cvar in the infostring on Mickael9 builds can be longer than in the official builds, as they include git rev

Thanks Karnute, this seems like a good candidate, git revs can be pretty long at up to 40 chars depending on the truncation.

Another server that had similar issue occasionally was the map rotating server... but, it again effed up occasionally, though rarely, so I finally shortened it to...

Hey spiki! I was doing the same thing, it was driving me nuts, just kept making it shorter and shorter until finding that even mapnames were affecting it as you have discovered as well. But as Daniel pointed out above the mapname is of course also part of the severinfo string so it all matches Daniel's suggestion so far.

I did notice some kind of max string length warning being occasionally printed out to the shell the server was running in but this even seemed to be the case with default config so I thought maybe it was a more generic and benign issue.

I suppose not every truncation of the serverinfo string will necessarily cause incorrect parsing which explains why these warnings do not always coincide with this particular manifestation of the issue, just whichever combination finally pushes the value of g_gametype off the cliff :P, I wonder what else reaches that fate first. I'll try have a dig into this tonight.

[Barbatos]

Yeah unfortunately we can't really fix this issue as it would break the compatibility with Quake 3 and its 1024 chars limit for the serverinfo string. We've been trying to limit the number and length of cvars that we add to the serverinfo string since 4.2.001 but it clearly isn't enough to get rid of the overflow issues.

[ThomasBrierley]

Ok I did wonder this as the obvious solution is to change the limit, but that obviously requires a difference in the client. Well for this repo at least the git rev can be truncated to something less demanding, git describe can be used to produce the minimum unambigous truncation.

More generally it might be good have a functional failure modes. i.e ensure a correctly parsable serverinfo string with at least all of the functionally important cvars by truncating any superficial variable length values when necessary.

Some obvious candidates in order of least importance would be:
sv_joinmessage
g_motd
sv_hostname

I suppose mapname is the most annoying because it could potentially affect the truncation of other superficial values on each map cycle making it unpreditable for server admins.

Without really knowing how this thing works yet, here's another thought: is it possible to have a backwards compatible overflow? i.e does the truncation of the serverinfo string happen at the client end? in which case my former solution can be used to force the length to a 1024 parsable chunk, then use a variable length overflow to store the truncated parts for custom clients that are able to use it to rebuild a the larger original serverinfo string (i.e the UrT engine could have longer serverinfo values while quake3 compatible ones would get some truncated values but operate correctly otherwise).

</mass speculation>

[KroniK907]

Without really knowing how this thing works yet, here's another thought: is it possible to have a backwards compatible overflow?

I was thinking the same thing. At minimum would it be possible to re-order the items in the string so that the most important information is first in the string and will get parsed, and if there is an overflow, the last information given (g_motd etc) would get truncated from the string?

[ThomasBrierley]

That would certainly be a good start to mitigate the issue. I think all of these solutions build up quite nicely on top of each other as a route to a potential complete fix:

1. Adjust git rev length to reduce chance of hitting limit (Pointed out by Karnute).
2. If serverinfo is key based: determine functionally important cvars and use to reorder to ensure they appear at the start of serverinfo string (Suggested by Kronik).
3. Determine superficial cvars with variable length values in order of importance, and use to truncate when serverinfo exceeds limit. This ensures it never exceeds 1024 and will always parse correctly.
4. If it's possible to send a larger serverinfo string and simply let it safely overflow at client end: Add a second serverinfo section after the first 1024 chars for UrT which contains the original values of any truncated cvars in the first serverinfo string.

The first 2 get most of the way with minimal work, 3 and 4 might be a little over the top but we can try if they turn out to be necessary.

[KroniK907]

Ok, after spending a couple hours looking into this, this appears to be where the server sets the serverinfo string:
https://github.com/id-Software/Quake-III-Arena/blob/dbe4ddb10315479fc00086f08e25d968b4b43c49/code/server/sv_init.c#L535

The client appears to parse that string here:
https://github.com/id-Software/Quake-III-Arena/blob/dbe4ddb10315479fc00086f08e25d968b4b43c49/code/cgame/cg_servercmds.c#L144

Digging deeper, it looks like this is the function that the server uses to decide what goes into the serverinfo string:
https://github.com/id-Software/Quake-III-Arena/blob/dbe4ddb10315479fc00086f08e25d968b4b43c49/code/qcommon/cvar.c#L777

And this is the function that actually builds that string:
https://github.com/id-Software/Quake-III-Arena/blob/dbe4ddb10315479fc00086f08e25d968b4b43c49/code/game/q_shared.c#L1165

Let me take a sec to just talk through the code that builds the serverinfo string. It looks like it initializes a character array to the max allowable length, and then it loops through all the cvars. When it reaches a cvar that it should add to the string, it runs the string builder function sending it the character array, the cvar name and the cvar value string. Then the string builder adds the the cvar name and value to the serverinfo character array.

Now if the serverinfo string would be too long if the string builder function added the new cvar value, then it simply returns without adding it. That's it. There is no built in method for knowing what was missed or what cvars are more important, etc. Not to mention, the function that is picking the cvars to add to the serverinfo string, has no way of knowing if the cvar it wants to add is too big or if the serverinfo string is close enough to being full that another cvar cant be added to it.

I'm not sure what the best way to solve the problem would be, but clearly some better error reporting and logic built into it to make sure that the most important cvars are in the serverinfo string and maybe just truncate the least important like the motd.

Unfortunately it doesnt look like it would be easy to add an overflow string like @ThomasBrierley was thinking, but I may be wrong on that.

[ThomasBrierley]

Thanks for digging those up Kronik 👍

Unfortunately it doesnt look like it would be easy to add an overflow string like @ThomasBrierley was thinking, but I may be wrong on that.

It looks that way, but the concept of essentially "sending the extra bits for custom clients" could be achieved separately also. But that was just an extra nice to have, most important is to make it functionally reliable, and from what you've found that looks possible.

[ThomasBrierley]

Shortening the version alone was a quick win, it's fixed it for my current server.cfg but it will no doubt need more work for more hungry server and map name combos.