From 48c45cf4aceb1e6e9dce8a0670af79570a11f369 Mon Sep 17 00:00:00 2001
From: Nick Hill <nickhill@us.ibm.com>
Date: Sun, 28 Oct 2018 02:28:18 -0700
Subject: [PATCH] Fix leak and corruption bugs in CompositeByteBuf (#8438)

Motivation:

I came across two bugs:
- Components removed due to capacity reduction aren't released
- Offsets aren't set correctly on empty components that are added
between existing components

Modifications:

Add unit tests which expose these bugs, fix them.

Result:

Bugs are fixed
---
 .../io/netty/buffer/CompositeByteBuf.java     | 26 ++++------
 .../buffer/AbstractCompositeByteBufTest.java  | 50 +++++++++++++++++++
 2 files changed, 61 insertions(+), 15 deletions(-)

diff --git a/buffer/src/main/java/io/netty/buffer/CompositeByteBuf.java b/buffer/src/main/java/io/netty/buffer/CompositeByteBuf.java
index f987e48681..f66a4b91d5 100644
--- a/buffer/src/main/java/io/netty/buffer/CompositeByteBuf.java
+++ b/buffer/src/main/java/io/netty/buffer/CompositeByteBuf.java
@@ -260,24 +260,18 @@ public class CompositeByteBuf extends AbstractReferenceCountedByteBuf implements
             // No need to consolidate - just add a component to the list.
             @SuppressWarnings("deprecation")
             Component c = new Component(buffer.order(ByteOrder.BIG_ENDIAN).slice());
-            if (cIndex == components.size()) {
-                wasAdded = components.add(c);
-                if (cIndex == 0) {
-                    c.endOffset = readableBytes;
-                } else {
-                    Component prev = components.get(cIndex - 1);
-                    c.offset = prev.endOffset;
-                    c.endOffset = c.offset + readableBytes;
-                }
+            components.add(cIndex, c);
+            wasAdded = true;
+            if (readableBytes > 0 && cIndex < components.size() - 1) {
+                updateComponentOffsets(cIndex);
+            } else if (cIndex == 0) {
+                c.endOffset = readableBytes;
             } else {
-                components.add(cIndex, c);
-                wasAdded = true;
-                if (readableBytes != 0) {
-                    updateComponentOffsets(cIndex);
-                }
+                c.offset = components.get(cIndex - 1).endOffset;
+                c.endOffset = c.offset + readableBytes;
             }
             if (increaseWriterIndex) {
-                writerIndex(writerIndex() + buffer.readableBytes());
+                writerIndex(writerIndex() + readableBytes);
             }
             return cIndex;
         } finally {
@@ -663,6 +657,7 @@ public class CompositeByteBuf extends AbstractReferenceCountedByteBuf implements
                 Component c = i.previous();
                 if (bytesToTrim >= c.length) {
                     bytesToTrim -= c.length;
+                    c.freeIfNecessary();
                     i.remove();
                     continue;
                 }
@@ -1635,6 +1630,7 @@ public class CompositeByteBuf extends AbstractReferenceCountedByteBuf implements
         if (adjustment == c.length) {
             // new slice would be empty, so remove instead
             firstComponentId++;
+            c.freeIfNecessary();
         } else {
             Component newC = new Component(c.buf.slice(adjustment, c.length - adjustment));
             components.set(firstComponentId, newC);
diff --git a/buffer/src/test/java/io/netty/buffer/AbstractCompositeByteBufTest.java b/buffer/src/test/java/io/netty/buffer/AbstractCompositeByteBufTest.java
index 03e55913a0..363b25d8c7 100644
--- a/buffer/src/test/java/io/netty/buffer/AbstractCompositeByteBufTest.java
+++ b/buffer/src/test/java/io/netty/buffer/AbstractCompositeByteBufTest.java
@@ -1018,6 +1018,33 @@ public abstract class AbstractCompositeByteBufTest extends AbstractByteBufTest {
         cbuf.release();
     }
 
+    @Test
+    public void testInsertEmptyBufferInMiddle() {
+        CompositeByteBuf cbuf = compositeBuffer();
+        ByteBuf buf1 = buffer().writeByte((byte) 1);
+        cbuf.addComponent(true, buf1);
+        ByteBuf buf2 = buffer().writeByte((byte) 2);
+        cbuf.addComponent(true, buf2);
+
+        // insert empty one between the first two
+        cbuf.addComponent(true, 1, EMPTY_BUFFER);
+
+        assertEquals(2, cbuf.readableBytes());
+        assertEquals((byte) 1, cbuf.readByte());
+        assertEquals((byte) 2, cbuf.readByte());
+
+        assertEquals(2, cbuf.capacity());
+        assertEquals(3, cbuf.numComponents());
+
+        byte[] dest = new byte[2];
+        // should skip over the empty one, not throw a java.lang.Error :)
+        cbuf.getBytes(0, dest);
+
+        assertArrayEquals(new byte[] {1, 2}, dest);
+
+        cbuf.release();
+    }
+
     @Test
     public void testIterator() {
         CompositeByteBuf cbuf = compositeBuffer();
@@ -1117,6 +1144,29 @@ public abstract class AbstractCompositeByteBufTest extends AbstractByteBufTest {
         assertEquals(0, buffer.refCnt());
     }
 
+    @Test
+    public void testReleasesOnShrink() {
+
+        ByteBuf b1 = Unpooled.buffer(2).writeShort(1);
+        ByteBuf b2 = Unpooled.buffer(2).writeShort(2);
+
+        // composite takes ownership of s1 and s2
+        ByteBuf composite = Unpooled.compositeBuffer()
+            .addComponents(b1, b2);
+
+        assertEquals(4, composite.capacity());
+
+        // reduce capacity down to two, will drop the second component
+        composite.capacity(2);
+        assertEquals(2, composite.capacity());
+
+        // releasing composite should release the components
+        composite.release();
+        assertEquals(0, composite.refCnt());
+        assertEquals(0, b1.refCnt());
+        assertEquals(0, b2.refCnt());
+    }
+
     @Test
     public void testAllocatorIsSameWhenCopy() {
         testAllocatorIsSameWhenCopy(false);