From c2795892fdd654b333fd72a289f58af507212931 Mon Sep 17 00:00:00 2001 From: valaphee <32491319+valaphee@users.noreply.github.com> Date: Mon, 19 Aug 2024 14:37:45 +0200 Subject: [PATCH 1/2] Fix readVarLong, remove multiplication in readVarInt --- .../network/codec/BasePacketCodecHelper.java | 20 ++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/protocol/src/main/java/org/geysermc/mcprotocollib/network/codec/BasePacketCodecHelper.java b/protocol/src/main/java/org/geysermc/mcprotocollib/network/codec/BasePacketCodecHelper.java index 6aec58f54..669740287 100644 --- a/protocol/src/main/java/org/geysermc/mcprotocollib/network/codec/BasePacketCodecHelper.java +++ b/protocol/src/main/java/org/geysermc/mcprotocollib/network/codec/BasePacketCodecHelper.java @@ -18,13 +18,14 @@ public int readVarInt(ByteBuf buf) { int size = 0; int b; while (((b = buf.readByte()) & 0x80) == 0x80) { - value |= (b & 0x7F) << (size++ * 7); - if (size > 5) { - throw new IllegalArgumentException("VarInt too long (length must be <= 5)"); + value |= (b & 0x7F) << size; + size += 7; + if (size > 35) { + throw new IllegalArgumentException("VarInt wider than 35-bit"); } } - return value | ((b & 0x7F) << (size * 7)); + return value | ((b & 0x7F) << size); } // Based off of Andrew Steinborn's blog post: @@ -127,17 +128,18 @@ private static void writeVarLongFull(ByteBuf buf, long value) { @Override public long readVarLong(ByteBuf buf) { - int value = 0; + long value = 0; int size = 0; int b; while (((b = buf.readByte()) & 0x80) == 0x80) { - value |= (b & 0x7F) << (size++ * 7); - if (size > 10) { - throw new IllegalArgumentException("VarLong too long (length must be <= 10)"); + value |= (b & 0x7FL) << size; + size += 7; + if (size > 70) { + throw new IllegalArgumentException("VarLong wider than 70-bit"); } } - return value | ((b & 0x7FL) << (size * 7)); + return value | ((b & 0x7FL) << size); } @Override From cb596f1105a499161dfb9c03918b3745a9d77dd7 Mon Sep 17 00:00:00 2001 From: valaphee <32491319+valaphee@users.noreply.github.com> Date: Mon, 16 Sep 2024 16:02:09 +0200 Subject: [PATCH 2/2] Simplify readVarInt/Long, Fix and simplify writeVarInt/Long, add some simple tests to ensure functionality, VarInt/Long wider than 5/10 bytes will now take precedence over index out of bounds --- .../network/codec/BasePacketCodecHelper.java | 135 ++++-------------- .../codec/BasePacketCodecHelperTest.java | 50 +++++++ 2 files changed, 78 insertions(+), 107 deletions(-) create mode 100644 protocol/src/test/java/org/geysermc/mcprotocollib/protocol/codec/BasePacketCodecHelperTest.java diff --git a/protocol/src/main/java/org/geysermc/mcprotocollib/network/codec/BasePacketCodecHelper.java b/protocol/src/main/java/org/geysermc/mcprotocollib/network/codec/BasePacketCodecHelper.java index 669740287..e21df42f7 100644 --- a/protocol/src/main/java/org/geysermc/mcprotocollib/network/codec/BasePacketCodecHelper.java +++ b/protocol/src/main/java/org/geysermc/mcprotocollib/network/codec/BasePacketCodecHelper.java @@ -9,137 +9,58 @@ public class BasePacketCodecHelper implements PacketCodecHelper { @Override public void writeVarInt(ByteBuf buf, int value) { - this.writeVarLong(buf, value & 0xFFFFFFFFL); + while ((value & ~0x7F) != 0) { + buf.writeByte(value & 0x7F | 0x80); + value >>>= 7; + } + + buf.writeByte(value); } @Override public int readVarInt(ByteBuf buf) { int value = 0; int size = 0; - int b; - while (((b = buf.readByte()) & 0x80) == 0x80) { + + byte b; + do { + if (size >= 35) { + throw new RuntimeException("VarInt wider than 5 bytes"); + } + b = buf.readByte(); value |= (b & 0x7F) << size; size += 7; - if (size > 35) { - throw new IllegalArgumentException("VarInt wider than 35-bit"); - } - } + } while ((b & 0x80) == 0x80); - return value | ((b & 0x7F) << size); + return value; } - // Based off of Andrew Steinborn's blog post: - // https://steinborn.me/posts/performance/how-fast-can-you-write-a-varint/ @Override public void writeVarLong(ByteBuf buf, long value) { - // Peel the one and two byte count cases explicitly as they are the most common VarInt sizes - // that the server will write, to improve inlining. - if ((value & ~0x7FL) == 0) { - buf.writeByte((byte) value); - } else if ((value & ~0x3FFFL) == 0) { - int w = (int) ((value & 0x7FL | 0x80L) << 8 | - (value >>> 7)); - buf.writeShort(w); - } else { - writeVarLongFull(buf, value); + while ((value & ~0x7FL) != 0) { + buf.writeByte((int) (value & 0x7F | 0x80)); + value >>>= 7; } - } - private static void writeVarLongFull(ByteBuf buf, long value) { - if ((value & ~0x7FL) == 0) { - buf.writeByte((byte) value); - } else if ((value & ~0x3FFFL) == 0) { - int w = (int) ((value & 0x7FL | 0x80L) << 8 | - (value >>> 7)); - buf.writeShort(w); - } else if ((value & ~0x1FFFFFL) == 0) { - int w = (int) ((value & 0x7FL | 0x80L) << 16 | - ((value >>> 7) & 0x7FL | 0x80L) << 8 | - (value >>> 14)); - buf.writeMedium(w); - } else if ((value & ~0xFFFFFFFL) == 0) { - int w = (int) ((value & 0x7F | 0x80) << 24 | - (((value >>> 7) & 0x7F | 0x80) << 16) | - ((value >>> 14) & 0x7F | 0x80) << 8 | - (value >>> 21)); - buf.writeInt(w); - } else if ((value & ~0x7FFFFFFFFL) == 0) { - int w = (int) ((value & 0x7F | 0x80) << 24 | - ((value >>> 7) & 0x7F | 0x80) << 16 | - ((value >>> 14) & 0x7F | 0x80) << 8 | - ((value >>> 21) & 0x7F | 0x80)); - buf.writeInt(w); - buf.writeByte((int) (value >>> 28)); - } else if ((value & ~0x3FFFFFFFFFFL) == 0) { - int w = (int) ((value & 0x7F | 0x80) << 24 | - ((value >>> 7) & 0x7F | 0x80) << 16 | - ((value >>> 14) & 0x7F | 0x80) << 8 | - ((value >>> 21) & 0x7F | 0x80)); - int w2 = (int) (((value >>> 28) & 0x7FL | 0x80L) << 8 | - (value >>> 35)); - buf.writeInt(w); - buf.writeShort(w2); - } else if ((value & ~0x1FFFFFFFFFFFFL) == 0) { - int w = (int) ((value & 0x7F | 0x80) << 24 | - ((value >>> 7) & 0x7F | 0x80) << 16 | - ((value >>> 14) & 0x7F | 0x80) << 8 | - ((value >>> 21) & 0x7F | 0x80)); - int w2 = (int) ((((value >>> 28) & 0x7FL | 0x80L) << 16 | - ((value >>> 35) & 0x7FL | 0x80L) << 8) | - (value >>> 42)); - buf.writeInt(w); - buf.writeMedium(w2); - } else if ((value & ~0xFFFFFFFFFFFFFFL) == 0) { - long w = (value & 0x7F | 0x80) << 56 | - ((value >>> 7) & 0x7F | 0x80) << 48 | - ((value >>> 14) & 0x7F | 0x80) << 40 | - ((value >>> 21) & 0x7F | 0x80) << 32 | - ((value >>> 28) & 0x7FL | 0x80L) << 24 | - ((value >>> 35) & 0x7FL | 0x80L) << 16 | - ((value >>> 42) & 0x7FL | 0x80L) << 8 | - (value >>> 49); - buf.writeLong(w); - } else if ((value & ~0x7FFFFFFFFFFFFFFFL) == 0) { - long w = (value & 0x7F | 0x80) << 56 | - ((value >>> 7) & 0x7F | 0x80) << 48 | - ((value >>> 14) & 0x7F | 0x80) << 40 | - ((value >>> 21) & 0x7F | 0x80) << 32 | - ((value >>> 28) & 0x7FL | 0x80L) << 24 | - ((value >>> 35) & 0x7FL | 0x80L) << 16 | - ((value >>> 42) & 0x7FL | 0x80L) << 8 | - (value >>> 49); - buf.writeLong(w); - buf.writeByte((byte) (value >>> 56)); - } else { - long w = (value & 0x7F | 0x80) << 56 | - ((value >>> 7) & 0x7F | 0x80) << 48 | - ((value >>> 14) & 0x7F | 0x80) << 40 | - ((value >>> 21) & 0x7F | 0x80) << 32 | - ((value >>> 28) & 0x7FL | 0x80L) << 24 | - ((value >>> 35) & 0x7FL | 0x80L) << 16 | - ((value >>> 42) & 0x7FL | 0x80L) << 8 | - (value >>> 49); - int w2 = (int) (((value >>> 56) & 0x7FL | 0x80L) << 8 | - (value >>> 63)); - buf.writeLong(w); - buf.writeShort(w2); - } + buf.writeByte((int) value); } @Override public long readVarLong(ByteBuf buf) { long value = 0; int size = 0; - int b; - while (((b = buf.readByte()) & 0x80) == 0x80) { + + byte b; + do { + if (size >= 70) { + throw new RuntimeException("VarLong wider than 10 bytes"); + } + b = buf.readByte(); value |= (b & 0x7FL) << size; size += 7; - if (size > 70) { - throw new IllegalArgumentException("VarLong wider than 70-bit"); - } - } + } while ((b & 0x80) == 0x80); - return value | ((b & 0x7FL) << size); + return value; } @Override diff --git a/protocol/src/test/java/org/geysermc/mcprotocollib/protocol/codec/BasePacketCodecHelperTest.java b/protocol/src/test/java/org/geysermc/mcprotocollib/protocol/codec/BasePacketCodecHelperTest.java new file mode 100644 index 000000000..5c7413645 --- /dev/null +++ b/protocol/src/test/java/org/geysermc/mcprotocollib/protocol/codec/BasePacketCodecHelperTest.java @@ -0,0 +1,50 @@ +package org.geysermc.mcprotocollib.protocol.codec; + +import io.netty.buffer.Unpooled; +import org.geysermc.mcprotocollib.network.codec.BasePacketCodecHelper; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +public class BasePacketCodecHelperTest { + private static final BasePacketCodecHelper codecHelper = new BasePacketCodecHelper(); + + @Test + public void readVarInt() { + assertEquals(0x80808080, codecHelper.readVarInt(Unpooled.wrappedBuffer(new byte[]{ + (byte) 0x80, (byte) 0x81, (byte) 0x82, (byte) 0x84, (byte) 0x08 + }))); + } + + @Test + public void readVarIntTooLong() { + try { + // VarInt too long error should take precedence over IndexOutOfBoundsException + codecHelper.readVarInt(Unpooled.wrappedBuffer(new byte[]{ + (byte) 0x80, (byte) 0x80, (byte) 0x80, (byte) 0x80, (byte) 0x80 + })); + } catch (RuntimeException ex) { + // (assertThrow doesn't work as IndexOutOfBoundsException is also a RuntimeException) + assertEquals("VarInt wider than 5 bytes", ex.getMessage()); + } + } + + @Test + public void readVarLong() { + assertEquals(0x8080808080808080L, codecHelper.readVarLong(Unpooled.wrappedBuffer(new byte[]{ + (byte) 0x80, (byte) 0x81, (byte) 0x82, (byte) 0x84, (byte) 0x88, (byte) 0x90, (byte) 0xa0, (byte) 0xc0, (byte) 0x80, (byte) 0x01 + }))); + } + + @Test + public void readVarLongTooLong() { + try { + // VarLong too long error should take precedence over IndexOutOfBoundsException + codecHelper.readVarLong(Unpooled.wrappedBuffer(new byte[]{ + (byte) 0x80, (byte) 0x80, (byte) 0x80, (byte) 0x80, (byte) 0x80, (byte) 0x80, (byte) 0x80, (byte) 0x80, (byte) 0x80, (byte) 0x80 + })); + } catch (RuntimeException ex) { + assertEquals("VarLong wider than 10 bytes", ex.getMessage()); + } + } +}