Fix server resource leaks during client reconnect #281
Conversation
Ensure client disconnect functions are called if a client reconnects without sending a disconnect command first. Fixes potential memory leaks, file handle leaks (particularly during UDP downloads), and bugs in unpatched mods (such as CTF flags disappearing from the game if the player holding it reconnects).
|
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. |
|
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. |
|
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 |
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?
If ClientDisconnect did actually wipe the userinfo here that could be a problem. But it doesn't appear that it does. |
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 userinfocomment 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.