Repository: netty/netty
Issue: Leak detection combined with composite buffers results in incorrectly handled writerIndex
Labels: defect

This is a nasty bug, which only manifests with leak detection, so would be very confusing in the wild. It was discovered in a test suite which validated with a range of buffer types and resulted in a flaky test. Fortunately composite buffers are not in common use, and leak detection is not permanently on, so the bug should not effect most people most of the time. This also made it harder to hunt down.

In some places the `writerIndex` field of `AbstractByteBuf` is manipulated directly under the assumption that subclasses of `AbstractByteBuf` are safe to manipulate, while subclasses of `WrappedByteBuf` are to be unwrapped until, hopefully, an `AbstractByteBuf` will emerge.

E.g.:
```
    public static int writeAscii(ByteBuf buf, CharSequence seq) {
        // ASCII uses 1 byte per char
        final int len = seq.length();
        if (seq instanceof AsciiString) {
            AsciiString asciiString = (AsciiString) seq;
            buf.writeBytes(asciiString.array(), asciiString.arrayOffset(), len);
        } else {
            for (;;) {
                if (buf instanceof AbstractByteBuf) {
                    AbstractByteBuf byteBuf = (AbstractByteBuf) buf;
                    byteBuf.ensureWritable(len);
                    int written = writeAscii(byteBuf, byteBuf.writerIndex, seq, len);
                    byteBuf.writerIndex += written;
                    return written;
                } else if (buf instanceof WrappedByteBuf) {
                    // Unwrap as the wrapped buffer may be an AbstractByteBuf and so we can use fast-path.
                    buf = buf.unwrap();
                } else {
                    buf.writeBytes(seq.toString().getBytes(CharsetUtil.US_ASCII));
                }
            }
        }
        return len;
    }
```
The assumption breaks with `WrappedCompositeByteBuf` as it is a subclass of `AbstractByteBuf` via `CompositeByteBuf`, but should have perhaps been a subclass of `WrappedByteBuf`. The code ends up writing the data to the 0 offset, overwriting whatever it finds there.

I've hit the resulting bug on `writeUtf` and can see it would also manifest for `writeAscii`, and perhaps elsewhere. Leak detection for composite buffers extends `WrappedCompositeByteBuf`. The bug can be fixed by using the accessor methods instead of the field.

### Steps to reproduce
Run a writeAscii/Utf test with paranoid leak detection to reproduce.

### Minimal yet complete reproducer code (or URL to code)

### Netty version
4.1, perhaps earlier too, I did not check.

Issue comments:
@nitsanw doh! I will look into it
It looks like the issue is still present in 4.0.

I ran the tests from https://github.com/netty/netty/pull/8153/files#diff-21be031058403699535d72fa9ea7548fR242 against 4.0 (https://github.com/Gerrrr/netty/commit/69cefda98abf6afe7eaf35b7b0db0e95e28a7f19) and got are two failures:

```
$ mvn test -Dtest=ByteBufUtilTest
....
 T E S T S
-------------------------------------------------------
[jetty-alpn-agent] Using: alpn-boot-8.1.11.v20170118.jar
Running io.netty.buffer.ByteBufUtilTest
17:57:54.424 [main] DEBUG i.n.u.i.l.InternalLoggerFactory - Using SLF4J as the default logging framework
17:57:54.426 [main] DEBUG io.netty.util.ResourceLeakDetector - -Dio.netty.leakDetection.level: simple
17:57:54.426 [main] DEBUG io.netty.util.ResourceLeakDetector - -Dio.netty.leakDetection.targetRecords: 4
17:57:54.442 [main] DEBUG i.n.util.internal.PlatformDependent - Platform: MacOS
17:57:54.444 [main] DEBUG i.n.util.internal.PlatformDependent0 - -Dio.netty.noUnsafe: false
17:57:54.444 [main] DEBUG i.n.util.internal.PlatformDependent0 - Java version: 8
17:57:54.447 [main] DEBUG i.n.util.internal.PlatformDependent0 - sun.misc.Unsafe.theUnsafe: available
17:57:54.448 [main] DEBUG i.n.util.internal.PlatformDependent0 - sun.misc.Unsafe.copyMemory: available
17:57:54.449 [main] DEBUG i.n.util.internal.PlatformDependent0 - java.nio.Buffer.address: available
17:57:54.449 [main] DEBUG i.n.util.internal.PlatformDependent0 - direct buffer constructor: available
17:57:54.450 [main] DEBUG i.n.util.internal.PlatformDependent0 - java.nio.Bits.unaligned: available, true
17:57:54.450 [main] DEBUG i.n.util.internal.PlatformDependent0 - jdk.internal.misc.Unsafe.allocateUninitializedArray(int): unavailable prior to Java9
17:57:54.450 [main] DEBUG i.n.util.internal.PlatformDependent0 - java.nio.DirectByteBuffer.<init>(long, int): available
17:57:54.450 [main] DEBUG i.n.util.internal.PlatformDependent - sun.misc.Unsafe: available
17:57:54.451 [main] DEBUG i.n.util.internal.PlatformDependent - -Dio.netty.tmpdir: /var/folders/9b/pdm76rrd72l04r6dyf1j4lt80000gn/T (java.io.tmpdir)
17:57:54.451 [main] DEBUG i.n.util.internal.PlatformDependent - -Dio.netty.bitMode: 64 (sun.arch.data.model)
17:57:54.454 [main] DEBUG i.n.util.internal.PlatformDependent - -Dio.netty.noPreferDirect: false
17:57:54.454 [main] DEBUG i.n.util.internal.PlatformDependent - -Dio.netty.maxDirectMemory: 3817865216 bytes
17:57:54.454 [main] DEBUG i.n.util.internal.PlatformDependent - -Dio.netty.uninitializedArrayAllocationThreshold: -1
17:57:54.455 [main] DEBUG io.netty.util.internal.CleanerJava6 - java.nio.ByteBuffer.cleaner(): available
17:57:54.499 [main] DEBUG io.netty.buffer.AbstractByteBuf - -Dio.netty.buffer.bytebuf.checkAccessible: true
17:57:54.503 [main] DEBUG i.n.util.ResourceLeakDetectorFactory - Loaded default ResourceLeakDetector: io.netty.util.ResourceLeakDetector@56aac163
17:57:54.520 [main] DEBUG i.n.u.i.InternalThreadLocalMap - -Dio.netty.threadLocalMap.stringBuilder.initialSize: 1024
17:57:54.520 [main] DEBUG i.n.u.i.InternalThreadLocalMap - -Dio.netty.threadLocalMap.stringBuilder.maxSize: 4096
17:57:54.521 [main] DEBUG io.netty.buffer.ByteBufUtil - -Dio.netty.allocator.type: unpooled
17:57:54.521 [main] DEBUG io.netty.buffer.ByteBufUtil - -Dio.netty.threadLocalDirectBufferSize: 65536
17:57:54.521 [main] DEBUG io.netty.buffer.ByteBufUtil - -Dio.netty.maxThreadLocalCharBufferSize: 16384
Tests run: 29, Failures: 2, Errors: 0, Skipped: 0, Time elapsed: 0.754 sec <<< FAILURE! - in io.netty.buffer.ByteBufUtilTest
testWriteUtf8CompositeWrapped(io.netty.buffer.ByteBufUtilTest)  Time elapsed: 0.009 sec  <<< FAILURE!
java.lang.AssertionError: expected:<UnpooledByteBufAllocator$InstrumentedUnpooledUnsafeHeapByteBuf(ridx: 0, widx: 27, cap: 64)> but was:<CompositeByteBuf(ridx: 1, widx: 1, cap: 64, components=3)>
	at io.netty.buffer.ByteBufUtilTest.testWriteUtf8CompositeWrapped(ByteBufUtilTest.java:183)

testWriteUsAsciiCompositeWrapped(io.netty.buffer.ByteBufUtilTest)  Time elapsed: 0.001 sec  <<< FAILURE!
java.lang.AssertionError: expected:<UnpooledByteBufAllocator$InstrumentedUnpooledUnsafeHeapByteBuf(ridx: 0, widx: 10, cap: 16)> but was:<CompositeByteBuf(ridx: 1, widx: 1, cap: 64, components=3)>
	at io.netty.buffer.ByteBufUtilTest.testWriteUsAsciiCompositeWrapped(ByteBufUtilTest.java:138)

Heap
 PSYoungGen      total 76288K, used 52573K [0x000000076ab00000, 0x0000000770000000, 0x00000007c0000000)
  eden space 65536K, 80% used [0x000000076ab00000,0x000000076de577a0,0x000000076eb00000)
  from space 10752K, 0% used [0x000000076f580000,0x000000076f580000,0x0000000770000000)
  to   space 10752K, 0% used [0x000000076eb00000,0x000000076eb00000,0x000000076f580000)
 ParOldGen       total 175104K, used 0K [0x00000006c0000000, 0x00000006cab00000, 0x000000076ab00000)
  object space 175104K, 0% used [0x00000006c0000000,0x00000006c0000000,0x00000006cab00000)
 Metaspace       used 10029K, capacity 10146K, committed 10240K, reserved 1058816K
  class space    used 1254K, capacity 1267K, committed 1280K, reserved 1048576K

Results :

Failed tests:
  ByteBufUtilTest.testWriteUsAsciiCompositeWrapped:138 expected:<UnpooledByteBufAllocator$InstrumentedUnpooledUnsafeHeapByteBuf(ridx: 0, widx: 10, cap: 16)> but was:<CompositeByteBuf(ridx: 1, widx: 1, cap: 64, components=3)>
  ByteBufUtilTest.testWriteUtf8CompositeWrapped:183 expected:<UnpooledByteBufAllocator$InstrumentedUnpooledUnsafeHeapByteBuf(ridx: 0, widx: 27, cap: 64)> but was:<CompositeByteBuf(ridx: 1, widx: 1, cap: 64, components=3)>

Tests run: 29, Failures: 2, Errors: 0, Skipped: 0
```
@Gerrrr 4.0 is EOL, try updating to 4.1.

Pull request review comments:

Patch that resolved the issue:
diff --git a/buffer/src/main/java/io/netty/buffer/ByteBufUtil.java b/buffer/src/main/java/io/netty/buffer/ByteBufUtil.java
index 0d1b3a9b89f..e7afab36728 100644
--- a/buffer/src/main/java/io/netty/buffer/ByteBufUtil.java
+++ b/buffer/src/main/java/io/netty/buffer/ByteBufUtil.java
@@ -498,7 +498,10 @@ public static int writeUtf8(ByteBuf buf, CharSequence seq) {
      */
     public static int reserveAndWriteUtf8(ByteBuf buf, CharSequence seq, int reserveBytes) {
         for (;;) {
-            if (buf instanceof AbstractByteBuf) {
+            if (buf instanceof WrappedCompositeByteBuf) {
+                // WrappedCompositeByteBuf is a sub-class of AbstractByteBuf so it needs special handling.
+                buf = buf.unwrap();
+            } else if (buf instanceof AbstractByteBuf) {
                 AbstractByteBuf byteBuf = (AbstractByteBuf) buf;
                 byteBuf.ensureWritable0(reserveBytes);
                 int written = writeUtf8(byteBuf, byteBuf.writerIndex, seq, seq.length());
@@ -665,7 +668,10 @@ public static int writeAscii(ByteBuf buf, CharSequence seq) {
             buf.writeBytes(asciiString.array(), asciiString.arrayOffset(), len);
         } else {
             for (;;) {
-                if (buf instanceof AbstractByteBuf) {
+                if (buf instanceof WrappedCompositeByteBuf) {
+                    // WrappedCompositeByteBuf is a sub-class of AbstractByteBuf so it needs special handling.
+                    buf = buf.unwrap();
+                } else if (buf instanceof AbstractByteBuf) {
                     AbstractByteBuf byteBuf = (AbstractByteBuf) buf;
                     byteBuf.ensureWritable0(len);
                     int written = writeAscii(byteBuf, byteBuf.writerIndex, seq, len);


Test patch:
diff --git a/buffer/src/test/java/io/netty/buffer/ByteBufUtilTest.java b/buffer/src/test/java/io/netty/buffer/ByteBufUtilTest.java
index 38e36e20db3..da9b344f406 100644
--- a/buffer/src/test/java/io/netty/buffer/ByteBufUtilTest.java
+++ b/buffer/src/test/java/io/netty/buffer/ByteBufUtilTest.java
@@ -238,6 +238,42 @@ public void testWriteUsAsciiWrapped() {
         buf2.unwrap().release();
     }
 
+    @Test
+    public void testWriteUsAsciiComposite() {
+        String usAscii = "NettyRocks";
+        ByteBuf buf = Unpooled.buffer(16);
+        buf.writeBytes(usAscii.getBytes(CharsetUtil.US_ASCII));
+        ByteBuf buf2 = Unpooled.compositeBuffer().addComponent(
+                Unpooled.buffer(8)).addComponent(Unpooled.buffer(24));
+        // write some byte so we start writing with an offset.
+        buf2.writeByte(1);
+        ByteBufUtil.writeAscii(buf2, usAscii);
+
+        // Skip the previously written byte.
+        assertEquals(buf, buf2.skipBytes(1));
+
+        buf.release();
+        buf2.release();
+    }
+
+    @Test
+    public void testWriteUsAsciiCompositeWrapped() {
+        String usAscii = "NettyRocks";
+        ByteBuf buf = Unpooled.buffer(16);
+        buf.writeBytes(usAscii.getBytes(CharsetUtil.US_ASCII));
+        ByteBuf buf2 = new WrappedCompositeByteBuf(Unpooled.compositeBuffer().addComponent(
+                Unpooled.buffer(8)).addComponent(Unpooled.buffer(24)));
+        // write some byte so we start writing with an offset.
+        buf2.writeByte(1);
+        ByteBufUtil.writeAscii(buf2, usAscii);
+
+        // Skip the previously written byte.
+        assertEquals(buf, buf2.skipBytes(1));
+
+        buf.release();
+        buf2.release();
+    }
+
     @Test
     public void testWriteUtf8() {
         String usAscii = "Some UTF-8 like äÄ∏ŒŒ";
@@ -252,6 +288,42 @@ public void testWriteUtf8() {
         buf2.release();
     }
 
+    @Test
+    public void testWriteUtf8Composite() {
+        String utf8 = "Some UTF-8 like äÄ∏ŒŒ";
+        ByteBuf buf = Unpooled.buffer(16);
+        buf.writeBytes(utf8.getBytes(CharsetUtil.UTF_8));
+        ByteBuf buf2 = Unpooled.compositeBuffer().addComponent(
+                Unpooled.buffer(8)).addComponent(Unpooled.buffer(24));
+        // write some byte so we start writing with an offset.
+        buf2.writeByte(1);
+        ByteBufUtil.writeUtf8(buf2, utf8);
+
+        // Skip the previously written byte.
+        assertEquals(buf, buf2.skipBytes(1));
+
+        buf.release();
+        buf2.release();
+    }
+
+    @Test
+    public void testWriteUtf8CompositeWrapped() {
+        String utf8 = "Some UTF-8 like äÄ∏ŒŒ";
+        ByteBuf buf = Unpooled.buffer(16);
+        buf.writeBytes(utf8.getBytes(CharsetUtil.UTF_8));
+        ByteBuf buf2 = new WrappedCompositeByteBuf(Unpooled.compositeBuffer().addComponent(
+                Unpooled.buffer(8)).addComponent(Unpooled.buffer(24)));
+        // write some byte so we start writing with an offset.
+        buf2.writeByte(1);
+        ByteBufUtil.writeUtf8(buf2, utf8);
+
+        // Skip the previously written byte.
+        assertEquals(buf, buf2.skipBytes(1));
+
+        buf.release();
+        buf2.release();
+    }
+
     @Test
     public void testWriteUtf8Surrogates() {
         // leading surrogate + trailing surrogate