Skip to content

Fix connection throttle cleanup wiping the whole tracker#14011

Open
gunjanjaswal wants to merge 1 commit into
PaperMC:mainfrom
gunjanjaswal:fix/13984-connection-throttle-cleanup
Open

Fix connection throttle cleanup wiping the whole tracker#14011
gunjanjaswal wants to merge 1 commit into
PaperMC:mainfrom
gunjanjaswal:fix/13984-connection-throttle-cleanup

Conversation

@gunjanjaswal

Copy link
Copy Markdown

Problem

The connection throttle cleanup in ServerHandshakePacketListenerImpl wipes the entire throttleTracker every ~200 connections. The cleanup predicate compares a stored timestamp against the throttle duration:

throttleTracker.values().removeIf(time -> time > connectionThrottle);

time is an epoch-millisecond timestamp (~1.7e12); connectionThrottle is a duration (default 4000ms). So time > connectionThrottle is always true and every entry is removed. The check a few lines above already does it correctly:

currentTime - throttleTracker.get(address) < connectionThrottle

Within a cleanup cycle the per-IP throttle works, but every 200th connection clears the map, so a client can time reconnects around the cleanup and slip past the throttle.

Fix

Compare elapsed time, reusing the currentTime already computed in the same block:

throttleTracker.values().removeIf(time -> currentTime - time > connectionThrottle);

This is a content-only change to the existing added line in the patch, so it applies cleanly with no rebuildPatches churn.

Fixes #13984.

The stale-entry cleanup compared a stored timestamp directly against the
throttle duration (`time > connectionThrottle`). A real epoch timestamp
is always far greater than the configured duration (default 4000ms), so
the predicate matched every entry and cleared the entire throttleTracker
every ~200 connections. Between cleanups a client could reconnect around
the cycle and dodge the per-IP throttle.

Compare elapsed time instead (`currentTime - time > connectionThrottle`),
matching the check used a few lines above where the same `currentTime`
is already computed.

Fixes PaperMC#13984.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Awaiting review

Development

Successfully merging this pull request may close these issues.

Connection throttle cleanup predicate compares timestamp against duration — always removes all entries

2 participants