From a8bb9dc1802f59a3a97d555e245f0b8826700bc6 Mon Sep 17 00:00:00 2001 From: Scott Mitchell Date: Fri, 17 Nov 2017 14:50:09 -0800 Subject: [PATCH] AbstractByteBuf readSlice bound check bug Motivation: AbstractByteBuf#readSlice relied upon the bounds checking of the slice operation in order to detect index out of bounds conditions. However the slice bounds checking operation allows for the slice to go beyond the writer index, and this is out of bounds for a read operation. Modifications: - AbstractByteBuf#readSlice and AbstractByteBuf#readRetainedSlice should ensure the desired amount of bytes are readable before taking a slice Result: No reading of undefined data in AbstractByteBuf#readSlice and AbstractByteBuf#readRetainedSlice. --- .../java/io/netty/buffer/AbstractByteBuf.java | 2 ++ .../io/netty/buffer/AbstractByteBufTest.java | 25 +++++++++++++++++++ .../buffer/SimpleLeakAwareByteBufTest.java | 16 +++++++++--- .../SimpleLeakAwareCompositeByteBufTest.java | 15 ++++++++--- .../codec/http2/DefaultHttp2FrameWriter.java | 4 ++- 5 files changed, 55 insertions(+), 7 deletions(-) diff --git a/buffer/src/main/java/io/netty/buffer/AbstractByteBuf.java b/buffer/src/main/java/io/netty/buffer/AbstractByteBuf.java index add15b1890..a312b9cb2c 100644 --- a/buffer/src/main/java/io/netty/buffer/AbstractByteBuf.java +++ b/buffer/src/main/java/io/netty/buffer/AbstractByteBuf.java @@ -853,6 +853,7 @@ public abstract class AbstractByteBuf extends ByteBuf { @Override public ByteBuf readSlice(int length) { + checkReadableBytes(length); ByteBuf slice = slice(readerIndex, length); readerIndex += length; return slice; @@ -860,6 +861,7 @@ public abstract class AbstractByteBuf extends ByteBuf { @Override public ByteBuf readRetainedSlice(int length) { + checkReadableBytes(length); ByteBuf slice = retainedSlice(readerIndex, length); readerIndex += length; return slice; diff --git a/buffer/src/test/java/io/netty/buffer/AbstractByteBufTest.java b/buffer/src/test/java/io/netty/buffer/AbstractByteBufTest.java index e4a117a206..fc97258f7b 100644 --- a/buffer/src/test/java/io/netty/buffer/AbstractByteBufTest.java +++ b/buffer/src/test/java/io/netty/buffer/AbstractByteBufTest.java @@ -3286,6 +3286,31 @@ public abstract class AbstractByteBufTest { assertEquals(0, buf.refCnt()); } + @Test(expected = IndexOutOfBoundsException.class) + public void testReadSliceOutOfBounds() { + testReadSliceOutOfBounds(false); + } + + @Test(expected = IndexOutOfBoundsException.class) + public void testReadRetainedSliceOutOfBounds() { + testReadSliceOutOfBounds(true); + } + + private void testReadSliceOutOfBounds(boolean retainedSlice) { + ByteBuf buf = newBuffer(100); + try { + buf.writeZero(50); + if (retainedSlice) { + buf.readRetainedSlice(51); + } else { + buf.readSlice(51); + } + fail(); + } finally { + buf.release(); + } + } + @Test public void testWriteUsAsciiCharSequenceExpand() { testWriteCharSequenceExpand(CharsetUtil.US_ASCII); diff --git a/buffer/src/test/java/io/netty/buffer/SimpleLeakAwareByteBufTest.java b/buffer/src/test/java/io/netty/buffer/SimpleLeakAwareByteBufTest.java index 3e1a55216a..6a82b2e440 100644 --- a/buffer/src/test/java/io/netty/buffer/SimpleLeakAwareByteBufTest.java +++ b/buffer/src/test/java/io/netty/buffer/SimpleLeakAwareByteBufTest.java @@ -25,6 +25,7 @@ import java.util.Queue; import static org.junit.Assert.assertSame; import static org.junit.Assert.assertTrue; +import static org.junit.Assume.assumeTrue; public class SimpleLeakAwareByteBufTest extends BigEndianHeapByteBufTest { private final Class clazz = leakClass(); @@ -84,7 +85,12 @@ public class SimpleLeakAwareByteBufTest extends BigEndianHeapByteBufTest { @Test public void testWrapReadSlice() { - assertWrapped(newBuffer(8).readSlice(1)); + ByteBuf buffer = newBuffer(8); + if (buffer.isReadable()) { + assertWrapped(buffer.readSlice(1)); + } else { + assertTrue(buffer.release()); + } } @Test @@ -97,14 +103,18 @@ public class SimpleLeakAwareByteBufTest extends BigEndianHeapByteBufTest { @Test public void testWrapRetainedSlice2() { ByteBuf buffer = newBuffer(8); - assertWrapped(buffer.retainedSlice(0, 1)); + if (buffer.isReadable()) { + assertWrapped(buffer.retainedSlice(0, 1)); + } assertTrue(buffer.release()); } @Test public void testWrapReadRetainedSlice() { ByteBuf buffer = newBuffer(8); - assertWrapped(buffer.readRetainedSlice(1)); + if (buffer.isReadable()) { + assertWrapped(buffer.readRetainedSlice(1)); + } assertTrue(buffer.release()); } diff --git a/buffer/src/test/java/io/netty/buffer/SimpleLeakAwareCompositeByteBufTest.java b/buffer/src/test/java/io/netty/buffer/SimpleLeakAwareCompositeByteBufTest.java index 3f51edb81c..713fe728a4 100644 --- a/buffer/src/test/java/io/netty/buffer/SimpleLeakAwareCompositeByteBufTest.java +++ b/buffer/src/test/java/io/netty/buffer/SimpleLeakAwareCompositeByteBufTest.java @@ -81,7 +81,12 @@ public class SimpleLeakAwareCompositeByteBufTest extends WrappedCompositeByteBuf @Test public void testWrapReadSlice() { - assertWrapped(newBuffer(8).readSlice(1)); + ByteBuf buffer = newBuffer(8); + if (buffer.isReadable()) { + assertWrapped(buffer.readSlice(1)); + } else { + assertTrue(buffer.release()); + } } @Test @@ -94,14 +99,18 @@ public class SimpleLeakAwareCompositeByteBufTest extends WrappedCompositeByteBuf @Test public void testWrapRetainedSlice2() { ByteBuf buffer = newBuffer(8); - assertWrapped(buffer.retainedSlice(0, 1)); + if (buffer.isReadable()) { + assertWrapped(buffer.retainedSlice(0, 1)); + } assertTrue(buffer.release()); } @Test public void testWrapReadRetainedSlice() { ByteBuf buffer = newBuffer(8); - assertWrapped(buffer.readRetainedSlice(1)); + if (buffer.isReadable()) { + assertWrapped(buffer.readRetainedSlice(1)); + } assertTrue(buffer.release()); } diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2FrameWriter.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2FrameWriter.java index d953665117..47aab1d1ea 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2FrameWriter.java +++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2FrameWriter.java @@ -641,7 +641,9 @@ public class DefaultHttp2FrameWriter implements Http2FrameWriter, Http2FrameSize prevPadding = padding; flags.paddingPresent(padding > 0); flags.endOfStream(endOfStream); - frameHeader = buffer.readSlice(DATA_FRAME_HEADER_LENGTH).writerIndex(0); + frameHeader = buffer.slice(buffer.readerIndex(), DATA_FRAME_HEADER_LENGTH).writerIndex(0); + buffer.setIndex(buffer.readerIndex() + DATA_FRAME_HEADER_LENGTH, + buffer.writerIndex() + DATA_FRAME_HEADER_LENGTH); int payloadLength = data + padding; writeFrameHeaderInternal(frameHeader, payloadLength, DATA, flags, streamId);