Fix sv_floodprotect dropping chat messages on player death

[danielepantaleone]

This PR should fix https://github.com/FrozenSand/UrbanTerror4/issues/261

[Barbatos]

Thank you. It seems to work fine on my end. Tested on 4.3 RC3.

[t3slider]

I don't mean to pry, but how is this better than just setting sv_floodprotect to 2 (or 3)? Now it is no longer configurable, so it's either all or nothing again, like it used to be; either you turn flood protection off entirely or you get 2 commands every 1.5 seconds (IIRC it used to be 1 every 1 second). Before this patch you could allow eg. 5 commands per second (approximately) if you wanted to be permissive but still prevent abuse. I should say I haven't tested this specific use case but it seems like this was more of a sub-optimal server config (or default setting) than a bug that needed to be fixed, and the fix is a functional regression.

[Barbatos]

In fact I didn't remember that we added the ability to set the cvar sv_floodprotect to more than 1. I agree with @t3slider, this change is a regression for server admins who used to set the cvar to some higher values.

I'll revert the changes for now unless someone comes up with something else. I think that an easy fix would be to set sv_floodprotect to 2 by default instead of 1. That way chat messages shouldn't get dropped and the flood protection is still satisfying.

What are your thoughts on this @danielepantaleone & @t3slider ?

[danielepantaleone]

@Barbatos as far as I can see here sv_floodprotect is never integer compared, but always treated like a boolean, so what @t3slider is asserting is not true when using this version of the engine (but maybe he is using another version). If you want to go for the other option you would need to implement the change. The patch I proposed has been originally created by Rambetter and I'm currently adopting it in my ioq3 fork.

EDIT: I guess that just changing:

cl->nextReliableTime = svs.time + 1000;

to

cl->nextReliableTime = svs.time + sv_floodProtect->integer * 1000;

will do the job.

[Barbatos]

Thanks for your reply!

As far as I can see myself, sv_floodprotect can currently be used to set how many client commands you can perform in one second : https://github.com/Barbatos/ioq3-for-UrbanTerror-4/blob/master/code/server/sv_client.c#L1499

Commented code:

So eg. if we set sv_floodprotect to 3 we can effectively send 3 client commands per second.

Am I reading it wrong?

[t3slider]

The integer (non-boolean) comparison from your link is on line 1499: if (++(cl->numcmds) > sv_floodProtect->integer ) {

Doing cl->nextReliableTime = svs.time + sv_floodProtect->integer * 1000; has the potential to block user commands for sv_floodProtect seconds if the limit is reached (instead of allowing sv_floodProtect commands in one second). Right now it isn't really 'sv_floodProtect commands in 1 second' because the time gets extended with each user-command on success or failure (so if you have reached the limit and continue to flood, it will block you until you stop flooding instead of just letting you continue after a second is up), but it's close enough in practice not to bother changing unless you literally need 'x commands in 1 second'.

If you want proof that this works, start a server with sv_floodprotect set to 3 and wear tacs. Toggle ut_itemuse multiple times in a row and see what happens.

[edit] And changing the default to 2 sounds reasonable to me, though I don't know how often server administrators actually update their configs from upstream.