Created attachment 917 [details]
strip special characters from name in server code

Hold off on applying this for now.  It needs to allow spaces and doesn't do the check on every name change.  I'll submit a new patch soon.

Created attachment 918 [details]
special chars fix

this allows space (32) and also cleans up the name if it's changed with the userinfo command.

At the moment, we don't even know whether this is something to do with the Tremulous QVMs or the engine.
Your patches fix symptoms directly in the engine for a problem that could be unrelated to vanilla quake3.

(In reply to comment #4)
> At the moment, we don't even know whether this is something to do with the
> Tremulous QVMs or the engine.
> Your patches fix symptoms directly in the engine for a problem that could be
> unrelated to vanilla quake3.
> 

No, the patch fixes an exploit in the engine.  This is not Tremulous specific.
I know someone reported this with Enemy Territory in the etpub forum, but can't seem to locate the thread.

You should be able to easily reproduce this in any Q3 engine game not running Punkbuster by opening up your autorun.cfg with a text editor and inserting: 

set name "test<character with ordinal reference greater than 127>test"

Once you connect to a server every connected client to spaz out every time your userinfo string is updated and the server sends all clients the updated information with the "cs" command.  

Perhaps further investigation is needed to find out if extended ASCII is really supposed to be allowed in these message strings (NOTE: it's probably pointless since the client does not have glyphs anyway, right?).  I just assumed that since Punkbuster kicks you for having those characters in your name, we probably should not allow them.

(In reply to comment #5)

> No, the patch fixes an exploit in the engine.  This is not Tremulous specific.
> I know someone reported this with Enemy Territory in the etpub forum, but can't
> seem to locate the thread.

I verified that this exploit also works in Enemy Territory 2.60b.  I also verified that Punkbuster does kick you for having extended ASCII characters in your name.  The effects in ET are different though, instead of freezing you in place, it moves the viewangles seemingly randomly and sometimes fires off a few shots :(.  It's not just the userinfo updates either, it also happens when anyone with the bad name says anything in chat.

I will try to reproduce it and have a look at the cause and then decide on how best to fix it.

(In reply to comment #7)
> I will try to reproduce it and have a look at the cause and then decide on how
> best to fix it.
> 

Something else just occured to me.  Assuming that we do end up stripping these characters, this fix will probably have to extend to the entire userinfo string since a client can probably just stick these characters into any userinfo cvar (with setu) and cause the same problem.   I have not tested this yet, but it seems likely.

Created attachment 925 [details]
strip all extended ASCII from userinfo

OK, I've investigated the problem further.

When a client first connects, they provide their userinfo string to the server as param 1 to the "connect" command.  SV_DirectConnect was blindly copying this string into svs.clients[].userinfo:

     Q_strncpyz( userinfo, Cmd_Argv(1), sizeof(userinfo) );
     ...
     Q_strncpyz( newcl->userinfo, userinfo, sizeof(newcl->userinfo) );

Normally messages of this type pass through the MSG_ReadString() and MSG_WriteString() functions which would correctly replace these characters with '.'.  Similarly, if the client issued a "userinfo" command, the server would blindly copy the string:

     Q_strncpyz( cl->userinfo, Cmd_Argv(1), sizeof(cl->userinfo) );

Now technically this should not be an issue since MSG_WriteString() would correctly strip out the special characters in all the messages the server prints out, BUT there is no such stripping of the netname in playerState.

So the problem is pretty specific to special characters in the name afterall.

This patch is essentially the same as the last one but strips the extended ACII from the entire userinfo string in exactly the same way MSG_ReadString() would by replacing all chars with ordinal > 127 with '.'.

I'm sorry, but I'm still not happy about this.
As long as I don't have time for a few continous hours (probably next sunday) I can't check on this.
Stripping special characters from a name maybe really is the solution to this problem. But up to now, we have no idea WHY exactly these characters cause the hiccups.
Is the problem client side? Is it server side? Is it in the engine? in the QVMs?

Okay, I used these special chars in names and had no problems at all with vanilla quake3.

(In reply to comment #11)
> Okay, I used these special chars in names and had no problems at all with
> vanilla quake3.

It seems the quake3 game code strips the userinfo string similarly:
Quake3
------
ClientConnect: 0
ClientUserinfoChanged: 0 n\tj

Ok, I was now able to reproduce the error. It only required a little modification in the client's source.
I will need to further check the implications before I can act upon this. Thanks for the report.

I tracked the actual cause to the bug down.
The problem was server side in the engine part.
The first time a user connects to the server, the infostring is read over MSG_ReadStringLine() which does not as much string sanitizing as the MSG_ReadString function does which will result in ascii chars > 127 will slip through.
Every time a client sends a command like "say" that is broadcasted to other players, the server will add the say command to each reliableCommands array for every client and then use the particular string from the string array on MSG_WriteString() which will replace the special chars with dots in the actual message to the client, but not the reliableCommands array.
So this is the cause to some kind of desync in the reliableCommands arrays between client and server. The server thinks the command has the special chars included, the client sees dots in that particular place.
The problem now is that usermove commands that are used to inform the server about movements, taunts, weapon commands etc are scrambled with a key using the last sent reliable command from the server. I think they did that to make it harder to reverse engineer the q3 protocol. Anyways.. as the server and client have two different versions of the last sent servercommand, the client uses a different key to scramble the usermove command than the server uses for descrambling which ultimately results in wrong interpretation of client commands.
This condition is fixed starting with svn rev 803.
I corrected MSG_ReadStringLine that now corresponds to the MSG_WriteString behaviour. This is a more clean solution than fiddling around with the actual userinfo strings.

Setting a QA contact on all ioquake3 bugs, even resolved ones. Sorry if you get a flood of email from this, it should only happen once. Apologies for the incovenience.

--ryan.