[BUG] g_maxConnPerIP/g_antiFakePlayer broken?

[TomArrow]

Reporting a bug? Please make sure you've given the following information - thanks!

Operating system and version:
Probably universal

Is this for single player or multiplayer?
multiplayer

Description of the bug (and if possible, steps to reproduce the bug):
Connect 3 (or g_maxConnPerIP) players to a server with g_antiFakePlayer set to 1. Disconnect them all. Now there's a good chance you can no longer connect at all with the error messaage "Too many connections from the same IP". It might depend a bit on timing and which clientNum you get assigned and whether other players join/leave in the meantime.

Looking at the code I think it's easy to see why:

#if 0
    if ( level.clients[i].pers.connected != CON_DISCONNECTED && i != clientNum )
    {
        if ( CompareIPs( clientNum, i ) )
        {
            if ( !level.security.clientConnectionActive[i] )
            {//This IP has a dead connection pending, wait for it to time out
            //    client->pers.connected = CON_DISCONNECTED;
                return "Please wait, another connection from this IP is still pending...";
            }
        }
    }
#else
    if ( CompareIPs( tmpIP, level.clients[i].sess.IP ) )
        count++;
#endif

The code blindly compares against IPs of every clientNum from 0 to 31, even if those players are no longer connected. For some reason (probably to not waste memory writes?) the IPs are not actually cleared from level.clients[i].sess.IP once a client disconnects, therefore past connections count towards the maximum.

With unlucky timing this can lock out a player who connects 3 times over the course of a server's current lifespan (without restart). And if g_maxConnPerIP was set to 1, it would be likely to lock you out after connecting and disconnecting a single time.

The code that is commented out with "#if 0" actually seems to try to do the correct checks (haven't tried to verify but the logic looks right-ish) but it's replaced with a simpler version that does not do these checks.

Possibly introduced in this commit as a whole code of block that immediately came with the "#if 0": b319c52
Or maybe worked back then and some other later change made it stop working.

Then there was an amendment to it here that left the "#if 0" intact: 28903e5

A few more commits that touch it but nothing that would fundamentally change how it works.

Maybe it worked at some point (perhaps the session IPs were indeed being cleared at some point in time?), but now it doesn't seem to.

What did you expect to happen instead?
A disconnected player should not count towards the maximum amount of connections per IP, perhaps by clearing the session IP after a disconnect or by checking in the loop that the player that's being compared against is still connected.

[ensiform]

The commented out block (which is what #if 0 is) can't work with openjk code as it relied on engine hook for jamp only.

There's other ways to look at the list of clients that are only connected but even still, if they are stuck in an in-between state such that caused by crash, they will still be counted until the engine decides they get cleared.

[TomArrow]

I see. Well that would be fine. Normally clients would just time out after a few minutes. But with the current behavior that would just never get cleared at all until a server restart.