Bound compressed packet decoding (#58)

.github/workflows/build-and-test.yml

@@ -3,9 +3,9 @@ name: Build and test
 
 on:
   pull_request:
-    branches: [ master, main ]
+    branches: [ master, main, release/** ]
   push:
-    branches: [ master, main ]
+    branches: [ master, main, release/** ]
 
 jobs:
   build-and-test:

src/main/java/codechicken/core/ReflectionManager.java

@@ -89,6 +89,22 @@ public class ReflectionManager {
         }
     }
 
+    /**
+     * Set a field of the object {@code instance} of type {@code class1} to {@code value}.
+     *
+     * @deprecated According to {@link Class#getDeclaredFields()}, the array of fields has no defined order, therefore
+     *             selecting a field based on its index will result in JVM-specific behaviour. Use
+     *             {@link #setField(Class, Object, String, Object)} instead.
+     *
+     * @param class1     The class which defines the field in question.
+     * @param instance   The object being altered - must be an instance of {@code class1}
+     * @param fieldindex The index of the field being set.
+     * @param value      The value to set the field to.
+     * @throws IllegalArgumentException If {@code instance} is not an instance of {@code class1}, or if {@code value}
+     *                                  doesn't match the type of the field.
+     * @throws IllegalAccessException   If unable to write to the field.
+     */
+    @Deprecated
     public static void setField(Class<?> class1, Object instance, int fieldindex, Object value)
             throws IllegalArgumentException, IllegalAccessException {
         Field field = class1.getDeclaredFields()[fieldindex];
@@ -179,6 +195,21 @@ public class ReflectionManager {
         return null;
     }
 
+    /**
+     * Get a field of the object {@code instance} of type {@code class1}.
+     *
+     * @deprecated According to {@link Class#getDeclaredFields()}, the array of fields has no defined order, therefore
+     *             selecting a field based on its index will result in JVM-specific behaviour. Use
+     *             {@link #getField(Class, Class, Object, String)} instead.
+     *
+     * @param class1     The class which defines the field in question.
+     * @param fieldType  The type of the field being read.
+     * @param instance   The object being read from - must be an instance of {@code class1}
+     * @param fieldIndex The index of the field being read.
+     * @throws IllegalArgumentException If {@code instance} is not an instance of {@code class1}.
+     * @throws IllegalAccessException   If unable to read from the field.
+     */
+    @Deprecated
     public static <T> T getField(Class<?> class1, Class<T> fieldType, Object instance, int fieldIndex)
             throws IllegalArgumentException, IllegalAccessException {
         Field field = class1.getDeclaredFields()[fieldIndex];

src/main/java/codechicken/lib/packet/PacketCustom.java

@@ -2,6 +2,7 @@ package codechicken.lib.packet;
 
 import java.util.EnumMap;
 import java.util.List;
+import java.util.zip.DataFormatException;
 import java.util.zip.Deflater;
 import java.util.zip.Inflater;
 
@@ -50,6 +51,8 @@ import io.netty.util.AttributeKey;
 
 public final class PacketCustom implements MCDataInput, MCDataOutput {
 
+    private static final int MAX_DECOMPRESSED_SIZE = 2 * 1024 * 1024;
+
     public static interface ICustomPacketHandler {
     }
 
@@ -190,7 +193,7 @@ public final class PacketCustom implements MCDataInput, MCDataOutput {
         byteBuf = payload;
 
         type = byteBuf.readUnsignedByte();
-        if (type > 0x80) decompress();
+        if (type > 0x80) decompressPayload();
         type &= 0x7F;
     }
 
@@ -204,44 +207,61 @@ public final class PacketCustom implements MCDataInput, MCDataOutput {
         byteBuf.writeByte(type);
     }
 
-    /**
-     * Decompresses the remaining ByteBuf (after type has been read) using Snappy
-     */
-    private void decompress() {
+    private void decompressPayload() {
+        if (byteBuf.readableBytes() < 4) throw new IllegalArgumentException("Missing compressed packet length");
+        int declaredLength = byteBuf.readInt();
+        if (declaredLength < 0 || declaredLength > MAX_DECOMPRESSED_SIZE)
+            throw new IllegalArgumentException("Invalid decompressed packet length: " + declaredLength);
+
         Inflater inflater = new Inflater();
+        ByteBuf decodedPayload = null;
         try {
-            int len = byteBuf.readInt();
-            ByteBuf out = Unpooled.buffer(len);
-            inflater.setInput(byteBuf.array(), byteBuf.readerIndex(), byteBuf.readableBytes());
-            inflater.inflate(out.array());
-            out.writerIndex(len);
-            byteBuf = out;
-        } catch (Exception e) {
-            throw new RuntimeException(e);
+            byte[] compressedInput = new byte[byteBuf.readableBytes()];
+            byteBuf.readBytes(compressedInput);
+            inflater.setInput(compressedInput);
+            decodedPayload = Unpooled.buffer(Math.min(declaredLength, 8192), declaredLength);
+            byte[] inflatedChunk = new byte[Math.min(declaredLength + 1, 8192)];
+            while (!inflater.finished()) {
+                int inflatedBytes = inflater.inflate(inflatedChunk);
+                if (inflatedBytes > declaredLength - decodedPayload.writerIndex())
+                    throw new IllegalArgumentException("Decompressed packet exceeds declared length");
+                if (inflatedBytes == 0 && !inflater.finished())
+                    throw new IllegalArgumentException("Incomplete or dictionary-dependent compressed packet");
+                decodedPayload.writeBytes(inflatedChunk, 0, inflatedBytes);
+            }
+            if (decodedPayload.writerIndex() != declaredLength || inflater.getRemaining() != 0)
+                throw new IllegalArgumentException(
+                        "Compressed packet does not match declared length or has trailing data");
+            byteBuf = decodedPayload;
+            decodedPayload = null;
+        } catch (DataFormatException e) {
+            throw new IllegalArgumentException("Invalid compressed packet", e);
         } finally {
+            if (decodedPayload != null) decodedPayload.release();
             inflater.end();
         }
     }
 
-    /**
-     * Compresses the payload ByteBuf after the type byte
-     */
-    private void do_compress() {
+    private void compressPayload() {
+        if ((byteBuf.getUnsignedByte(0) & 0x80) != 0) return;
+
+        int payloadLength = byteBuf.writerIndex() - 1;
+        if (payloadLength > MAX_DECOMPRESSED_SIZE)
+            throw new IllegalArgumentException("Packet payload exceeds 2 MiB decompressed limit: " + payloadLength);
+
         Deflater deflater = new Deflater();
         try {
             byteBuf.readerIndex(1);
-            int len = byteBuf.readableBytes();
-            deflater.setInput(byteBuf.array(), byteBuf.readerIndex(), len);
+            deflater.setInput(byteBuf.array(), byteBuf.readerIndex(), payloadLength);
             deflater.finish();
-            ByteBuf out = Unpooled.buffer(len + 5);
-            int clen = deflater.deflate(out.array(), 5, len);
-            if (clen >= len - 5 || !deflater.finished()) // not worth compressing, gets larger
-                return;
+            ByteBuf compressedPayload = Unpooled.buffer(payloadLength + 5);
+            int compressedLength = deflater.deflate(compressedPayload.array(), 5, payloadLength);
+            if (compressedLength >= payloadLength - 5 || !deflater.finished()) return;
 
-            out.setByte(0, type | 0x80);
-            out.setInt(1, len);
-            out.writerIndex(clen + 5);
-            byteBuf = out;
+            compressedPayload.setByte(0, type | 0x80);
+            compressedPayload.setInt(1, payloadLength);
+            compressedPayload.writerIndex(compressedLength + 5);
+            byteBuf = compressedPayload;
         } catch (Exception e) {
             throw new RuntimeException(e);
         } finally {
@@ -474,7 +494,7 @@ public final class PacketCustom implements MCDataInput, MCDataOutput {
     public FMLProxyPacket toPacket() {
         if (incoming()) throw new IllegalStateException("Tried to write an incoming packet");
 
-        if (byteBuf.readableBytes() > 32000 || (type & 0x80) != 0) do_compress();
+        if (byteBuf.readableBytes() > 32000 || (type & 0x80) != 0) compressPayload();
 
         // FML packet impl returns the whole of the backing array, copy used portion of array to another ByteBuf
         return new FMLProxyPacket(byteBuf.copy(), channel);

src/main/resources/assets/codechickencore/lang/uk_UA.lang

@@ -0,0 +1 @@
+codechickencore.update=Доступна версія %s для %s

src/test/java/codechicken/lib/packet/PacketCustomTest.java

@@ -0,0 +1,218 @@
+package codechicken.lib.packet;
+
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+import java.io.ByteArrayOutputStream;
+import java.util.Arrays;
+import java.util.zip.Deflater;
+
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+
+import cpw.mods.fml.common.network.internal.FMLProxyPacket;
+import io.netty.buffer.ByteBuf;
+import io.netty.buffer.Unpooled;
+
+public class PacketCustomTest {
+
+    private static final int MAX_DECOMPRESSED_SIZE = 2 * 1024 * 1024;
+
+    @Test
+    public void rejectsLargeDeclarationWithoutCompressedData() {
+        assertRejected(Unpooled.buffer(5).writeByte(0x81).writeInt(8 * 1024 * 1024));
+    }
+
+    @ParameterizedTest
+    @ValueSource(ints = { -1, Integer.MIN_VALUE, MAX_DECOMPRESSED_SIZE + 1, Integer.MAX_VALUE })
+    public void rejectsInvalidDeclaredLengths(int length) {
+        assertRejected(Unpooled.buffer(5).writeByte(0x81).writeInt(length));
+    }
+
+    @Test
+    public void rejectsMissingCompressedDataWithinLimit() {
+        assertRejected(Unpooled.buffer(5).writeByte(0x81).writeInt(MAX_DECOMPRESSED_SIZE));
+    }
+
+    @Test
+    public void rejectsCompleteStreamAboveDecodeLimit() {
+        assertRejected(compressed(new byte[MAX_DECOMPRESSED_SIZE + 1]));
+    }
+
+    @Test
+    public void rejectsTruncatedLengthHeader() {
+        assertRejected(Unpooled.buffer(4).writeByte(0x81).writeMedium(10));
+    }
+
+    @Test
+    public void rejectsMismatchedInflatedLengths() {
+        byte[] data = new byte[16384];
+        assertRejected(compressed(data).setInt(1, data.length - 1));
+        assertRejected(compressed(data).setInt(1, data.length + 1));
+        assertRejected(compressed(data).setInt(1, 0));
+    }
+
+    @Test
+    public void rejectsTruncatedStreamEvenWhenAllOutputWasProduced() {
+        ByteBuf payload = compressed(new byte[16384]);
+        payload.writerIndex(payload.writerIndex() - 1);
+        assertRejected(payload);
+    }
+
+    @Test
+    public void rejectsCorruptStream() {
+        ByteBuf payload = compressed(new byte[16384]);
+        payload.setByte(payload.writerIndex() - 1, payload.getByte(payload.writerIndex() - 1) ^ 1);
+        assertRejected(payload);
+    }
+
+    @Test
+    public void rejectsDictionaryDependentStream() {
+        assertRejected(compressed(new byte[16384], new byte[] { 1, 2, 3 }));
+    }
+
+    @Test
+    public void rejectsTrailingCompressedData() {
+        assertRejected(compressed(new byte[16384]).writeByte(0));
+    }
+
+    @ParameterizedTest
+    @ValueSource(ints = { 0, 1, 8192, 8193, MAX_DECOMPRESSED_SIZE })
+    public void decodesCompleteStreams(int length) {
+        byte[] data = new byte[length];
+        Arrays.fill(data, (byte) 42);
+        assertDecoded(compressed(data), data);
+    }
+
+    @Test
+    public void decodesSlicedAndDirectBuffers() {
+        byte[] data = new byte[16384];
+        Arrays.fill(data, (byte) 42);
+        ByteBuf payload = compressed(data);
+        ByteBuf prefixed = Unpooled.buffer().writeLong(-1).writeBytes(payload, 0, payload.readableBytes());
+        ByteBuf direct = Unpooled.directBuffer().writeBytes(payload);
+        payload.release();
+        try {
+            assertDecoded(prefixed.slice(8, prefixed.readableBytes() - 8).retain(), data);
+            assertDecoded(direct, data);
+        } finally {
+            prefixed.release();
+        }
+    }
+
+    @Test
+    public void preservesUncompressedPackets() {
+        ByteBuf payload = Unpooled.buffer().writeByte(7).writeInt(42);
+        try {
+            PacketCustom packet = new PacketCustom(payload);
+            assertEquals(7, packet.getType());
+            assertEquals(42, packet.readInt());
+            assertEquals(0, packet.getByteBuf().readableBytes());
+        } finally {
+            payload.release();
+        }
+    }
+
+    @Test
+    public void roundTripsOutgoingCompression() {
+        for (int length : new int[] { 16, 64, 40000, MAX_DECOMPRESSED_SIZE }) {
+            byte[] data = new byte[length];
+            Arrays.fill(data, (byte) 42);
+            PacketCustom outgoing = new PacketCustom("PacketTest", 7);
+            FMLProxyPacket wire = null;
+            try {
+                outgoing.writeByteArray(data);
+                if (length <= 64) outgoing.compress();
+                wire = outgoing.toPacket();
+                assertEquals(length == 16 ? 7 : 0x87, wire.payload().getUnsignedByte(0));
+                assertDecoded(wire.payload().retain(), data);
+            } finally {
+                if (wire != null) wire.payload().release();
+                outgoing.getByteBuf().release();
+            }
+        }
+    }
+
+    @ParameterizedTest
+    @ValueSource(booleans = { false, true })
+    public void repeatedSerializationPreservesCompressedPayload(boolean forceCompression) {
+        byte[] data = new byte[40000];
+        Arrays.fill(data, (byte) 42);
+        PacketCustom outgoing = new PacketCustom("PacketTest", 7).writeByteArray(data);
+        if (forceCompression) outgoing.compress();
+        try {
+            for (int serialization = 0; serialization < 3; serialization++) {
+                FMLProxyPacket wire = outgoing.toPacket();
+                try {
+                    assertEquals(0x87, wire.payload().getUnsignedByte(0));
+                    assertDecoded(wire.payload().retain(), data);
+                } finally {
+                    wire.payload().release();
+                }
+            }
+        } finally {
+            outgoing.getByteBuf().release();
+        }
+    }
+
+    @Test
+    public void rejectsOutgoingPayloadAboveDecodeLimit() {
+        PacketCustom outgoing = new PacketCustom("PacketTest", 7);
+        try {
+            outgoing.getByteBuf().writeZero(MAX_DECOMPRESSED_SIZE + 1);
+            assertThrows(IllegalArgumentException.class, outgoing::toPacket);
+        } finally {
+            outgoing.getByteBuf().release();
+        }
+    }
+
+    private static ByteBuf compressed(byte[] data) {
+        return compressed(data, null);
+    }
+
+    private static ByteBuf compressed(byte[] data, byte[] dictionary) {
+        Deflater deflater = new Deflater();
+        try {
+            if (dictionary != null) deflater.setDictionary(dictionary);
+            deflater.setInput(data);
+            deflater.finish();
+            ByteArrayOutputStream compressed = new ByteArrayOutputStream();
+            byte[] scratch = new byte[8192];
+            while (!deflater.finished()) {
+                int count = deflater.deflate(scratch);
+                compressed.write(scratch, 0, count);
+            }
+            return Unpooled.buffer().writeByte(0x87).writeInt(data.length).writeBytes(compressed.toByteArray());
+        } finally {
+            deflater.end();
+        }
+    }
+
+    private static void assertDecoded(ByteBuf payload, byte[] expected) {
+        PacketCustom packet = null;
+        try {
+            packet = new PacketCustom(payload);
+            assertEquals(7, packet.getType());
+            assertEquals(expected.length, packet.getByteBuf().readableBytes());
+            byte[] actual = new byte[expected.length];
+            packet.getByteBuf().readBytes(actual);
+            assertArrayEquals(expected, actual);
+        } finally {
+            if (packet != null && packet.getByteBuf() != payload) packet.getByteBuf().release();
+            payload.release();
+        }
+    }
+
+    private static void assertRejected(ByteBuf payload) {
+        try {
+            assertThrows(IllegalArgumentException.class, () -> {
+                PacketCustom packet = new PacketCustom(payload);
+                if (packet.getByteBuf() != payload) packet.getByteBuf().release();
+            });
+        } finally {
+            payload.release();
+        }
+    }
+}