Merge branch 'dev/1.1.0' into experiment/io_uring

CONTRIBUTING.md

@@ -28,9 +28,7 @@ sure that you are properly adhering to the code style.
 To reduce bugs and ensure code quality, we run the following tools on all commits
 and pull requests:
 
-* [Checker Framework](https://checkerframework.org/): an enhancement to Java's type
+* [SpotBugs](https://spotbugs.github.io/): ensures that common errors do not
-  system that is designed to help catch bugs. Velocity runs the _Nullness Checker_
+  get into the codebase. The build will fail if SpotBugs finds an issue.
-  and the _Optional Checker_. The build will fail if Checker Framework notices an
-  issue.
 * [Checkstyle](http://checkstyle.sourceforge.net/): ensures that your code is
   correctly formatted. The build will fail if Checkstyle detects a problem.

build.gradle

@@ -21,7 +21,7 @@ allprojects {
     ext {
         // dependency versions
         textVersion = '3.0.4'
-        adventureVersion = '4.3.0'
+        adventureVersion = '4.4.0'
         adventurePlatformVersion = '4.0.0-SNAPSHOT'
         junitVersion = '5.7.0'
         slf4jVersion = '1.7.30'

proxy/build.gradle

@@ -58,6 +58,7 @@ dependencies {
     implementation "org.apache.logging.log4j:log4j-core:${log4jVersion}"
     implementation "org.apache.logging.log4j:log4j-slf4j-impl:${log4jVersion}"
     implementation "org.apache.logging.log4j:log4j-iostreams:${log4jVersion}"
+    implementation "org.apache.logging.log4j:log4j-jul:${log4jVersion}"
 
     implementation 'net.sf.jopt-simple:jopt-simple:5.0.4' // command-line options
     implementation 'net.minecrell:terminalconsoleappender:1.2.0'

proxy/src/main/java/com/velocitypowered/proxy/Velocity.java

@@ -8,9 +8,12 @@ import org.apache.logging.log4j.Logger;
 
 public class Velocity {
 
-  private static final Logger logger = LogManager.getLogger(Velocity.class);
+  private static final Logger logger;
 
   static {
+    System.setProperty("java.util.logging.manager", "org.apache.logging.log4j.jul.LogManager");
+    logger = LogManager.getLogger(Velocity.class);
+
     // We use BufferedImage for favicons, and on macOS this puts the Java application in the dock.
     // How inconvenient. Force AWT to work with its head chopped off.
     System.setProperty("java.awt.headless", "true");

proxy/src/main/java/com/velocitypowered/proxy/command/builtin/VelocityCommand.java

@@ -38,6 +38,7 @@ import net.kyori.adventure.text.TextComponent;
 import net.kyori.adventure.text.event.ClickEvent;
 import net.kyori.adventure.text.event.HoverEvent;
 import net.kyori.adventure.text.format.NamedTextColor;
+import net.kyori.adventure.text.format.TextColor;
 import net.kyori.adventure.text.format.TextDecoration;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
@@ -188,6 +189,7 @@ public class VelocityCommand implements SimpleCommand {
 
   private static class Info implements SubCommand {
 
+    private static final TextColor VELOCITY_COLOR = TextColor.fromHexString("#09add3");
     private final ProxyServer server;
 
     private Info(ProxyServer server) {
@@ -205,7 +207,7 @@ public class VelocityCommand implements SimpleCommand {
 
       TextComponent velocity = Component.text().content(version.getName() + " ")
           .decoration(TextDecoration.BOLD, true)
-          .color(NamedTextColor.DARK_AQUA)
+          .color(VELOCITY_COLOR)
           .append(Component.text(version.getVersion()).decoration(TextDecoration.BOLD, false))
           .build();
       TextComponent copyright = Component

proxy/src/main/java/com/velocitypowered/proxy/config/VelocityConfiguration.java

@@ -458,7 +458,7 @@ public class VelocityConfiguration implements ProxyConfig {
         PingPassthroughMode.DISABLED);
 
     String bind = config.getOrElse("bind", "0.0.0.0:25577");
-    String motd = config.getOrElse("motd", "&3A Velocity Server");
+    String motd = config.getOrElse("motd", "&#09add3A Velocity Server");
     int maxPlayers = config.getIntOrElse("show-max-players", 500);
     Boolean onlineMode = config.getOrElse("online-mode", true);
     Boolean announceForge = config.getOrElse("announce-forge", true);

proxy/src/main/java/com/velocitypowered/proxy/connection/backend/BackendPlaySessionHandler.java

@@ -83,7 +83,7 @@ public class BackendPlaySessionHandler implements MinecraftSessionHandler {
 
   @Override
   public boolean handle(KeepAlive packet) {
-    serverConn.setLastPingId(packet.getRandomId());
+    serverConn.getPendingPings().put(packet.getRandomId(), System.currentTimeMillis());
     return false; // forwards on
   }
 

proxy/src/main/java/com/velocitypowered/proxy/connection/backend/VelocityServerConnection.java

@@ -29,7 +29,9 @@ import io.netty.buffer.Unpooled;
 import io.netty.channel.ChannelFutureListener;
 import java.net.InetSocketAddress;
 import java.nio.charset.StandardCharsets;
+import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
 import java.util.concurrent.CompletableFuture;
 import java.util.function.UnaryOperator;
 import org.checkerframework.checker.nullness.qual.MonotonicNonNull;
@@ -44,8 +46,7 @@ public class VelocityServerConnection implements MinecraftConnectionAssociation,
   private boolean hasCompletedJoin = false;
   private boolean gracefulDisconnect = false;
   private BackendConnectionPhase connectionPhase = BackendConnectionPhases.UNKNOWN;
-  private long lastPingId;
+  private final Map<Long, Long> pendingPings = new HashMap<>();
-  private long lastPingSent;
   private @MonotonicNonNull DimensionRegistry activeDimensionRegistry;
 
   /**
@@ -244,21 +245,8 @@ public class VelocityServerConnection implements MinecraftConnectionAssociation,
     return gracefulDisconnect;
   }
 
-  public long getLastPingId() {
+  public Map<Long, Long> getPendingPings() {
-    return lastPingId;
+    return pendingPings;
-  }
-
-  public long getLastPingSent() {
-    return lastPingSent;
-  }
-
-  void setLastPingId(long lastPingId) {
-    this.lastPingId = lastPingId;
-    this.lastPingSent = System.currentTimeMillis();
-  }
-
-  public void resetLastPingId() {
-    this.lastPingId = -1;
   }
 
   /**

proxy/src/main/java/com/velocitypowered/proxy/connection/client/ClientPlaySessionHandler.java

@@ -99,12 +99,14 @@ public class ClientPlaySessionHandler implements MinecraftSessionHandler {
   @Override
   public boolean handle(KeepAlive packet) {
     VelocityServerConnection serverConnection = player.getConnectedServer();
-    if (serverConnection != null && packet.getRandomId() == serverConnection.getLastPingId()) {
+    if (serverConnection != null) {
+      Long sentTime = serverConnection.getPendingPings().remove(packet.getRandomId());
+      if (sentTime != null) {
         MinecraftConnection smc = serverConnection.getConnection();
         if (smc != null) {
-        player.setPing(System.currentTimeMillis() - serverConnection.getLastPingSent());
+          player.setPing(System.currentTimeMillis() - sentTime);
           smc.write(packet);
-        serverConnection.resetLastPingId();
+        }
       }
     }
     return true;

proxy/src/main/java/com/velocitypowered/proxy/connection/client/LoginSessionHandler.java

@@ -36,6 +36,7 @@ import io.netty.buffer.ByteBuf;
 import java.net.InetSocketAddress;
 import java.security.GeneralSecurityException;
 import java.security.KeyPair;
+import java.security.MessageDigest;
 import java.util.Arrays;
 import java.util.Optional;
 import java.util.UUID;
@@ -90,7 +91,7 @@ public class LoginSessionHandler implements MinecraftSessionHandler {
     try {
       KeyPair serverKeyPair = server.getServerKeyPair();
       byte[] decryptedVerifyToken = decryptRsa(serverKeyPair, packet.getVerifyToken());
-      if (!Arrays.equals(verify, decryptedVerifyToken)) {
+      if (!MessageDigest.isEqual(verify, decryptedVerifyToken)) {
         throw new IllegalStateException("Unable to successfully decrypt the verification token.");
       }
 

proxy/src/main/java/com/velocitypowered/proxy/protocol/MinecraftPacket.java

@@ -11,4 +11,12 @@ public interface MinecraftPacket {
   void encode(ByteBuf buf, ProtocolUtils.Direction direction, ProtocolVersion protocolVersion);
 
   boolean handle(MinecraftSessionHandler handler);
+
+  default int expectedMaxLength(ByteBuf buf, ProtocolUtils.Direction direction, ProtocolVersion version) {
+    return -1;
+  }
+
+  default int expectedMinLength(ByteBuf buf, ProtocolUtils.Direction direction, ProtocolVersion version) {
+    return 0;
+  }
 }

proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java

@@ -16,6 +16,8 @@ import io.netty.handler.codec.CorruptedFrameException;
 import io.netty.handler.codec.DecoderException;
 import io.netty.handler.codec.EncoderException;
 
+import java.io.DataInput;
+import java.io.DataOutput;
 import java.io.IOException;
 import java.nio.charset.StandardCharsets;
 import java.util.ArrayList;
@@ -227,11 +229,12 @@ public enum ProtocolUtils {
   /**
    * Reads a {@link net.kyori.adventure.nbt.CompoundBinaryTag} from the {@code buf}.
    * @param buf the buffer to read from
+   * @param reader the NBT reader to use
    * @return {@link net.kyori.adventure.nbt.CompoundBinaryTag} the CompoundTag from the buffer
    */
-  public static CompoundBinaryTag readCompoundTag(ByteBuf buf) {
+  public static CompoundBinaryTag readCompoundTag(ByteBuf buf, BinaryTagIO.Reader reader) {
     try {
-      return BinaryTagIO.readDataInput(new ByteBufInputStream(buf));
+      return reader.read((DataInput) new ByteBufInputStream(buf));
     } catch (IOException thrown) {
       throw new DecoderException(
               "Unable to parse NBT CompoundTag, full error: " + thrown.getMessage());
@@ -245,7 +248,7 @@ public enum ProtocolUtils {
    */
   public static void writeCompoundTag(ByteBuf buf, CompoundBinaryTag compoundTag) {
     try {
-      BinaryTagIO.writeDataOutput(compoundTag, new ByteBufOutputStream(buf));
+      BinaryTagIO.writer().write(compoundTag, (DataOutput) new ByteBufOutputStream(buf));
     } catch (IOException e) {
       throw new EncoderException("Unable to encode NBT CompoundTag");
     }

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

@@ -58,6 +58,8 @@ public class MinecraftDecoder extends ChannelInboundHandlerAdapter {
       ctx.fireChannelRead(buf);
     } else {
       try {
+        doLengthSanityChecks(buf, packet);
+
         try {
           packet.decode(buf, direction, registry.version);
         } catch (Exception e) {
@@ -65,7 +67,7 @@ public class MinecraftDecoder extends ChannelInboundHandlerAdapter {
         }
 
         if (buf.isReadable()) {
-          throw handleNotReadEnough(packet, packetId);
+          throw handleOverflow(packet, buf.readerIndex(), buf.writerIndex());
         }
         ctx.fireChannelRead(packet);
       } finally {
@@ -74,10 +76,30 @@ public class MinecraftDecoder extends ChannelInboundHandlerAdapter {
     }
   }
 
-  private Exception handleNotReadEnough(MinecraftPacket packet, int packetId) {
+  private void doLengthSanityChecks(ByteBuf buf, MinecraftPacket packet) throws Exception {
+    int expectedMinLen = packet.expectedMinLength(buf, direction, registry.version);
+    int expectedMaxLen = packet.expectedMaxLength(buf, direction, registry.version);
+    if (expectedMaxLen != -1 && buf.readableBytes() > expectedMaxLen) {
+      throw handleOverflow(packet, expectedMaxLen, buf.readableBytes());
+    }
+    if (buf.readableBytes() < expectedMinLen) {
+      throw handleUnderflow(packet, expectedMaxLen, buf.readableBytes());
+    }
+  }
+
+  private Exception handleOverflow(MinecraftPacket packet, int expected, int actual) {
     if (DEBUG) {
-      return new CorruptedFrameException("Did not read full packet for " + packet.getClass() + " "
+      return new CorruptedFrameException("Packet sent for " + packet.getClass() + " was too "
-          + getExtraConnectionDetail(packetId));
+          + "big (expected " + expected + " bytes, got " + actual + " bytes)");
+    } else {
+      return DECODE_FAILED;
+    }
+  }
+
+  private Exception handleUnderflow(MinecraftPacket packet, int expected, int actual) {
+    if (DEBUG) {
+      return new CorruptedFrameException("Packet sent for " + packet.getClass() + " was too "
+          + "small (expected " + expected + " bytes, got " + actual + " bytes)");
     } else {
       return DECODE_FAILED;
     }

proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/EncryptionResponse.java

@@ -6,6 +6,7 @@ import com.velocitypowered.api.network.ProtocolVersion;
 import com.velocitypowered.proxy.connection.MinecraftSessionHandler;
 import com.velocitypowered.proxy.protocol.MinecraftPacket;
 import com.velocitypowered.proxy.protocol.ProtocolUtils;
+import com.velocitypowered.proxy.protocol.ProtocolUtils.Direction;
 import io.netty.buffer.ByteBuf;
 import java.util.Arrays;
 
@@ -33,7 +34,7 @@ public class EncryptionResponse implements MinecraftPacket {
   @Override
   public void decode(ByteBuf buf, ProtocolUtils.Direction direction, ProtocolVersion version) {
     if (version.compareTo(ProtocolVersion.MINECRAFT_1_8) >= 0) {
-      this.sharedSecret = ProtocolUtils.readByteArray(buf, 256);
+      this.sharedSecret = ProtocolUtils.readByteArray(buf, 128);
       this.verifyToken = ProtocolUtils.readByteArray(buf, 128);
     } else {
       this.sharedSecret = ProtocolUtils.readByteArray17(buf);
@@ -56,4 +57,16 @@ public class EncryptionResponse implements MinecraftPacket {
   public boolean handle(MinecraftSessionHandler handler) {
     return handler.handle(this);
   }
+
+  @Override
+  public int expectedMaxLength(ByteBuf buf, Direction direction, ProtocolVersion version) {
+    // It turns out these come out to the same length, whether we're talking >=1.8 or not.
+    // The length prefix always winds up being 2 bytes.
+    return 260;
+  }
+
+  @Override
+  public int expectedMinLength(ByteBuf buf, Direction direction, ProtocolVersion version) {
+    return expectedMaxLength(buf, direction, version);
+  }
 }

proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/JoinGame.java

@@ -8,6 +8,7 @@ import com.velocitypowered.proxy.connection.registry.DimensionInfo;
 import com.velocitypowered.proxy.connection.registry.DimensionRegistry;
 import com.velocitypowered.proxy.protocol.*;
 import io.netty.buffer.ByteBuf;
+import net.kyori.adventure.nbt.BinaryTagIO;
 import net.kyori.adventure.nbt.BinaryTagTypes;
 import net.kyori.adventure.nbt.CompoundBinaryTag;
 import net.kyori.adventure.nbt.ListBinaryTag;
@@ -15,6 +16,7 @@ import org.checkerframework.checker.nullness.qual.Nullable;
 
 public class JoinGame implements MinecraftPacket {
 
+  private static final BinaryTagIO.Reader JOINGAME_READER = BinaryTagIO.reader(2 * 1024 * 1024);
   private int entityId;
   private short gamemode;
   private int dimension;
@@ -178,7 +180,7 @@ public class JoinGame implements MinecraftPacket {
     if (version.compareTo(ProtocolVersion.MINECRAFT_1_16) >= 0) {
       this.previousGamemode = buf.readByte();
       ImmutableSet<String> levelNames = ImmutableSet.copyOf(ProtocolUtils.readStringArray(buf));
-      CompoundBinaryTag registryContainer = ProtocolUtils.readCompoundTag(buf);
+      CompoundBinaryTag registryContainer = ProtocolUtils.readCompoundTag(buf, JOINGAME_READER);
       ListBinaryTag dimensionRegistryContainer = null;
       if (version.compareTo(ProtocolVersion.MINECRAFT_1_16_2) >= 0) {
         dimensionRegistryContainer = registryContainer.getCompound("minecraft:dimension_type")
@@ -192,7 +194,7 @@ public class JoinGame implements MinecraftPacket {
           DimensionRegistry.fromGameData(dimensionRegistryContainer, version);
       this.dimensionRegistry = new DimensionRegistry(readData, levelNames);
       if (version.compareTo(ProtocolVersion.MINECRAFT_1_16_2) >= 0) {
-        CompoundBinaryTag currentDimDataTag = ProtocolUtils.readCompoundTag(buf);
+        CompoundBinaryTag currentDimDataTag = ProtocolUtils.readCompoundTag(buf, JOINGAME_READER);
         dimensionIdentifier = ProtocolUtils.readString(buf);
         this.currentDimensionData = DimensionData.decodeBaseCompoundTag(currentDimDataTag, version)
             .annotateWith(dimensionIdentifier, null);

proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/Respawn.java

@@ -7,6 +7,7 @@ import com.velocitypowered.proxy.connection.registry.DimensionInfo;
 import com.velocitypowered.proxy.protocol.MinecraftPacket;
 import com.velocitypowered.proxy.protocol.ProtocolUtils;
 import io.netty.buffer.ByteBuf;
+import net.kyori.adventure.nbt.BinaryTagIO;
 import net.kyori.adventure.nbt.CompoundBinaryTag;
 
 public class Respawn implements MinecraftPacket {
@@ -116,7 +117,7 @@ public class Respawn implements MinecraftPacket {
     String levelName = null;
     if (version.compareTo(ProtocolVersion.MINECRAFT_1_16) >= 0) {
       if (version.compareTo(ProtocolVersion.MINECRAFT_1_16_2) >= 0) {
-        CompoundBinaryTag dimDataTag = ProtocolUtils.readCompoundTag(buf);
+        CompoundBinaryTag dimDataTag = ProtocolUtils.readCompoundTag(buf, BinaryTagIO.reader());
         dimensionIdentifier = ProtocolUtils.readString(buf);
         this.currentDimensionData = DimensionData.decodeBaseCompoundTag(dimDataTag, version)
             .annotateWith(dimensionIdentifier, null);

proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/ServerLogin.java

@@ -5,6 +5,7 @@ import com.velocitypowered.api.network.ProtocolVersion;
 import com.velocitypowered.proxy.connection.MinecraftSessionHandler;
 import com.velocitypowered.proxy.protocol.MinecraftPacket;
 import com.velocitypowered.proxy.protocol.ProtocolUtils;
+import com.velocitypowered.proxy.protocol.ProtocolUtils.Direction;
 import com.velocitypowered.proxy.util.except.QuietDecoderException;
 import io.netty.buffer.ByteBuf;
 import org.checkerframework.checker.nullness.qual.Nullable;
@@ -52,6 +53,13 @@ public class ServerLogin implements MinecraftPacket {
     ProtocolUtils.writeString(buf, username);
   }
 
+  @Override
+  public int expectedMaxLength(ByteBuf buf, Direction direction, ProtocolVersion version) {
+    // Accommodate the rare (but likely malicious) use of UTF-8 usernames, since it is technically
+    // legal on the protocol level.
+    return 1 + (16 * 4);
+  }
+
   @Override
   public boolean handle(MinecraftSessionHandler handler) {
     return handler.handle(this);

proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/StatusPing.java

@@ -4,6 +4,7 @@ import com.velocitypowered.api.network.ProtocolVersion;
 import com.velocitypowered.proxy.connection.MinecraftSessionHandler;
 import com.velocitypowered.proxy.protocol.MinecraftPacket;
 import com.velocitypowered.proxy.protocol.ProtocolUtils;
+import com.velocitypowered.proxy.protocol.ProtocolUtils.Direction;
 import io.netty.buffer.ByteBuf;
 
 public class StatusPing implements MinecraftPacket {
@@ -24,4 +25,14 @@ public class StatusPing implements MinecraftPacket {
   public boolean handle(MinecraftSessionHandler handler) {
     return handler.handle(this);
   }
+
+  @Override
+  public int expectedMaxLength(ByteBuf buf, Direction direction, ProtocolVersion version) {
+    return 8;
+  }
+
+  @Override
+  public int expectedMinLength(ByteBuf buf, Direction direction, ProtocolVersion version) {
+    return 8;
+  }
 }

proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/StatusRequest.java

@@ -4,6 +4,7 @@ import com.velocitypowered.api.network.ProtocolVersion;
 import com.velocitypowered.proxy.connection.MinecraftSessionHandler;
 import com.velocitypowered.proxy.protocol.MinecraftPacket;
 import com.velocitypowered.proxy.protocol.ProtocolUtils;
+import com.velocitypowered.proxy.protocol.ProtocolUtils.Direction;
 import io.netty.buffer.ByteBuf;
 
 public class StatusRequest implements MinecraftPacket {
@@ -33,4 +34,9 @@ public class StatusRequest implements MinecraftPacket {
   public boolean handle(MinecraftSessionHandler handler) {
     return handler.handle(this);
   }
+
+  @Override
+  public int expectedMaxLength(ByteBuf buf, Direction direction, ProtocolVersion version) {
+    return 0;
+  }
 }

proxy/src/main/resources/default-velocity.toml

@@ -6,7 +6,7 @@ bind = "0.0.0.0:25577"
 
 # What should be the MOTD? This gets displayed when the player adds your server to
 # their server list. Legacy color codes and JSON are accepted.
-motd = "&3A Velocity Server"
+motd = "&#09add3A Velocity Server"
 
 # What should we display for the maximum number of players? (Velocity does not support a cap
 # on the number of players online.)