Skip to content

[BUG] g_maxConnPerIP/g_antiFakePlayer broken? #1146

@TomArrow

Description

@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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions