Repository: netty/netty
Issue: Unpooled tries to double release empty buffer
Labels: defect

See:
https://github.com/netty/netty/blob/4.1/buffer/src/main/java/io/netty/buffer/Unpooled.java#L316
In unpooled when an empty buffer is at the start of the list of several to be wrapped, it first detects the empty buffer. Then without removing it from the list it passes the whole list to the CompositeByteBuf constructor. Which will attempt to take ownership of the buffer, calling release on each of the buffers it is passed. This results in release being called twice on the empty buffer, which can cause an exception to be thrown.


Issue comments:
Thanks will check

thanks for reporting @tkaitchuck !

Fixed by https://github.com/netty/netty/pull/5605


Pull request review comments:

Patch that resolved the issue:
diff --git a/buffer/src/main/java/io/netty/buffer/CompositeByteBuf.java b/buffer/src/main/java/io/netty/buffer/CompositeByteBuf.java
index 05ceb4adfff..8cd285e2bc8 100644
--- a/buffer/src/main/java/io/netty/buffer/CompositeByteBuf.java
+++ b/buffer/src/main/java/io/netty/buffer/CompositeByteBuf.java
@@ -65,6 +65,11 @@ public CompositeByteBuf(ByteBufAllocator alloc, boolean direct, int maxNumCompon
     }
 
     public CompositeByteBuf(ByteBufAllocator alloc, boolean direct, int maxNumComponents, ByteBuf... buffers) {
+        this(alloc, direct, maxNumComponents, buffers, 0, buffers.length);
+    }
+
+    CompositeByteBuf(
+            ByteBufAllocator alloc, boolean direct, int maxNumComponents, ByteBuf[] buffers, int offset, int len) {
         super(Integer.MAX_VALUE);
         if (alloc == null) {
             throw new NullPointerException("alloc");
@@ -79,7 +84,7 @@ public CompositeByteBuf(ByteBufAllocator alloc, boolean direct, int maxNumCompon
         this.maxNumComponents = maxNumComponents;
         components = newList(maxNumComponents);
 
-        addComponents0(false, 0, buffers);
+        addComponents0(false, 0, buffers, offset, len);
         consolidateIfNeeded();
         setIndex(0, capacity());
     }
@@ -202,7 +207,7 @@ public CompositeByteBuf addComponent(boolean increaseWriterIndex, ByteBuf buffer
      * ownership of all {@link ByteBuf} objects is transfered to this {@link CompositeByteBuf}.
      */
     public CompositeByteBuf addComponents(boolean increaseWriterIndex, ByteBuf... buffers) {
-        addComponents0(increaseWriterIndex, components.size(), buffers);
+        addComponents0(increaseWriterIndex, components.size(), buffers, 0, buffers.length);
         consolidateIfNeeded();
         return this;
     }
@@ -294,19 +299,19 @@ private int addComponent0(boolean increaseWriterIndex, int cIndex, ByteBuf buffe
      * ownership of all {@link ByteBuf} objects is transfered to this {@link CompositeByteBuf}.
      */
     public CompositeByteBuf addComponents(int cIndex, ByteBuf... buffers) {
-        addComponents0(false, cIndex, buffers);
+        addComponents0(false, cIndex, buffers, 0, buffers.length);
         consolidateIfNeeded();
         return this;
     }
 
-    private int addComponents0(boolean increaseWriterIndex, int cIndex, ByteBuf... buffers) {
+    private int addComponents0(boolean increaseWriterIndex, int cIndex, ByteBuf[] buffers, int offset, int len) {
         checkNotNull(buffers, "buffers");
-        int i = 0;
+        int i = offset;
         try {
             checkComponentIndex(cIndex);
 
             // No need for consolidation
-            while (i < buffers.length) {
+            while (i < len) {
                 // Increment i now to prepare for the next iteration and prevent a duplicate release (addComponent0
                 // will release if an exception occurs, and we also release in the finally block here).
                 ByteBuf b = buffers[i++];
@@ -321,7 +326,7 @@ private int addComponents0(boolean increaseWriterIndex, int cIndex, ByteBuf... b
             }
             return cIndex;
         } finally {
-            for (; i < buffers.length; ++i) {
+            for (; i < len; ++i) {
                 ByteBuf b = buffers[i];
                 if (b != null) {
                     try {
@@ -383,7 +388,7 @@ private int addComponents0(boolean increaseIndex, int cIndex, Iterable<ByteBuf>
         }
 
         Collection<ByteBuf> col = (Collection<ByteBuf>) buffers;
-        return addComponents0(increaseIndex, cIndex, col.toArray(new ByteBuf[col.size()]));
+        return addComponents0(increaseIndex, cIndex, col.toArray(new ByteBuf[col.size()]), 0 , col.size());
     }
 
     /**
diff --git a/buffer/src/main/java/io/netty/buffer/Unpooled.java b/buffer/src/main/java/io/netty/buffer/Unpooled.java
index 9b6c434c944..faaaabc2cd6 100644
--- a/buffer/src/main/java/io/netty/buffer/Unpooled.java
+++ b/buffer/src/main/java/io/netty/buffer/Unpooled.java
@@ -309,13 +309,14 @@ public static ByteBuf wrappedBuffer(int maxNumComponents, ByteBuf... buffers) {
             }
             break;
         default:
-            for (ByteBuf b: buffers) {
-                if (b.isReadable()) {
-                    return new CompositeByteBuf(ALLOC, false, maxNumComponents, buffers);
-                } else {
-                    b.release();
+            for (int i = 0; i < buffers.length; i++) {
+                ByteBuf buf = buffers[i];
+                if (buf.isReadable()) {
+                    return new CompositeByteBuf(ALLOC, false, maxNumComponents, buffers, i, buffers.length);
                 }
+                buf.release();
             }
+            break;
         }
         return EMPTY_BUFFER;
     }


Test patch:
diff --git a/buffer/src/test/java/io/netty/buffer/UnpooledTest.java b/buffer/src/test/java/io/netty/buffer/UnpooledTest.java
index d0dfc1ed292..5abf700267f 100644
--- a/buffer/src/test/java/io/netty/buffer/UnpooledTest.java
+++ b/buffer/src/test/java/io/netty/buffer/UnpooledTest.java
@@ -628,4 +628,22 @@ public void skipBytesNegativeLength() {
         ByteBuf buf = freeLater(buffer(8));
         buf.skipBytes(-1);
     }
+
+    // See https://github.com/netty/netty/issues/5597
+    @Test
+    public void testWrapByteBufArrayStartsWithNonReadable() {
+        ByteBuf buffer1 = buffer(8);
+        ByteBuf buffer2 = buffer(8).writeZero(8); // Ensure the ByteBuf is readable.
+        ByteBuf buffer3 = buffer(8);
+        ByteBuf buffer4 = buffer(8).writeZero(8); // Ensure the ByteBuf is readable.
+
+        ByteBuf wrapped = wrappedBuffer(buffer1, buffer2, buffer3, buffer4);
+        assertEquals(16, wrapped.readableBytes());
+        assertTrue(wrapped.release());
+        assertEquals(0, buffer1.refCnt());
+        assertEquals(0, buffer2.refCnt());
+        assertEquals(0, buffer3.refCnt());
+        assertEquals(0, buffer4.refCnt());
+        assertEquals(0, wrapped.refCnt());
+    }
 }