search

ec- / Quake3e

Improved Quake III Arena engine
GNU General Public License v2.0
1.15k stars 148 forks source link

Fix server resource leaks during client reconnect #281

Closed Chomenor closed 1 month ago

Chomenor commented 1 month ago

When a client reconnects using the same IP and port as a previous connection, the engine tries to use the same client slot as the previous connection. If a disconnect command from the client was not received first, the existing slot is not properly freed before starting the new connection.

This can be abused to crash a server by reconnecting MAX_FILE_HANDLES (64) times during a UDP download. It also allows bugs in unpatched mods that can for example cause the CTF flag to disappear from the game if the player holding it reconnects.

This fix ignores the this doesn't work because it nukes the players userinfo comment from the original Q3 code. At this point in the connection the new userinfo is stored in a local variable that shouldn't be affected by the game module disconnect, and the client slot userinfo will be wiped afterwards anyway. I suspect this comment is incorrect or a remnant of an old version of the codebase.

ensiform commented 1 month ago

Have you tested this breaks nothing? There's a reason id did not keep this call there and the original comment tells why they don't call that there, because of userinfo nuking.

Chomenor commented 1 month ago

I did do some basic testing such as simulating reconnects and didn't notice any problems. Do you have a specific idea of something that should be tested?

Based on the code it seems most likely the userinfo comment was incorrect. The userinfo at the time of the GAME_CLIENT_DISCONNECT call is stored in a local variable. It is only copied out to the client slot after the disconnect call is completed. It should not be possible for the disconnect call to "nuke" the userinfo in this situation.

ensiform commented 1 month ago

The ClientDisconnect() function calls trap_SetUserinfo(ENT, "") iirc

ec- commented 1 month ago

The ClientDisconnect() function calls trap_SetUserinfo(ENT, "") iirc

Any negative side-effects from that? If client is not properly disconnected - we still need to call disconnect function for proper cleanup: https://github.com/ec-/baseq3a/blob/master/code/game/g_client.c#L758-L759

Chomenor commented 1 month ago

The ClientDisconnect() function calls trap_SetUserinfo(ENT, "") iirc

It should not be a problem for this fix. The mod can set the client_t userinfo to whatever it wants and it will be wiped as soon as the disconnect call returns, as I've been saying...

Although it shouldn't matter for this fix, I can't find the trap_SetUserinfo call you are referring to. Could you point me to an example?

Any negative side-effects from that? If client is not properly disconnected - we still need to call disconnect function for proper cleanup: https://github.com/ec-/baseq3a/blob/master/code/game/g_client.c#L758-L759

If ClientDisconnect did actually wipe the userinfo here that could be a problem. But it doesn't appear that it does.