Fix LegacyPingDecoder misreading modern frames as legacy pings

proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/LegacyPingDecoder.java

@@ -28,13 +28,21 @@
 import java.net.InetSocketAddress;
 import java.nio.charset.StandardCharsets;
 import java.util.List;
+import java.util.concurrent.ScheduledFuture;
+import java.util.concurrent.TimeUnit;
 
 /**
  * Decodes Minecraft 1.3-1.6.4 server ping requests.
  */
 public class LegacyPingDecoder extends ByteToMessageDecoder {
 
   private static final String MC_1_6_CHANNEL = "MC|PingHost";
+  // FE and FE 01 are complete legacy pings, but also the beginning of a modern 254-byte frame.
+  // Give a fragmented modern frame a chance to supply its packet ID before choosing legacy.
+  private static final long LEGACY_PING_GRACE_PERIOD_MILLIS = 100;
+
+  private ScheduledFuture<?> pendingLegacyPing;
+  private int pendingLegacyPingLength;
 
   @Override
   protected void decode(ChannelHandlerContext ctx, ByteBuf in, List<Object> out) throws Exception {
@@ -52,27 +60,90 @@ protected void decode(ChannelHandlerContext ctx, ByteBuf in, List<Object> out) t
     if (first == 0xfe) {
       // possibly a ping
       if (!in.isReadable()) {
-        out.add(new LegacyPingPacket(LegacyMinecraftPingVersion.MINECRAFT_1_3));
+        in.readerIndex(originalReaderIndex);
+        scheduleLegacyPing(ctx, 1, LegacyMinecraftPingVersion.MINECRAFT_1_3);
         return;
       }
 
       short next = in.readUnsignedByte();
-      if (next == 1 && !in.isReadable()) {
-        out.add(new LegacyPingPacket(LegacyMinecraftPingVersion.MINECRAFT_1_4));
-        return;
+      if (next == 1) {
+        if (!in.isReadable()) {
+          in.readerIndex(originalReaderIndex);
+          scheduleLegacyPing(ctx, 2, LegacyMinecraftPingVersion.MINECRAFT_1_4);
+          return;
+        }
+        if (in.getUnsignedByte(in.readerIndex()) == 0xFA) {
+          cancelPendingLegacyPing();
+          out.add(readExtended16Data(in));
+          return;
+        }
       }
 
-      // We got a 1.6.x ping. Let's chomp off the stuff we don't need.
-      out.add(readExtended16Data(in));
+      // Not a legacy ping. Reset and let the modern decoder handle it.
+      cancelPendingLegacyPing();
+      in.readerIndex(originalReaderIndex);
+      ctx.pipeline().remove(this);
     } else if (first == 0x02 && in.isReadable()) {
+      cancelPendingLegacyPing();
       in.skipBytes(in.readableBytes());
       out.add(new LegacyHandshakePacket());
     } else {
+      cancelPendingLegacyPing();
       in.readerIndex(originalReaderIndex);
       ctx.pipeline().remove(this);
     }
   }
 
+  private void scheduleLegacyPing(ChannelHandlerContext ctx, int length,
+                                  LegacyMinecraftPingVersion version) {
+    if (pendingLegacyPing != null && pendingLegacyPingLength == length) {
+      return;
+    }
+
+    cancelPendingLegacyPing();
+    pendingLegacyPingLength = length;
+    pendingLegacyPing = ctx.executor().schedule(() -> {
+      pendingLegacyPing = null;
+      if (!ctx.channel().isActive() || pendingLegacyPingLength != length) {
+        return;
+      }
+
+      ByteBuf buffer = internalBuffer();
+      if (buffer.readableBytes() != length) {
+        pendingLegacyPingLength = 0;
+        return;
+      }
+
+      buffer.skipBytes(length);
+      pendingLegacyPingLength = 0;
+      ctx.fireChannelRead(new LegacyPingPacket(version));
+      ctx.fireChannelReadComplete();
+      if (ctx.pipeline().context(this) != null) {
+        ctx.pipeline().remove(this);
+      }
+    }, LEGACY_PING_GRACE_PERIOD_MILLIS, TimeUnit.MILLISECONDS);
+  }
+
+  private void cancelPendingLegacyPing() {
+    if (pendingLegacyPing != null) {
+      pendingLegacyPing.cancel(false);
+      pendingLegacyPing = null;
+    }
+    pendingLegacyPingLength = 0;
+  }
+
+  @Override
+  protected void handlerRemoved0(ChannelHandlerContext ctx) throws Exception {
+    cancelPendingLegacyPing();
+    super.handlerRemoved0(ctx);
+  }
+
+  @Override
+  public void channelInactive(ChannelHandlerContext ctx) throws Exception {
+    cancelPendingLegacyPing();
+    super.channelInactive(ctx);
+  }
+
   private static LegacyPingPacket readExtended16Data(ByteBuf in) {
     in.skipBytes(1);
     String channelName = readLegacyString(in);

proxy/src/test/java/com/velocitypowered/proxy/protocol/netty/LegacyPingDecoderTest.java

@@ -0,0 +1,109 @@
+/*
+ * Copyright (C) 2018-2026 Velocity Contributors
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <https://www.gnu.org/licenses/>.
+ */
+
+package com.velocitypowered.proxy.protocol.netty;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertInstanceOf;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import com.velocitypowered.proxy.protocol.packet.LegacyPingPacket;
+import com.velocitypowered.proxy.protocol.packet.legacyping.LegacyMinecraftPingVersion;
+import io.netty.buffer.ByteBuf;
+import io.netty.buffer.ByteBufUtil;
+import io.netty.buffer.Unpooled;
+import io.netty.channel.embedded.EmbeddedChannel;
+import java.util.concurrent.TimeUnit;
+import org.junit.jupiter.api.Test;
+
+class LegacyPingDecoderTest {
+
+  private static final byte[] MODERN_PACKET = {(byte) 0xFE, 0x01, 0x00, 0x01, 0x02};
+
+  @Test
+  void forwardsUnfragmentedModernPacket() {
+    EmbeddedChannel channel = new EmbeddedChannel(new LegacyPingDecoder());
+    try {
+      assertTrue(channel.writeInbound(Unpooled.wrappedBuffer(MODERN_PACKET)));
+      assertModernPacketReceived(channel);
+    } finally {
+      channel.finishAndReleaseAll();
+    }
+  }
+
+  @Test
+  void forwardsModernPacketSplitAfterFirstByte() {
+    assertModernPacketForwarded(1);
+  }
+
+  @Test
+  void forwardsModernPacketSplitAfterSecondByte() {
+    assertModernPacketForwarded(2);
+  }
+
+  @Test
+  void emitsSingleByteLegacyPingAfterGracePeriod() {
+    assertLegacyPing(new byte[]{(byte) 0xFE}, LegacyMinecraftPingVersion.MINECRAFT_1_3);
+  }
+
+  @Test
+  void emitsTwoByteLegacyPingAfterGracePeriod() {
+    assertLegacyPing(new byte[]{(byte) 0xFE, 0x01},
+        LegacyMinecraftPingVersion.MINECRAFT_1_4);
+  }
+
+  private static void assertModernPacketForwarded(int splitIndex) {
+    EmbeddedChannel channel = new EmbeddedChannel(new LegacyPingDecoder());
+    try {
+      assertFalse(channel.writeInbound(Unpooled.wrappedBuffer(MODERN_PACKET, 0, splitIndex)));
+      assertTrue(channel.writeInbound(
+          Unpooled.wrappedBuffer(MODERN_PACKET, splitIndex, MODERN_PACKET.length - splitIndex)));
+      assertModernPacketReceived(channel);
+    } finally {
+      channel.finishAndReleaseAll();
+    }
+  }
+
+  private static void assertModernPacketReceived(EmbeddedChannel channel) {
+    ByteBuf forwarded = channel.readInbound();
+    try {
+      assertEquals(ByteBufUtil.hexDump(MODERN_PACKET), ByteBufUtil.hexDump(forwarded));
+    } finally {
+      forwarded.release();
+    }
+    assertNull(channel.readInbound());
+  }
+
+  private static void assertLegacyPing(byte[] request, LegacyMinecraftPingVersion version) {
+    EmbeddedChannel channel = new EmbeddedChannel(new LegacyPingDecoder());
+    try {
+      assertFalse(channel.writeInbound(Unpooled.wrappedBuffer(request)));
+      assertNull(channel.readInbound());
+
+      channel.advanceTimeBy(1, TimeUnit.SECONDS);
+      channel.runScheduledPendingTasks();
+
+      LegacyPingPacket ping = assertInstanceOf(LegacyPingPacket.class, channel.readInbound());
+      assertEquals(version, ping.getVersion());
+      assertNull(channel.readInbound());
+    } finally {
+      channel.finishAndReleaseAll();
+    }
+  }
+}