Skip to content

Fix LegacyPingDecoder misreading modern frames as legacy pings - #1908

Open
SendableMetatype wants to merge 1 commit into
PaperMC:dev/4.0.0from
SendableMetatype:fix/legacy-ping-varint-collision
Open

SendableMetatype wants to merge 1 commit into
PaperMC:dev/4.0.0from
SendableMetatype:fix/legacy-ping-varint-collision

Conversation

@SendableMetatype

Copy link
Copy Markdown

LegacyPingDecoder treats any connection that starts with 0xFE as a possible legacy ping. A modern frame starts with its VarInt length, and that length starts with 0xFE for frame sizes such as 254 or 382 bytes. Unless the client sent exactly FE or FE 01, the decoder parses the bytes as a 1.6 ping. For a modern frame, readExtended16Data then fails with a StringIndexOutOfBoundsException and the connection closes without a log message. Handshakes that carry Floodgate data hit this when the hostname length produces a 254-byte frame.

Changes:

  • The decoder parses a 1.6 ping only when the third byte is 0xFA, the packet ID that every 1.6 ping starts with. Otherwise it passes the bytes to the modern decoder.
  • When only FE or FE 01 has arrived, the decoder waits up to 100 ms for more bytes before it answers with a 1.3 or 1.4 ping. This way, a modern frame or a 1.6 ping that arrives split after those bytes does not get answered as an older ping.

The tests cover a complete modern frame, split modern frames, and both delayed legacy pings.

@jonesdevelopment

Copy link
Copy Markdown
Contributor

Possibly an unpopular opinion, but I think the legacy ping functionality should be removed altogether. Minecraft 1.7.2 was released on October 25, 2013, and Velocity doesn't support versions older than that anyway. Why maintain a feature nobody uses?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants