From 3dc8e25ec7e9c8f3dac71f0c825dc7cd20fc6ee0 Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Tue, 4 May 2021 16:30:32 -0400 Subject: [PATCH] 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/protocol/ProtocolUtils.java | 23 +++-- .../proxy/protocol/ProtocolUtilsTest.java | 94 +++++++++++++++++++ 2 files changed, 108 insertions(+), 9 deletions(-) diff --git a/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java b/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java index 2591da8cc..9ac62ff69 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java +++ b/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; } diff --git a/proxy/src/test/java/com/velocitypowered/proxy/protocol/ProtocolUtilsTest.java b/proxy/src/test/java/com/velocitypowered/proxy/protocol/ProtocolUtilsTest.java index 40800935d..43b940806 100644 --- a/proxy/src/test/java/com/velocitypowered/proxy/protocol/ProtocolUtilsTest.java +++ b/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) {