Fix varint writing (for real!)

Thanks to @Leymooo for reporting and providing test cases. (I also added one of my own tests which proved the most useful to debugging the issue.)

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

@@ -62,16 +62,13 @@ public enum ProtocolUtils {
   private static final int DEFAULT_MAX_STRING_SIZE = 65536; // 64KiB
   private static final QuietDecoderException BAD_VARINT_CACHED =
       new QuietDecoderException("Bad VarInt decoded");
-  private static final int[] VARINT_BYTE_LENGTHS = new int[33];
+  private static final int[] VARINT_EXACT_BYTE_LENGTHS = new int[33];
 
   static {
-    // Inspired by https://richardstartin.github.io/posts/dont-use-protobuf-for-telemetry
-    // This has been slightly modified in that we reduce the length to 32-bit only, since Velocity
-    // doesn't look at any part of the Minecraft protocol that requires us to look at VarLongs.
     for (int i = 0; i <= 32; ++i) {
-      VARINT_BYTE_LENGTHS[i] = (int) Math.ceil((31d - (i - 1)) / 7d);
+      VARINT_EXACT_BYTE_LENGTHS[i] = (int) Math.ceil((31d - (i - 1)) / 7d);
     }
-    VARINT_BYTE_LENGTHS[32] = 1; // Special case for the number 0.
+    VARINT_EXACT_BYTE_LENGTHS[32] = 1; // Special case for the number 0.
   }
 
   /**
@@ -108,8 +105,13 @@ public enum ProtocolUtils {
     return Integer.MIN_VALUE;
   }
 
+  /**
+   * Returns the exact byte size of {@code value} if it were encoded as a VarInt.
+   * @param value the value to encode
+   * @return the byte size of {@code value} if encoded as a VarInt
+   */
   public static int varIntBytes(int value) {
-    return VARINT_BYTE_LENGTHS[Integer.numberOfLeadingZeros(value)];
+    return VARINT_EXACT_BYTE_LENGTHS[Integer.numberOfLeadingZeros(value)];
   }
 
   /**
@@ -118,8 +120,11 @@ public enum ProtocolUtils {
    * @param value the integer to write
    */
   public static void writeVarInt(ByteBuf buf, int value) {
-    int length = varIntBytes(value);
-    for (int i = 0; i < length; ++i) {
+    // Inspired by https://richardstartin.github.io/posts/dont-use-protobuf-for-telemetry
+    // This has been slightly modified in that we reduce the length to 32-bit only, since Velocity
+    // doesn't look at any part of the Minecraft protocol that requires us to look at VarLongs.
+    int continuationBytes = (31 - Integer.numberOfLeadingZeros(value)) / 7;
+    for (int i = 0; i < continuationBytes; ++i) {
       buf.writeByte(((byte) ((value & 0x7F) | 0x80)));
       value >>>= 7;
     }

proxy/src/test/java/com/velocitypowered/proxy/protocol/ProtocolUtilsTest.java

@@ -17,8 +17,13 @@
 
 package com.velocitypowered.proxy.protocol;
 
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 
+import io.netty.buffer.ByteBuf;
+import io.netty.buffer.ByteBufUtil;
+import io.netty.buffer.Unpooled;
 import org.junit.jupiter.api.Test;
 
 public class ProtocolUtilsTest {
@@ -44,6 +49,95 @@ public class ProtocolUtilsTest {
     }
   }
 
+  @Test
+  void testPositiveOld() {
+    ByteBuf buf = Unpooled.buffer(5);
+    for (int i = 0; i >= 0; i += 127) {
+      writeReadTestOld(buf, i);
+    }
+  }
+
+  @Test
+  void testNegativeOld() {
+    ByteBuf buf = Unpooled.buffer(5);
+    for (int i = 0; i <= 0; i -= 127) {
+      writeReadTestOld(buf, i);
+    }
+  }
+
+  private void writeReadTestOld(ByteBuf buf, int test) {
+    buf.clear();
+    writeVarIntOld(buf, test);
+    assertEquals(test, ProtocolUtils.readVarIntSafely(buf));
+  }
+
+  @Test
+  void testBytesWrittenAtBitBoundaries() {
+    ByteBuf varintNew = Unpooled.buffer(5);
+    ByteBuf varintOld = Unpooled.buffer(5);
+
+    long bytesNew = 0;
+    long bytesOld = 0;
+    for (int bit = 0; bit <= 31; bit++) {
+      int i = (1 << bit) - 1;
+
+      writeVarIntOld(varintOld, i);
+      ProtocolUtils.writeVarInt(varintNew, i);
+      assertArrayEquals(varintOld.array(), varintNew.array(),
+          "Encoding of " + i + " was invalid");
+
+      assertEquals(i, oldReadVarIntSafely(varintNew));
+      assertEquals(i, ProtocolUtils.readVarIntSafely(varintOld));
+
+      varintNew.clear();
+      varintOld.clear();
+    }
+    assertEquals(bytesNew, bytesOld, "byte sizes differ");
+  }
+
+  @Test
+  void testBytesWritten() {
+    ByteBuf varintNew = Unpooled.buffer(5);
+    ByteBuf varintOld = Unpooled.buffer(5);
+
+    long bytesNew = 0;
+    long bytesOld = 0;
+    for (int i = 0; i <= 1_000_000; i++) {
+      ProtocolUtils.writeVarInt(varintNew, i);
+      writeVarIntOld(varintOld, i);
+      bytesNew += varintNew.readableBytes();
+      bytesOld += varintOld.readableBytes();
+      varintNew.clear();
+      varintOld.clear();
+    }
+    assertEquals(bytesNew, bytesOld, "byte sizes differ");
+  }
+
+  private static int oldReadVarIntSafely(ByteBuf buf) {
+    int i = 0;
+    int maxRead = Math.min(5, buf.readableBytes());
+    for (int j = 0; j < maxRead; j++) {
+      int k = buf.readByte();
+      i |= (k & 0x7F) << j * 7;
+      if ((k & 0x80) != 128) {
+        return i;
+      }
+    }
+    return Integer.MIN_VALUE;
+  }
+
+  private void writeVarIntOld(ByteBuf buf, int value) {
+    while (true) {
+      if ((value & 0xFFFFFF80) == 0) {
+        buf.writeByte(value);
+        return;
+      }
+
+      buf.writeByte(value & 0x7F | 0x80);
+      value >>>= 7;
+    }
+  }
+
   private int conventionalWrittenBytes(int value) {
     int wouldBeWritten = 0;
     while (true) {