From 5ca273814ffde676083751352f9c957a912387b9 Mon Sep 17 00:00:00 2001
From: Scott Mitchell <scott_mitchell@apple.com>
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.
---
 .../main/java/io/netty/buffer/AbstractByteBuf.java   |  1 +
 .../java/io/netty/buffer/AbstractByteBufTest.java    | 12 ++++++++++++
 .../io/netty/buffer/SimpleLeakAwareByteBufTest.java  |  8 +++++++-
 .../buffer/SimpleLeakAwareCompositeByteBufTest.java  |  7 ++++++-
 4 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/buffer/src/main/java/io/netty/buffer/AbstractByteBuf.java b/buffer/src/main/java/io/netty/buffer/AbstractByteBuf.java
index 89d91d795a..b77f62ee44 100644
--- a/buffer/src/main/java/io/netty/buffer/AbstractByteBuf.java
+++ b/buffer/src/main/java/io/netty/buffer/AbstractByteBuf.java
@@ -686,6 +686,7 @@ public abstract class AbstractByteBuf extends ByteBuf {
 
     @Override
     public ByteBuf readSlice(int length) {
+        checkReadableBytes(length);
         ByteBuf slice = slice(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 e973fcca65..7ddeed168a 100644
--- a/buffer/src/test/java/io/netty/buffer/AbstractByteBufTest.java
+++ b/buffer/src/test/java/io/netty/buffer/AbstractByteBufTest.java
@@ -2719,6 +2719,18 @@ public abstract class AbstractByteBufTest {
         assertEquals(0, buf.refCnt());
     }
 
+    @Test(expected = IndexOutOfBoundsException.class)
+    public void testReadSliceOutOfBounds() {
+        ByteBuf buf = newBuffer(100);
+        try {
+            buf.writeZero(50);
+            buf.readSlice(51);
+            fail();
+        } finally {
+            buf.release();
+        }
+    }
+
     @Test
     public void testDuplicateRelease() {
         ByteBuf buf = newBuffer(8);
diff --git a/buffer/src/test/java/io/netty/buffer/SimpleLeakAwareByteBufTest.java b/buffer/src/test/java/io/netty/buffer/SimpleLeakAwareByteBufTest.java
index dde45ba542..c9d7ca1522 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<? extends ByteBuf> 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
diff --git a/buffer/src/test/java/io/netty/buffer/SimpleLeakAwareCompositeByteBufTest.java b/buffer/src/test/java/io/netty/buffer/SimpleLeakAwareCompositeByteBufTest.java
index 9d1a9e6ed1..2fa30a09ee 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