andreacavalli/netty5
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Fix SnappyFramedDecoder issues

Browse Source 
- Checksum header was being incorrectly read due to incorrect order of
  shift and masking operations.
- Length field of 1-byte copy was being incorrectly interpreted due to a
  typo in the binary mask used to extract it.
- Use ByteBuf.readUnsignedByte() instead of readByte() & 0xff
- Use bitwise-OR wherever possible
- Use EmbeddedByteChannel to test
- Use ByteBuf comparison instead of array comparison
- Work done by @lw346 and then revised by @trustin
This commit is contained in:
Luke Wood 2013-01-31 10:42:05 +00:00 committed by Trustin Lee
parent 2ec932798f
commit bfe44180f9
8 changed files with 144 additions and 112 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

38
codec/src/main/java/io/netty/handler/codec/compression/Snappy.java
View File

@ -263,7 +263,7 @@ public class Snappy {
inputLength = readPreamble(in);
}
if (inputLength == 0 || in.readerIndex() - inIndex + in.readableBytes() < maxLength) {
if (inputLength == 0 || in.readerIndex() - inIndex + in.readableBytes() < maxLength - 5) {
// Wait until we've got the entire chunk before continuing
return;
}
@ -330,22 +330,22 @@ public class Snappy {
int length;
switch(tag >> 2 & 0x3F) {
case 60:
length = in.readByte() & 0x0ff;
length = in.readUnsignedByte();
break;
case 61:
length = (in.readByte() & 0x0ff)
+ ((in.readByte() & 0x0ff) << 8);
length = in.readUnsignedByte()
| (in.readUnsignedByte() << 8);
break;
case 62:
length = (in.readByte() & 0x0ff)
+ ((in.readByte() & 0x0ff) << 8)
+ ((in.readByte() & 0x0ff) << 16);
length = in.readUnsignedByte()
| (in.readUnsignedByte() << 8)
| (in.readUnsignedByte() << 16);
break;
case 64:
length = (in.readByte() & 0x0ff)
+ ((in.readByte() & 0x0ff) << 8)
+ ((in.readByte() & 0x0ff) << 16)
+ ((in.readByte() & 0x0ff) << 24);
length = in.readUnsignedByte()
| (in.readUnsignedByte() << 8)
| (in.readUnsignedByte() << 16)
| (in.readUnsignedByte() << 24);
break;
default:
length = tag >> 2 & 0x3F;
@ -368,9 +368,8 @@ public class Snappy {
*/
private static void decodeCopyWith1ByteOffset(byte tag, ByteBuf in, ByteBuf out) {
int initialIndex = in.readerIndex();
int length = 4 + ((tag & 0x0c) >> 2);
int offset = 1 + ((tag & 0x0e0) << 8)
+ (in.readByte() & 0x0ff);
int length = 4 + ((tag & 0x01c) >> 2);
int offset = 1 + ((tag & 0x0e0) << 8 | in.readUnsignedByte());
validateOffset(offset, initialIndex);
@ -394,8 +393,7 @@ public class Snappy {
private static void decodeCopyWith2ByteOffset(byte tag, ByteBuf in, ByteBuf out) {
int initialIndex = in.readerIndex();
int length = 1 + (tag >> 2 & 0x03f);
int offset = 1 + (in.readByte() & 0x0ff)
+ (in.readByte() & 0x0ff) << 8;
int offset = 1 + (in.readUnsignedByte() | in.readUnsignedByte() << 8);
validateOffset(offset, initialIndex);
@ -419,10 +417,10 @@ public class Snappy {
private static void decodeCopyWith4ByteOffset(byte tag, ByteBuf in, ByteBuf out) {
int initialIndex = in.readerIndex();
int length = 1 + (tag >> 2 & 0x03F);
int offset = 1 + (in.readByte() & 0x0ff)
+ ((in.readByte() & 0x0ff) << 8)
+ ((in.readByte() & 0x0ff) << 16)
+ ((in.readByte() & 0x0ff) << 24);
int offset = 1 + (in.readUnsignedByte() |
in.readUnsignedByte() << 8 |
in.readUnsignedByte() << 16 |
in.readUnsignedByte() << 24);
validateOffset(offset, initialIndex);

4
codec/src/main/java/io/netty/handler/codec/compression/SnappyChecksumUtil.java
View File

@ -44,15 +44,11 @@ public final class SnappyChecksumUtil {
public static int calculateChecksum(ByteBuf slice) {
CRC32 crc32 = new CRC32();
try {
if (slice.hasArray()) {
crc32.update(slice.array());
} else {
byte[] array = new byte[slice.readableBytes()];
slice.markReaderIndex();
slice.readBytes(array);
slice.resetReaderIndex();
crc32.update(array);
}
return maskChecksum((int) crc32.getValue());
} finally {

19
codec/src/main/java/io/netty/handler/codec/compression/SnappyFramedDecoder.java
View File

@ -84,8 +84,7 @@ public class SnappyFramedDecoder extends ByteToByteDecoder {
byte type = in.readByte();
chunkType = mapChunkType(type);
chunkLength = (in.readByte() & 0x0ff)
+ ((in.readByte() & 0x0ff) << 8);
chunkLength = in.readUnsignedByte() | in.readUnsignedByte() << 8;
// The spec mandates that reserved unskippable chunks must immediately
// return an error, as we must assume that we cannot decode the stream
@ -130,10 +129,10 @@ public class SnappyFramedDecoder extends ByteToByteDecoder {
if (!started) {
throw new CompressionException("Received UNCOMPRESSED_DATA tag before STREAM_IDENTIFIER");
}
checksum = in.readByte() & 0x0ff
+ (in.readByte() << 8 & 0x0ff)
+ (in.readByte() << 16 & 0x0ff)
+ (in.readByte() << 24 & 0x0ff);
checksum = in.readUnsignedByte()
| (in.readUnsignedByte() << 8)
| (in.readUnsignedByte() << 16)
| (in.readUnsignedByte() << 24);
if (validateChecksums) {
ByteBuf data = in.readBytes(chunkLength);
validateChecksum(data, checksum);
@ -146,10 +145,10 @@ public class SnappyFramedDecoder extends ByteToByteDecoder {
if (!started) {
throw new CompressionException("Received COMPRESSED_DATA tag before STREAM_IDENTIFIER");
}
checksum = in.readByte() & 0x0ff
+ (in.readByte() << 8 & 0x0ff)
+ (in.readByte() << 16 & 0x0ff)
+ (in.readByte() << 24 & 0x0ff);
checksum = in.readUnsignedByte()
| (in.readUnsignedByte() << 8)
| (in.readUnsignedByte() << 16)
| (in.readUnsignedByte() << 24);
if (validateChecksums) {
ByteBuf uncompressed = ctx.alloc().buffer();
snappy.decode(in, uncompressed, chunkLength);

15
codec/src/main/java/io/netty/handler/codec/compression/SnappyFramedEncoder.java
View File

@ -15,12 +15,12 @@
*/
package io.netty.handler.codec.compression;
import static io.netty.handler.codec.compression.SnappyChecksumUtil.calculateChecksum;
import io.netty.buffer.ByteBuf;
import io.netty.channel.ChannelHandlerContext;
import io.netty.handler.codec.ByteToByteEncoder;
import static io.netty.handler.codec.compression.SnappyChecksumUtil.*;
/**
* Compresses a {@link ByteBuf} using the Snappy framing format.
*
@ -64,20 +64,19 @@ public class SnappyFramedEncoder extends ByteToByteEncoder {
int subChunkLength = Math.min(Short.MAX_VALUE, chunkLength);
out.writeByte(0);
writeChunkLength(out, subChunkLength);
ByteBuf slice = in.slice();
ByteBuf slice = in.slice(in.readerIndex(), subChunkLength);
calculateAndWriteChecksum(slice, out);
snappy.encode(slice, out, subChunkLength);
in.readerIndex(slice.readerIndex());
}
} else {
out.writeByte(1);
writeChunkLength(out, chunkLength);
ByteBuf slice = in.slice();
calculateAndWriteChecksum(slice, out);
out.writeBytes(slice);
calculateAndWriteChecksum(in, out);
out.writeBytes(in, chunkLength);
}
in.readerIndex(in.readerIndex() + chunkLength);
}
/**

40
codec/src/test/java/io/netty/handler/codec/compression/SnappyFramedDecoderTest.java
View File

@ -17,12 +17,13 @@ package io.netty.handler.codec.compression;
import io.netty.buffer.ByteBuf;
import io.netty.buffer.Unpooled;
import io.netty.channel.embedded.EmbeddedByteChannel;
import org.junit.Test;
import static org.junit.Assert.*;
public class SnappyFramedDecoderTest {
private final SnappyFramedDecoder decoder = new SnappyFramedDecoder();
private final EmbeddedByteChannel channel = new EmbeddedByteChannel(new SnappyFramedDecoder());
@Test(expected = CompressionException.class)
public void testReservedUnskippableChunkTypeCausesError() throws Exception {
@ -30,7 +31,7 @@ public class SnappyFramedDecoderTest {
0x03, 0x01, 0x00, 0x00
});
decoder.decode(null, in, null);
channel.writeInbound(in);
}
@Test(expected = CompressionException.class)
@ -39,7 +40,7 @@ public class SnappyFramedDecoderTest {
-0x80, 0x05, 0x00, 'n', 'e', 't', 't', 'y'
});
decoder.decode(null, in, null);
channel.writeInbound(in);
}
@Test(expected = CompressionException.class)
@ -48,7 +49,7 @@ public class SnappyFramedDecoderTest {
-0x80, 0x06, 0x00, 's', 'n', 'e', 't', 't', 'y'
});
decoder.decode(null, in, null);
channel.writeInbound(in);
}
@Test(expected = CompressionException.class)
@ -57,7 +58,7 @@ public class SnappyFramedDecoderTest {
-0x7f, 0x06, 0x00, 's', 'n', 'e', 't', 't', 'y'
});
decoder.decode(null, in, null);
channel.writeInbound(in);
}
@Test(expected = CompressionException.class)
@ -66,7 +67,7 @@ public class SnappyFramedDecoderTest {
0x01, 0x05, 0x00, 'n', 'e', 't', 't', 'y'
});
decoder.decode(null, in, null);
channel.writeInbound(in);
}
@Test(expected = CompressionException.class)
@ -75,7 +76,7 @@ public class SnappyFramedDecoderTest {
0x00, 0x05, 0x00, 'n', 'e', 't', 't', 'y'
});
decoder.decode(null, in, null);
channel.writeInbound(in);
}
@Test
@ -85,9 +86,8 @@ public class SnappyFramedDecoderTest {
-0x7f, 0x05, 0x00, 'n', 'e', 't', 't', 'y'
});
ByteBuf out = Unpooled.unmodifiableBuffer(Unpooled.EMPTY_BUFFER);
decoder.decode(null, in, out);
channel.writeInbound(in);
assertNull(channel.readInbound());
assertEquals(17, in.readerIndex());
}
@ -99,14 +99,10 @@ public class SnappyFramedDecoderTest {
0x01, 0x05, 0x00, 0x00, 0x00, 0x00, 0x00, 'n', 'e', 't', 't', 'y'
});
ByteBuf out = Unpooled.buffer(5);
channel.writeInbound(in);
decoder.decode(null, in, out);
byte[] expected = {
'n', 'e', 't', 't', 'y'
};
assertArrayEquals(expected, out.array());
ByteBuf expected = Unpooled.wrappedBuffer(new byte[] { 'n', 'e', 't', 't', 'y' });
assertEquals(expected, channel.readInbound());
}
@Test
@ -119,13 +115,9 @@ public class SnappyFramedDecoderTest {
0x6e, 0x65, 0x74, 0x74, 0x79 // "netty"
});
ByteBuf out = Unpooled.buffer(5);
channel.writeInbound(in);
decoder.decode(null, in, out);
byte[] expected = {
'n', 'e', 't', 't', 'y'
};
assertArrayEquals(expected, out.array());
ByteBuf expected = Unpooled.wrappedBuffer(new byte[] { 'n', 'e', 't', 't', 'y' });
assertEquals(expected, channel.readInbound());
}
}

49
codec/src/test/java/io/netty/handler/codec/compression/SnappyFramedEncoderTest.java
View File

@ -17,12 +17,19 @@ package io.netty.handler.codec.compression;
import io.netty.buffer.ByteBuf;
import io.netty.buffer.Unpooled;
import io.netty.channel.embedded.EmbeddedByteChannel;
import org.junit.Before;
import org.junit.Test;
import static org.junit.Assert.*;
public class SnappyFramedEncoderTest {
private final SnappyFramedEncoder encoder = new SnappyFramedEncoder();
private EmbeddedByteChannel channel;
@Before
public void setUp() {
channel = new EmbeddedByteChannel(new SnappyFramedEncoder());
}
@Test
public void testSmallAmountOfDataIsUncompressed() throws Exception {
@ -30,15 +37,14 @@ public class SnappyFramedEncoderTest {
'n', 'e', 't', 't', 'y'
});
ByteBuf out = Unpooled.buffer(21);
channel.writeOutbound(in);
assertTrue(channel.finish());
encoder.encode(null, in, out);
byte[] expected = {
ByteBuf expected = Unpooled.wrappedBuffer(new byte[] {
-0x80, 0x06, 0x00, 0x73, 0x4e, 0x61, 0x50, 0x70, 0x59,
0x01, 0x05, 0x00, 0x2d, -0x5a, -0x7e, -0x5e, 'n', 'e', 't', 't', 'y'
};
assertArrayEquals(expected, out.array());
});
assertEquals(expected, channel.readOutbound());
}
@Test
@ -48,18 +54,17 @@ public class SnappyFramedEncoderTest {
'n', 'e', 't', 't', 'y', 'n', 'e', 't', 't', 'y'
});
ByteBuf out = Unpooled.buffer(26);
channel.writeOutbound(in);
assertTrue(channel.finish());
encoder.encode(null, in, out);
byte[] expected = {
ByteBuf expected = Unpooled.wrappedBuffer(new byte[] {
-0x80, 0x06, 0x00, 0x73, 0x4e, 0x61, 0x50, 0x70, 0x59,
0x00, 0x14, 0x00, 0x7b, 0x1f, 0x65, 0x64,
0x14, 0x10,
'n', 'e', 't', 't', 'y',
0x3a, 0x05, 0x00
};
assertArrayEquals(expected, out.array());
});
assertEquals(expected, channel.readOutbound());
}
@Test
@ -68,18 +73,17 @@ public class SnappyFramedEncoderTest {
'n', 'e', 't', 't', 'y'
});
ByteBuf out = Unpooled.buffer(33);
encoder.encode(null, in, out);
channel.writeOutbound(in);
in.readerIndex(0); // rewind the buffer to write the same data
encoder.encode(null, in, out);
channel.writeOutbound(in);
assertTrue(channel.finish());
byte[] expected = {
ByteBuf expected = Unpooled.wrappedBuffer(new byte[] {
-0x80, 0x06, 0x00, 0x73, 0x4e, 0x61, 0x50, 0x70, 0x59,
0x01, 0x05, 0x00, 0x2d, -0x5a, -0x7e, -0x5e, 'n', 'e', 't', 't', 'y',
0x01, 0x05, 0x00, 0x2d, -0x5a, -0x7e, -0x5e, 'n', 'e', 't', 't', 'y',
};
assertArrayEquals(expected, out.array());
});
assertEquals(expected, channel.readOutbound());
}
/**
@ -124,8 +128,7 @@ public class SnappyFramedEncoderTest {
-1, 1 // remainder
});
ByteBuf out = Unpooled.buffer(274);
encoder.encode(null, in, out);
channel.writeOutbound(in);
assertTrue(channel.finish());
}
}

44
codec/src/test/java/io/netty/handler/codec/compression/SnappyIntegrationTest.java Normal file
View File

@ -0,0 +1,44 @@
/*
* Copyright 2013 The Netty Project
*
* The Netty Project licenses this file to you under the Apache License,
* version 2.0 (the "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at:
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations
* under the License.
*/
package io.netty.handler.codec.compression;
import io.netty.buffer.ByteBuf;
import io.netty.buffer.Unpooled;
import io.netty.channel.embedded.EmbeddedByteChannel;
import io.netty.util.CharsetUtil;
import org.junit.Assert;
import org.junit.Test;
public class SnappyIntegrationTest {
private final EmbeddedByteChannel channel = new EmbeddedByteChannel(new SnappyFramedEncoder(),
new SnappyFramedDecoder());
@Test
public void testEncoderDecoderIdentity() throws Exception {
ByteBuf in = Unpooled.copiedBuffer(
"Netty has been designed carefully with the experiences " +
"earned from the implementation of a lot of protocols " +
"such as FTP, SMTP, HTTP, and various binary and " +
"text-based legacy protocols", CharsetUtil.US_ASCII
);
channel.writeOutbound(in);
channel.writeInbound(channel.readOutbound());
Assert.assertEquals(in, channel.readInbound());
}
}

37
codec/src/test/java/io/netty/handler/codec/compression/SnappyTest.java
View File

@ -18,9 +18,10 @@ package io.netty.handler.codec.compression;
import io.netty.buffer.ByteBuf;
import io.netty.buffer.Unpooled;
import org.junit.After;
import org.junit.Assert;
import org.junit.Test;
import static org.junit.Assert.*;
public class SnappyTest {
private final Snappy snappy = new Snappy();
@ -40,35 +41,35 @@ public class SnappyTest {
snappy.decode(in, out, 7);
// "netty"
byte[] expected = {
ByteBuf expected = Unpooled.wrappedBuffer(new byte[] {
0x6e, 0x65, 0x74, 0x74, 0x79
};
Assert.assertArrayEquals("Literal was not decoded correctly", expected, out.array());
});
assertEquals("Literal was not decoded correctly", expected, out);
}
@Test
public void testDecodeCopyWith1ByteOffset() throws Exception {
ByteBuf in = Unpooled.wrappedBuffer(new byte[] {
0x01, // preamble length
0x0a, // preamble length
0x04 << 2, // literal tag + length
0x6e, 0x65, 0x74, 0x74, 0x79, // "netty"
0x05 << 2 | 0x01, // copy with 1-byte offset + length
0x01 << 2 | 0x01, // copy with 1-byte offset + length
0x05 // offset
});
ByteBuf out = Unpooled.buffer(10);
snappy.decode(in, out, 9);
snappy.decode(in, out, 10);
// "nettynetty" - we saved a whole byte :)
byte[] expected = {
ByteBuf expected = Unpooled.wrappedBuffer(new byte[] {
0x6e, 0x65, 0x74, 0x74, 0x79, 0x6e, 0x65, 0x74, 0x74, 0x79
};
Assert.assertArrayEquals("Copy was not decoded correctly", expected, out.array());
});
assertEquals("Copy was not decoded correctly", expected, out);
}
@Test(expected = CompressionException.class)
public void testDecodeCopyWithTinyOffset() throws Exception {
ByteBuf in = Unpooled.wrappedBuffer(new byte[] {
0x01, // preamble length
0x0a, // preamble length
0x04 << 2, // literal tag + length
0x6e, 0x65, 0x74, 0x74, 0x79, // "netty"
0x05 << 2 | 0x01, // copy with 1-byte offset + length
@ -81,7 +82,7 @@ public class SnappyTest {
@Test(expected = CompressionException.class)
public void testDecodeCopyWithOffsetBeforeChunk() throws Exception {
ByteBuf in = Unpooled.wrappedBuffer(new byte[] {
0x01, // preamble length
0x0a, // preamble length
0x04 << 2, // literal tag + length
0x6e, 0x65, 0x74, 0x74, 0x79, // "netty"
0x05 << 2 | 0x01, // copy with 1-byte offset + length
@ -110,12 +111,12 @@ public class SnappyTest {
ByteBuf out = Unpooled.buffer(7);
snappy.encode(in, out, 5);
byte[] expected = {
ByteBuf expected = Unpooled.wrappedBuffer(new byte[] {
0x05, // preamble length
0x04 << 2, // literal tag + length
0x6e, 0x65, 0x74, 0x74, 0x79 // "netty"
};
Assert.assertArrayEquals("Encoded literal was invalid", expected, out.array());
});
assertEquals("Encoded literal was invalid", expected, out);
}
@Test
@ -132,7 +133,7 @@ public class SnappyTest {
// The only compressibility in the above are the words "the ",
// and "protocols", so this is a literal, followed by a copy
// followed by another literal, followed by another copy
byte[] expected = {
ByteBuf expected = Unpooled.wrappedBuffer(new byte[] {
-0x49, 0x01, // preamble length
-0x10, 0x42, // literal tag + length
@ -163,8 +164,8 @@ public class SnappyTest {
// Second copy (protocols)
0x15, 0x4c
};
});
Assert.assertArrayEquals("Encoded result was incorrect", expected, out.array());
assertEquals("Encoded result was incorrect", expected, out);
}
}