Repository: netty/netty
Issue: Snappy - wrong chunk type for stream identifier

Netty version: 4.0.18.Final

Context:
I am using snappy to send a compressed TCP data stream of arbitrary data. The peer application has spit out a warning that the stream started with the wrong chunk type. (0x80 instead of 0xff)

In my case this is not such a big problem because 0x80 frames are within the skippable chunk range. But the warning points to a potential problem for other cases.

Please see https://code.google.com/p/snappy/source/browse/trunk/framing_format.txt#68 for more information.

If wanted could look into the Netty code and provide a patch. Just let me know.


Issue comments:
Sure a fix would be awesome. We love contributions :)

> Am 18.04.2014 um 20:30 schrieb mkrueger92 notifications@github.com:
> 
> Netty version: 4.0.18.Final
> 
> Context:
> I am using snappy to send a compressed TCP data stream of arbitrary data. The peer application has spit out a warning that the stream started with the wrong chunk type. (0x80 instead of 0xff)
> 
> In my case this is not such a big problem because 0x80 frames are within the skippable chunk range. But the warning points to a potential problem for other cases.
> 
> Please see https://code.google.com/p/snappy/source/browse/trunk/framing_format.txt#68 for more information.
> 
> If wanted could look into the Netty code and provide a patch. Just let me know.
> 
> —
> Reply to this email directly or view it on GitHub.

@normanmaurer: Please have a look at the commit. If this one meets your quality requirements I can provide an additional patch for the 4.0 and 4.1 branch as well if needed.

Looks correct :)

> Am 18.04.2014 um 22:19 schrieb mkrueger92 notifications@github.com:
> 
> @normanmaurer: Please have a look at the commit. If this one meets your quality requirements I can provide an additional patch for the 4.0 and 4.1 branch as well if needed.
> 
> —
> Reply to this email directly or view it on GitHub.

@mkrueger92 create a PR and I will merge in

Fixed via #2411 thanks!


Pull request review comments:

Patch that resolved the issue:
diff --git a/codec/src/main/java/io/netty/handler/codec/compression/SnappyFramedDecoder.java b/codec/src/main/java/io/netty/handler/codec/compression/SnappyFramedDecoder.java
index a2db4488c62..f3dee9a8a02 100644
--- a/codec/src/main/java/io/netty/handler/codec/compression/SnappyFramedDecoder.java
+++ b/codec/src/main/java/io/netty/handler/codec/compression/SnappyFramedDecoder.java
@@ -198,7 +198,7 @@ static ChunkType mapChunkType(byte type) {
             return ChunkType.COMPRESSED_DATA;
         } else if (type == 1) {
             return ChunkType.UNCOMPRESSED_DATA;
-        } else if (type == -0x80) {
+        } else if (type == (byte) 0xff) {
             return ChunkType.STREAM_IDENTIFIER;
         } else if ((type & 0x80) == 0x80) {
             return ChunkType.RESERVED_SKIPPABLE;
diff --git a/codec/src/main/java/io/netty/handler/codec/compression/SnappyFramedEncoder.java b/codec/src/main/java/io/netty/handler/codec/compression/SnappyFramedEncoder.java
index b22a026c2c3..1cc5301ab76 100644
--- a/codec/src/main/java/io/netty/handler/codec/compression/SnappyFramedEncoder.java
+++ b/codec/src/main/java/io/netty/handler/codec/compression/SnappyFramedEncoder.java
@@ -40,7 +40,7 @@ public class SnappyFramedEncoder extends MessageToByteEncoder<ByteBuf> {
      * type 0xff, a length field of 0x6, and 'sNaPpY' in ASCII.
      */
     private static final byte[] STREAM_START = {
-        -0x80, 0x06, 0x00, 0x00, 0x73, 0x4e, 0x61, 0x50, 0x70, 0x59
+        (byte) 0xff, 0x06, 0x00, 0x00, 0x73, 0x4e, 0x61, 0x50, 0x70, 0x59
     };
 
     private final Snappy snappy = new Snappy();


Test patch:
diff --git a/codec/src/test/java/io/netty/handler/codec/compression/SnappyFramedDecoderTest.java b/codec/src/test/java/io/netty/handler/codec/compression/SnappyFramedDecoderTest.java
index 994369b4a17..5c8355fbd5a 100644
--- a/codec/src/test/java/io/netty/handler/codec/compression/SnappyFramedDecoderTest.java
+++ b/codec/src/test/java/io/netty/handler/codec/compression/SnappyFramedDecoderTest.java
@@ -53,7 +53,7 @@ public void testInvalidStreamIdentifierLength() throws Exception {
     @Test(expected = DecompressionException.class)
     public void testInvalidStreamIdentifierValue() throws Exception {
         ByteBuf in = Unpooled.wrappedBuffer(new byte[] {
-            -0x80, 0x06, 0x00, 0x00, 's', 'n', 'e', 't', 't', 'y'
+            (byte) 0xff, 0x06, 0x00, 0x00, 's', 'n', 'e', 't', 't', 'y'
         });
 
         channel.writeInbound(in);
@@ -89,7 +89,7 @@ public void testCompressedDataBeforeStreamIdentifier() throws Exception {
     @Test
     public void testReservedSkippableSkipsInput() throws Exception {
         ByteBuf in = Unpooled.wrappedBuffer(new byte[] {
-           -0x80, 0x06, 0x00, 0x00, 0x73, 0x4e, 0x61, 0x50, 0x70, 0x59,
+           (byte) 0xff, 0x06, 0x00, 0x00, 0x73, 0x4e, 0x61, 0x50, 0x70, 0x59,
            -0x7f, 0x05, 0x00, 0x00, 'n', 'e', 't', 't', 'y'
         });
 
@@ -102,7 +102,7 @@ public void testReservedSkippableSkipsInput() throws Exception {
     @Test
     public void testUncompressedDataAppendsToOut() throws Exception {
         ByteBuf in = Unpooled.wrappedBuffer(new byte[] {
-           -0x80, 0x06, 0x00, 0x00, 0x73, 0x4e, 0x61, 0x50, 0x70, 0x59,
+           (byte) 0xff, 0x06, 0x00, 0x00, 0x73, 0x4e, 0x61, 0x50, 0x70, 0x59,
             0x01, 0x09, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 'n', 'e', 't', 't', 'y'
         });
 
@@ -115,7 +115,7 @@ public void testUncompressedDataAppendsToOut() throws Exception {
     @Test
     public void testCompressedDataDecodesAndAppendsToOut() throws Exception {
         ByteBuf in = Unpooled.wrappedBuffer(new byte[] {
-           -0x80, 0x06, 0x00, 0x00, 0x73, 0x4e, 0x61, 0x50, 0x70, 0x59,
+           (byte) 0xff, 0x06, 0x00, 0x00, 0x73, 0x4e, 0x61, 0x50, 0x70, 0x59,
             0x00, 0x0B, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
                   0x05, // preamble length
                   0x04 << 2, // literal tag + length
@@ -137,7 +137,7 @@ public void testInvalidChecksumThrowsException() throws Exception {
 
         // checksum here is presented as 0
         ByteBuf in = Unpooled.wrappedBuffer(new byte[] {
-           -0x80, 0x06, 0x00, 0x00, 0x73, 0x4e, 0x61, 0x50, 0x70, 0x59,
+           (byte) 0xff, 0x06, 0x00, 0x00, 0x73, 0x4e, 0x61, 0x50, 0x70, 0x59,
             0x01, 0x09, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 'n', 'e', 't', 't', 'y'
         });
 
@@ -150,7 +150,7 @@ public void testInvalidChecksumDoesNotThrowException() throws Exception {
 
         // checksum here is presented as a282986f (little endian)
         ByteBuf in = Unpooled.wrappedBuffer(new byte[] {
-           -0x80, 0x06, 0x00, 0x00, 0x73, 0x4e, 0x61, 0x50, 0x70, 0x59,
+           (byte) 0xff, 0x06, 0x00, 0x00, 0x73, 0x4e, 0x61, 0x50, 0x70, 0x59,
             0x01, 0x09, 0x00, 0x00, 0x6f, -0x68, -0x7e, -0x5e, 'n', 'e', 't', 't', 'y'
         });
 
diff --git a/codec/src/test/java/io/netty/handler/codec/compression/SnappyFramedEncoderTest.java b/codec/src/test/java/io/netty/handler/codec/compression/SnappyFramedEncoderTest.java
index 25e2655a272..9b1d606e143 100644
--- a/codec/src/test/java/io/netty/handler/codec/compression/SnappyFramedEncoderTest.java
+++ b/codec/src/test/java/io/netty/handler/codec/compression/SnappyFramedEncoderTest.java
@@ -43,7 +43,7 @@ public void testSmallAmountOfDataIsUncompressed() throws Exception {
         assertTrue(channel.finish());
 
         ByteBuf expected = Unpooled.wrappedBuffer(new byte[] {
-            -0x80, 0x06, 0x00, 0x00, 0x73, 0x4e, 0x61, 0x50, 0x70, 0x59,
+            (byte) 0xff, 0x06, 0x00, 0x00, 0x73, 0x4e, 0x61, 0x50, 0x70, 0x59,
              0x01, 0x09, 0x00, 0x00, 0x6f, -0x68, -0x7e, -0x5e, 'n', 'e', 't', 't', 'y'
         });
 
@@ -61,7 +61,7 @@ public void testLargeAmountOfDataIsCompressed() throws Exception {
         assertTrue(channel.finish());
 
         ByteBuf expected = Unpooled.wrappedBuffer(new byte[] {
-            -0x80, 0x06, 0x00, 0x00, 0x73, 0x4e, 0x61, 0x50, 0x70, 0x59,
+            (byte) 0xff, 0x06, 0x00, 0x00, 0x73, 0x4e, 0x61, 0x50, 0x70, 0x59,
              0x00, 0x0E, 0x00, 0x00, 0x3b, 0x36, -0x7f, 0x37,
                    0x14, 0x10,
                    'n', 'e', 't', 't', 'y',
@@ -83,7 +83,7 @@ public void testStreamStartIsOnlyWrittenOnce() throws Exception {
         assertTrue(channel.finish());
 
         ByteBuf expected = Unpooled.wrappedBuffer(new byte[] {
-            -0x80, 0x06, 0x00, 0x00, 0x73, 0x4e, 0x61, 0x50, 0x70, 0x59,
+            (byte) 0xff, 0x06, 0x00, 0x00, 0x73, 0x4e, 0x61, 0x50, 0x70, 0x59,
              0x01, 0x09, 0x00, 0x00, 0x6f, -0x68, -0x7e, -0x5e, 'n', 'e', 't', 't', 'y',
              0x01, 0x09, 0x00, 0x00, 0x6f, -0x68, -0x7e, -0x5e, 'n', 'e', 't', 't', 'y',
         });