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:

• [x] 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)~.
• [x] 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 https://github.com/mickael9/ioq3/issues/30#issuecomment-379081955

[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 :+1:

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.

[KroniK907]

This definitely should work for the majority of servers, but it would be nice to find a way for it to fail gracefully and provide useful feedback to a server administrator without needing to grep logs to find out their sever name is too long when certain maps are loaded.

[ThomasBrierley]

This definitely should work for the majority of servers

Yeah maybe, it only actually removes 27 chars, I was expecting more but I guess that's how close it is flying to the limit so it helps.

it would be nice to find a way for it to fail gracefully and provide useful feedback to a server administrator without needing to grep logs

This might be possible if checking against the max length map name available upfront (I think mapname is the only variable length one while running).

[ThomasBrierley]

I found even with the shortened version cvar the infostring can become longer, specifically when tweaking things with rcon. I ended up striping the PLATFORM_NAME/DATE as well just to stop it from breaking.

I had a look at the content of infostring I'm not sure the previous suggestions are going to be very applicable... first of all there aren't many non-functional cvars, (motd and welcome message are not included). So it's not immediately obvious what is not useful.

However I noticed that things like bomb mode cvars are sent regardless of game type, so one possible solution would be to find out which cvars are contextual and exclude them from irelevant combinations (and then hope that the worst case combination actually reduces in length).

[ThomasBrierley]

TL;DR default server.cfg verbosity is mostly to blame

I've compared my test server infostring to other public servers to find some significant differences beyond just cvar values. There are a few subtle differences caused by the engine, specifically com_gamename and com_protocol are not prefixed in the official engine. However the most significant differences are with actual server.cfg of individual servers:

Many servers just ommit default cvars from their config resulting in a vastly shorter infostring. These can be easily compared using the website: http://www.urbanterror.info/servers/list/ The "settings:" section for each page enumerates the serverinfo string.

This one has particularly minimal serverinfo: http://www.urbanterror.info/servers/209.190.50.170:27960/ (45 cvars at time of posting)

Mine is comparably very long: http://www.urbanterror.info/servers/139.162.251.183:27960/ (58 cvars at time of posting)

Mine is new and just uses the default server.cfg with a few tweaked values.

It's pretty obvious from this that ommitting cvars with default values from infostring is going to be a big win and far simpler than previous ideas.

Although this does not gaurantee no overflow, it should work in almost all cases since a server.cfg would need close to every cvar value to be non-default for the issue to occur.

[KroniK907]

@ThomasBrierley, As far as I know, even if the cvar is not in the server.cfg it sends the default value as a part of the serverinfo string. But I haven't looked closely enough at the code to verify this statement.

[karnute]

Following the spirit of suggestions here, about reordering of cvars in infostrings, I have created a patch https://github.com/karnute/ioq3/tree/cvar_infoseclong with a new flag to mark secondary cvars that are inserted into infostrings in a second batch https://github.com/karnute/ioq3/commit/8452121fffd73af388ccbb95fbfd97addf7e85e0 . So important cvars are inserted first and then the probability of them being left out by exceeding the length is reduced. In this first patch, all user created cvars (from console or scripts) and a few of the c-code created ones in serverinfo (version, fraglimit, timelimit, sv_keywords, sv_dlURL) and systeminfo (sv_paks, sv_pakNames, sv_referencedPaks, sv_referencedPakNames) are marked directly as secondary. Also it includes a commit https://github.com/karnute/ioq3/commit/278ad3e48e3580a1a424eb0259113d53cc5a5aea to add more information when a cvar is left out of infostring due to overflow, so server admins can know which information is not being sent to clients, etc. If you think, that it could work, I will sent a pull request here. And if in the tests it does not break other things, the same changes can be proposed to official engine also. Edit: If any cvar created from scripts is important (affects clients if not sent in infostring), it should be created in c-code with a default value and without the new flag, to ensure it is included first.

[KroniK907]

So far as I can see this looks like an excellent solution. I think I'd like to test it maybe with a super long map name and some long server names etc to see how well it works. This should only affect the server build right?

[karnute]

Yes, I tested with extra long hostnames and it left out only some cvars (not g_gametype), and also the warning messages say which ones are left out. The change only affects server build in the sense that any client engine will receive and process the infostring in the same way. It could have some other unexpected effects depending on which cvars are left out if overflow...

[ThomasBrierley]

As far as I know, even if the cvar is not in the server.cfg it sends the default value as a part of the serverinfo string. But I haven't looked closely enough at the code to verify this statement.

I've been basing this on the "InitGame:" message in the server console (uses CVAR_SYSTEMINFO which looks to be the same variable used to send to the client). And from what I can see: Yes and no, some cvars are always sent and others only when defined by the config.

With an empty server.cfg there are 52 cvars (the difference from my earlier observation is due to the website listing some cvars in different places). This is the same for both the official engine and this one, except for two cvars which are different they are the same list.

When comparing this to the default server.cfg these are the extra ones that will only be included in the infostring iff user defined (These are only what I discovered in the default cfg, there may be other optional cvars also.):

\auth_verbosity
\auth_notoriety
\auth_tags
\auth_cheaters
\auth_enable
\g_teamNameBlue
\g_teamNameRed
\sv_dlURL
\auth_log

Mostly all of the auth cvars, admittedly not as many as I thought, but enough to make a substantial difference 128 chars + a variable length depending on team names and dlURL (doesn't sound like much but that's > 12% of the available chars).

It does make me wonder though if it's possible to omit some of the others and what the existing clients behavior is when they are omitted.

[edit]

Nice work @karnute, In light of that I think i'll abandon my above idea :smiley:

[karnute]

I have created an alternative patch https://github.com/karnute/ioq3/tree/cvar_infoprimary with the reverse meaning of the new flag (CVAR_INFOPRIMARY) to mark the relevant or important cvars that should go first when building the infostrings (instead of marking the secondary ones as in the other patch). I thought it should be equivalent but more clear and maintainable in the future, because if any important cvar is kept out by future overflows, then it could be marked as primary in the code. The problem is that with this alternative the order has changed (respect to marking secondary cvars), so some cvars like g_matchmode are left out when there is an overflow in infostring. We should revise the supposition that all script and console cvars were created in the function Cvar_Set_f() in cvar.c. We need to understand better were is created each cvar. Note: I will rebase both branchs to include the last commits merged by Daniele just at the same time I was uploading.

[danielepantaleone]

I don't know why this issue got closed in the first place after the merge of the request (I specifically merged without closing related issues, so I guess Github messed up something). Anyway sorry about that.

I'm not sure about the changes to CVAR flags....the problem here is not the engine but it's Urban Terror that needs to stay compatible with vanilla Q3A, hence the serverinfo string limit cannot be increased. I agree that here we are trying to address (or better, trying to bypass) an issue related to Urban Terror limitation, but I don't know if this is the right path (I have no other ideas though).

[karnute]

The new cvar flag does not affect compatibility with vanilla Q3A, because the same infostring overflows would occur also with vanilla Q3A and important cvars left out of it would cause the same errors. Both patches (not pull requested yet) would change only the order in which cvars are added to the infostring with the objective of inserting first the important ones (like g_gametype) that can affect the game if they are left out because an overflow in infostring (caused by other long and secondary cvars inserted before it). The order of cvars inside the infostring is not relevant in the client as it is parsed anyway. The difference in both patches is the meaning of the flag and to select the better one in which cvars are marked to get the best order in the infostrings.

[danielepantaleone]

Obviously it won’t affect compatibility with vanilla Q3A since it’s not vanilla Q3A but a custom fork. If you read again my reply I’m pointing out only that we are trying to circumvent an issue using these additional flags, while there could be a much easier way if only Urban Terror would not have to stay compatible with vanilla Q3A.

[ThomasBrierley]

I don't know why this issue got closed in the first place after the merge of the request (I specifically merged without closing related issues, so I guess Github messed up something). Anyway sorry about that.

My fault sorry, GitHub autocloses issues when merging a PR with a "fixed" keyword preceeding a reference to the issue in body or a commit message.

[KroniK907]

@danielepantaleone The point that @karnute was trying to make was that a server built using his custom flags wont affect a vanilla Q3A client in any way. A vanilla Q3A client will not have any issue since the info strings are put together server side.

Merging Karnute's code only affects the server build and not the client build, so there should be no conflicts with vanilla Q3A client.

EDIT: So much miscommunication in this thread. After re-reading @danielepantaleone comment again, I realize that he wasn't objecting just pointing out that he wishes that we could just up the limit.

[KroniK907]

@karnute are you going to make a PR for your solution?

[karnute]

Yes, but first I must check why the second alternative https://github.com/karnute/ioq3/tree/cvar_infoprimary (which I prefer) results in different ordering of cvars from the first one https://github.com/karnute/ioq3/tree/cvar_infoseclong , and currently I am very busy. Probably in a few days...

[BidyBiddle]

Any news @karnute ?