Repository: netty/netty
Issue: NPE in CompositeByteBuf when reading from channel
Labels: defect

### Expected behavior
There should not be a NullPointerException.

### Actual behavior
A NullPointerException occurs:
```
io.netty.handler.codec.DecoderException: java.lang.NullPointerException
at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:280) ~[netty-all-4.1.45.Final.jar:4.1.45.Final]
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:377) [netty-all-4.1.45.Final.jar:4.1.45.Final]
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:363) [netty-all-4.1.45.Final.jar:4.1.45.Final]
at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:355) [netty-all-4.1.45.Final.jar:4.1.45.Final]
at io.netty.channel.DefaultChannelPipeline$HeadContext.channelRead(DefaultChannelPipeline.java:1410) [netty-all-4.1.45.Final.jar:4.1.45.Final]
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:377) [netty-all-4.1.45.Final.jar:4.1.45.Final]
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:363) [netty-all-4.1.45.Final.jar:4.1.45.Final]
at io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:919) [netty-all-4.1.45.Final.jar:4.1.45.Final]
at io.netty.channel.nio.AbstractNioByteChannel$NioByteUnsafe.read(AbstractNioByteChannel.java:163) [netty-all-4.1.45.Final.jar:4.1.45.Final]
at io.netty.channel.nio.NioEventLoop.processSelectedKey(NioEventLoop.java:714) [netty-all-4.1.45.Final.jar:4.1.45.Final]
at io.netty.channel.nio.NioEventLoop.processSelectedKeysOptimized(NioEventLoop.java:650) [netty-all-4.1.45.Final.jar:4.1.45.Final]
at io.netty.channel.nio.NioEventLoop.processSelectedKeys(NioEventLoop.java:576) [netty-all-4.1.45.Final.jar:4.1.45.Final]
at io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:493) [netty-all-4.1.45.Final.jar:4.1.45.Final]
at io.netty.util.concurrent.SingleThreadEventExecutor$4.run(SingleThreadEventExecutor.java:989) [netty-all-4.1.45.Final.jar:4.1.45.Final]
at io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74) [netty-all-4.1.45.Final.jar:4.1.45.Final]
at java.lang.Thread.run(Thread.java:834) [?:?]
Caused by: java.lang.NullPointerException
at io.netty.buffer.CompositeByteBuf.toComponentIndex0(CompositeByteBuf.java:893) ~[netty-all-4.1.45.Final.jar:4.1.45.Final]
at io.netty.buffer.CompositeByteBuf.addFlattenedComponents(CompositeByteBuf.java:464) ~[netty-all-4.1.45.Final.jar:4.1.45.Final]
at io.netty.handler.codec.ByteToMessageDecoder$2.cumulate(ByteToMessageDecoder.java:132) ~[netty-all-4.1.45.Final.jar:4.1.45.Final]
at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:274) ~[netty-all-4.1.45.Final.jar:4.1.45.Final]
... 15 more
```

### Steps to reproduce
Not entirely clear. A coworker discovered that this happens sometimes with very high throughput. After scrutinizing the code a bit, I suspect this is a regression introduced by #8939. The only way a NPE could occur at line 893 is if the components array is null. It looks like that can only happen if the buffer passed to addFlattenedComponents() is a WrappedCompositeByteBuf. WCBB uses the special constructor in CBB that sets components to null, and does not override toComponentIndex0().

### Netty version
4.1.45.Final

### JVM version (e.g. `java -version`)
11 (not sure exactly which update)

### OS version (e.g. `uname -a`)
Windows 10


Issue comments:
Coworker reports that this NPE does not occur anymore after downgrading Netty to 4.1.34.Final, which is the last version before #8939.
High throughput in sense of Bandwidth or Connections?
Bandwidth. AIUI he had a simulator running that only created one connection but was sending messages constantly.
@rdicroce sorry about this. Your analysis looks correct to me and this appears to be an oversight. WCBB instances need to be unwrapped here. I will provide a fix asap.

One way to work-around in the meantime would be to run with `-Dio.netty.leakDetection.level=disabled`
@rdicroce @normanmaurer please see #10247

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 48a2006a9f5..f949a2e78c0 100644
--- a/buffer/src/main/java/io/netty/buffer/CompositeByteBuf.java
+++ b/buffer/src/main/java/io/netty/buffer/CompositeByteBuf.java
@@ -473,7 +473,12 @@ public CompositeByteBuf addFlattenedComponents(boolean increaseWriterIndex, Byte
             consolidateIfNeeded();
             return this;
         }
-        final CompositeByteBuf from = (CompositeByteBuf) buffer;
+        final CompositeByteBuf from;
+        if (buffer instanceof WrappedCompositeByteBuf) {
+            from = (CompositeByteBuf) buffer.unwrap();
+        } else {
+            from = (CompositeByteBuf) buffer;
+        }
         from.checkIndex(ridx, widx - ridx);
         final Component[] fromComponents = from.components;
         final int compCountBefore = componentCount;


Test patch:
diff --git a/buffer/src/test/java/io/netty/buffer/AbstractCompositeByteBufTest.java b/buffer/src/test/java/io/netty/buffer/AbstractCompositeByteBufTest.java
index d48b266a0ed..1c36be3b4ad 100644
--- a/buffer/src/test/java/io/netty/buffer/AbstractCompositeByteBufTest.java
+++ b/buffer/src/test/java/io/netty/buffer/AbstractCompositeByteBufTest.java
@@ -24,7 +24,6 @@
 import java.nio.ByteBuffer;
 import java.nio.ByteOrder;
 import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.Collections;
 import java.util.ConcurrentModificationException;
 import java.util.Iterator;
@@ -89,7 +88,20 @@ protected ByteBuf newBuffer(int length, int maxCapacity) {
             buffers.add(EMPTY_BUFFER);
         }
 
-        ByteBuf buffer = wrappedBuffer(Integer.MAX_VALUE, buffers.toArray(new ByteBuf[0])).order(order);
+        ByteBuf buffer;
+        // Ensure that we are really testing a CompositeByteBuf
+        switch (buffers.size()) {
+            case 0:
+                buffer = compositeBuffer(Integer.MAX_VALUE);
+                break;
+            case 1:
+                buffer = compositeBuffer(Integer.MAX_VALUE).addComponent(buffers.get(0));
+                break;
+            default:
+                buffer = wrappedBuffer(Integer.MAX_VALUE, buffers.toArray(new ByteBuf[0]));
+                break;
+        }
+        buffer = buffer.order(order);
 
         // Truncate to the requested capacity.
         buffer.capacity(length);
@@ -101,6 +113,10 @@ protected ByteBuf newBuffer(int length, int maxCapacity) {
         return buffer;
     }
 
+    protected CompositeByteBuf newCompositeBuffer() {
+        return compositeBuffer();
+    }
+
     // Composite buffer does not waste bandwidth on discardReadBytes, but
     // the test will fail in strict mode.
     @Override
@@ -1124,8 +1140,17 @@ public void testInsertEmptyBufferInMiddle() {
 
     @Test
     public void testAddFlattenedComponents() {
+        testAddFlattenedComponents(false);
+    }
+
+    @Test
+    public void testAddFlattenedComponentsWithWrappedComposite() {
+        testAddFlattenedComponents(true);
+    }
+
+    private void testAddFlattenedComponents(boolean addWrapped) {
         ByteBuf b1 = Unpooled.wrappedBuffer(new byte[] { 1, 2, 3 });
-        CompositeByteBuf newComposite = Unpooled.compositeBuffer()
+        CompositeByteBuf newComposite = newCompositeBuffer()
                 .addComponent(true, b1)
                 .addFlattenedComponents(true, b1.retain())
                 .addFlattenedComponents(true, Unpooled.EMPTY_BUFFER);
@@ -1148,7 +1173,7 @@ public void testAddFlattenedComponents() {
         ByteBuf s4 = s2.retainedSlice(0, 2);
         buffer.release();
 
-        ByteBuf compositeToAdd = Unpooled.compositeBuffer()
+        CompositeByteBuf compositeToAdd = compositeBuffer()
             .addComponent(s1)
             .addComponent(Unpooled.EMPTY_BUFFER)
             .addComponents(s2, s3, s4);
@@ -1161,6 +1186,9 @@ public void testAddFlattenedComponents() {
 
         ByteBuf compositeCopy = compositeToAdd.copy();
 
+        if (addWrapped) {
+            compositeToAdd = new WrappedCompositeByteBuf(compositeToAdd);
+        }
         newComposite.addFlattenedComponents(true, compositeToAdd);
 
         // verify that added range matches
@@ -1189,7 +1217,7 @@ public void testAddFlattenedComponents() {
 
     @Test
     public void testIterator() {
-        CompositeByteBuf cbuf = compositeBuffer();
+        CompositeByteBuf cbuf = newCompositeBuffer();
         cbuf.addComponent(EMPTY_BUFFER);
         cbuf.addComponent(EMPTY_BUFFER);
 
@@ -1211,7 +1239,7 @@ public void testIterator() {
 
     @Test
     public void testEmptyIterator() {
-        CompositeByteBuf cbuf = compositeBuffer();
+        CompositeByteBuf cbuf = newCompositeBuffer();
 
         Iterator<ByteBuf> it = cbuf.iterator();
         assertFalse(it.hasNext());
@@ -1227,7 +1255,7 @@ public void testEmptyIterator() {
 
     @Test(expected = ConcurrentModificationException.class)
     public void testIteratorConcurrentModificationAdd() {
-        CompositeByteBuf cbuf = compositeBuffer();
+        CompositeByteBuf cbuf = newCompositeBuffer();
         cbuf.addComponent(EMPTY_BUFFER);
 
         Iterator<ByteBuf> it = cbuf.iterator();
@@ -1243,7 +1271,7 @@ public void testIteratorConcurrentModificationAdd() {
 
     @Test(expected = ConcurrentModificationException.class)
     public void testIteratorConcurrentModificationRemove() {
-        CompositeByteBuf cbuf = compositeBuffer();
+        CompositeByteBuf cbuf = newCompositeBuffer();
         cbuf.addComponent(EMPTY_BUFFER);
 
         Iterator<ByteBuf> it = cbuf.iterator();
@@ -1302,7 +1330,7 @@ public void testReleasesItsComponents2() {
         ByteBuf s3 = s2.readRetainedSlice(2); // 4
         ByteBuf s4 = s3.readRetainedSlice(2); // 5
 
-        ByteBuf composite = Unpooled.compositeBuffer()
+        ByteBuf composite = newCompositeBuffer()
             .addComponent(s1)
             .addComponents(s2, s3, s4)
             .order(ByteOrder.LITTLE_ENDIAN);
@@ -1327,7 +1355,7 @@ public void testReleasesOnShrink() {
         ByteBuf b2 = Unpooled.buffer(2).writeShort(2);
 
         // composite takes ownership of s1 and s2
-        ByteBuf composite = Unpooled.compositeBuffer()
+        ByteBuf composite = newCompositeBuffer()
             .addComponents(b1, b2);
 
         assertEquals(4, composite.capacity());
@@ -1357,7 +1385,7 @@ public void testReleasesOnShrink2() {
         ByteBuf b2 = b1.retainedSlice(b1.readerIndex(), 2);
 
         // composite takes ownership of b1 and b2
-        ByteBuf composite = Unpooled.compositeBuffer()
+        ByteBuf composite = newCompositeBuffer()
             .addComponents(b1, b2);
 
         assertEquals(4, composite.capacity());
@@ -1413,12 +1441,12 @@ public void testDecomposeNone() {
         testDecompose(310, 0, 0);
     }
 
-    private static void testDecompose(int offset, int length, int expectedListSize) {
+    private void testDecompose(int offset, int length, int expectedListSize) {
         byte[] bytes = new byte[1024];
         PlatformDependent.threadLocalRandom().nextBytes(bytes);
         ByteBuf buf = wrappedBuffer(bytes);
 
-        CompositeByteBuf composite = compositeBuffer();
+        CompositeByteBuf composite = newCompositeBuffer();
         composite.addComponents(true,
                                 buf.retainedSlice(100, 200),
                                 buf.retainedSlice(300, 400),
@@ -1479,8 +1507,8 @@ public void testDiscardReadBytesCorrectlyUpdatesLastAccessed() {
         testDiscardCorrectlyUpdatesLastAccessed(false);
     }
 
-    private static void testDiscardCorrectlyUpdatesLastAccessed(boolean discardSome) {
-        CompositeByteBuf cbuf = compositeBuffer();
+    private void testDiscardCorrectlyUpdatesLastAccessed(boolean discardSome) {
+        CompositeByteBuf cbuf = newCompositeBuffer();
         List<ByteBuf> buffers = new ArrayList<ByteBuf>(4);
         for (int i = 0; i < 4; i++) {
             ByteBuf buf = buffer().writeInt(i);
diff --git a/buffer/src/test/java/io/netty/buffer/WrappedCompositeByteBufTest.java b/buffer/src/test/java/io/netty/buffer/WrappedCompositeByteBufTest.java
index 7b37177b435..20d82506139 100644
--- a/buffer/src/test/java/io/netty/buffer/WrappedCompositeByteBufTest.java
+++ b/buffer/src/test/java/io/netty/buffer/WrappedCompositeByteBufTest.java
@@ -25,4 +25,9 @@ protected final ByteBuf newBuffer(int length, int maxCapacity) {
     protected WrappedCompositeByteBuf wrap(CompositeByteBuf buffer) {
         return new WrappedCompositeByteBuf(buffer);
     }
+
+    @Override
+    protected CompositeByteBuf newCompositeBuffer() {
+        return wrap(super.newCompositeBuffer());
+    }
 }