Repository: netty/netty
Issue: BufferOverflowException with specific buffer sizes
Labels: defect

When Netty reallocates pooled buffers that are approximately 256 to 1024 bytes, a `BufferOverflowException` can be thrown. **Furthermore, the exception is swallowed** and is not printed by `LoggingHandler` (I was only able to find it by capturing it from a debugger).

I'm on JDK 13 and therefore `Unsafe` isn't available.

### Expected behavior
Buffers that are between 256 and 1024 bytes should be reallocated successfully. In my application. it always works when the buffer is outside of that range and always throws when the buffer is inside of the range.
### Actual behavior
```
io.netty.handler.codec.EncoderException: java.nio.BufferOverflowException
        at io.netty.codec@4.1.44.Final/io.netty.handler.codec.MessageToByteEncoder.write(MessageToByteEncoder.java:125)
        at io.netty.transport@4.1.44.Final/io.netty.channel.AbstractChannelHandlerContext.invokeWrite0(AbstractChannelHandlerContext.java:715)
        at io.netty.transport@4.1.44.Final/io.netty.channel.AbstractChannelHandlerContext.invokeWrite(AbstractChannelHandlerContext.java:707)
        at io.netty.transport@4.1.44.Final/io.netty.channel.AbstractChannelHandlerContext.write(AbstractChannelHandlerContext.java:790)
        at io.netty.transport@4.1.44.Final/io.netty.channel.AbstractChannelHandlerContext.write(AbstractChannelHandlerContext.java:700)
        at io.netty.codec@4.1.44.Final/io.netty.handler.codec.MessageToMessageEncoder.write(MessageToMessageEncoder.java:112)
        at io.netty.transport@4.1.44.Final/io.netty.channel.AbstractChannelHandlerContext.invokeWrite0(AbstractChannelHandlerContext.java:715)
        at io.netty.transport@4.1.44.Final/io.netty.channel.AbstractChannelHandlerContext.invokeWrite(AbstractChannelHandlerContext.java:707)
        at io.netty.transport@4.1.44.Final/io.netty.channel.AbstractChannelHandlerContext.write(AbstractChannelHandlerContext.java:790)
        at io.netty.transport@4.1.44.Final/io.netty.channel.AbstractChannelHandlerContext.write(AbstractChannelHandlerContext.java:700)
        at io.netty.handler@4.1.44.Final/io.netty.handler.logging.LoggingHandler.write(LoggingHandler.java:235)
        at io.netty.transport@4.1.44.Final/io.netty.channel.AbstractChannelHandlerContext.invokeWrite0(AbstractChannelHandlerContext.java:715)
        at io.netty.transport@4.1.44.Final/io.netty.channel.AbstractChannelHandlerContext.invokeWriteAndFlush(AbstractChannelHandlerContext.java:762)
        at io.netty.transport@4.1.44.Final/io.netty.channel.AbstractChannelHandlerContext$WriteTask.run(AbstractChannelHandlerContext.java:1089)
        at io.netty.common@4.1.44.Final/io.netty.util.concurrent.AbstractEventExecutor.safeExecute(AbstractEventExecutor.java:164)
        at io.netty.common@4.1.44.Final/io.netty.util.concurrent.SingleThreadEventExecutor.runAllTasks(SingleThreadEventExecutor.java:472)
        at io.netty.transport@4.1.44.Final/io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:500)
        at io.netty.common@4.1.44.Final/io.netty.util.concurrent.SingleThreadEventExecutor$4.run(SingleThreadEventExecutor.java:989)
        at io.netty.common@4.1.44.Final/io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74)
        at io.netty.common@4.1.44.Final/io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
        at java.base/java.lang.Thread.run(Thread.java:830)
Caused by: java.nio.BufferOverflowException
        at java.base/java.nio.DirectByteBuffer.put(DirectByteBuffer.java:411)
        at io.netty.buffer@4.1.44.Final/io.netty.buffer.PoolArena$DirectArena.memoryCopy(PoolArena.java:795)
        at io.netty.buffer@4.1.44.Final/io.netty.buffer.PoolArena$DirectArena.memoryCopy(PoolArena.java:704)
        at io.netty.buffer@4.1.44.Final/io.netty.buffer.PoolArena.reallocate(PoolArena.java:405)
        at io.netty.buffer@4.1.44.Final/io.netty.buffer.PooledByteBuf.capacity(PooledByteBuf.java:118)
        at io.netty.buffer@4.1.44.Final/io.netty.buffer.AbstractByteBuf.ensureWritable0(AbstractByteBuf.java:306)
        at io.netty.buffer@4.1.44.Final/io.netty.buffer.AbstractByteBuf.ensureWritable(AbstractByteBuf.java:282)
        at io.netty.codec@4.1.44.Final/io.netty.handler.codec.protobuf.ProtobufVarint32LengthFieldPrepender.encode(ProtobufVarint32LengthFieldPrepender.java:48)
        at io.netty.codec@4.1.44.Final/io.netty.handler.codec.protobuf.ProtobufVarint32LengthFieldPrepender.encode(ProtobufVarint32LengthFieldPrepender.java:40)
        at io.netty.codec@4.1.44.Final/io.netty.handler.codec.MessageToByteEncoder.write(MessageToByteEncoder.java:107)
        ... 20 more
```
This leads to messages of certain sizes getting dropped out of the pipeline.
### Minimal yet complete reproducer code (or URL to code)
I haven't been able to reproduce outside of my application (and I tried pretty hard). I suspect that the allocation sequence of pooled buffers in my application must be important. I'll try to come up with a way to test this easily in my application.

### Netty version
```
4.1.44.Final
```
### JVM version (e.g. `java -version`)
```
openjdk 13.0.1 2019-10-15
OpenJDK Runtime Environment (build 13.0.1+9)
OpenJDK 64-Bit Server VM (build 13.0.1+9, mixed mode)
```
### OS version (e.g. `uname -a`)
```
Linux OCTOLOGY 5.4.6-arch1-1 #1 SMP PREEMPT Sat, 21 Dec 2019 16:34:41 +0000 x86_64 GNU/Linux
```

Issue comments:
Is there any other information I can give that would be helpful?
@cilki thanks for reporting, is this something that started in 4.1.44.Final?
@njhill Yes. I just tested 4.1.40.Final through 4.1.43.Final and they all work as expected.
Thanks @cilki, sorry this is my bad, I should have time to submit a fix later today but you may need to stay on 4.1.43.Final until 4.1.45 is released.

The exception swallowing is likely unrelated but we should probably investigate that separately.
Thanks. I can confirm that reverting b0feb5a81fd03f35b9e26108efc71070d0a52288 fixes the issue.
I've opened #9912 to fix the main bug, but haven't looked at the exception swallowing yet.

Pull request review comments:
@normanmaurer I added this because I noticed that it fails when running the tests with `-Dio.netty.noUnsafe=true` (I think it's actually N/A in that case). Should we include test runs with Unsafe disabled in the CI?
@njhill yes will add it.
@njhill we need to call `src.release()` and `dst.release()` as well.
@normanmaurer done
@njhill nit: remove final ?
sure, did it for the adjacent test too
@njhill this may produce leaks as you loose the "wrapper".

You need to do something like this:

```java
ByteBuf src =PooledByteBufAllocator.DEFAULT.directBuffer(512);
ByteBuf dst = PooledByteBufAllocator.DEFAULT.directBuffer(512);

// This causes the internal reused ByteBuffer duplicate limit to be set to 128
dst.writeBytes(ByteBuffer.allocate(128));
         
// Ensure internal ByteBuffer duplicate limit is properly reset (used in memoryCopy non-Unsafe case)
PooledByteBuf<ByteBuffer> pooledSrc = unwrapIfNeeded(src);
PooledByteBuf<ByteBuffer> pooledDst = unwrapIfNeeded(dst);

pooledDst.chunk.arena.memoryCopy(pooledSrc.memory, 0, pooledDst, 512);

src.release();
dst.release();
```


@normanmaurer sorry I realized this after pushing but was already falling asleep :) now fixed

Patch that resolved the issue:
diff --git a/buffer/src/main/java/io/netty/buffer/PooledByteBuf.java b/buffer/src/main/java/io/netty/buffer/PooledByteBuf.java
index 9e33c0c5942..3ef1e26cba0 100644
--- a/buffer/src/main/java/io/netty/buffer/PooledByteBuf.java
+++ b/buffer/src/main/java/io/netty/buffer/PooledByteBuf.java
@@ -154,6 +154,8 @@ protected final ByteBuffer internalNioBuffer() {
         ByteBuffer tmpNioBuf = this.tmpNioBuf;
         if (tmpNioBuf == null) {
             this.tmpNioBuf = tmpNioBuf = newInternalNioBuffer(memory);
+        } else {
+            tmpNioBuf.clear();
         }
         return tmpNioBuf;
     }
diff --git a/buffer/src/main/java/io/netty/buffer/PooledDirectByteBuf.java b/buffer/src/main/java/io/netty/buffer/PooledDirectByteBuf.java
index dee7fe6774e..3d77ecf1953 100644
--- a/buffer/src/main/java/io/netty/buffer/PooledDirectByteBuf.java
+++ b/buffer/src/main/java/io/netty/buffer/PooledDirectByteBuf.java
@@ -260,7 +260,7 @@ public ByteBuf setBytes(int index, ByteBuffer src) {
         }
 
         index = idx(index);
-        tmpBuf.clear().position(index).limit(index + length);
+        tmpBuf.limit(index + length).position(index);
         tmpBuf.put(src);
         return this;
     }
@@ -274,7 +274,7 @@ public int setBytes(int index, InputStream in, int length) throws IOException {
             return readBytes;
         }
         ByteBuffer tmpBuf = internalNioBuffer();
-        tmpBuf.clear().position(idx(index));
+        tmpBuf.position(idx(index));
         tmpBuf.put(tmp, 0, readBytes);
         return readBytes;
     }


Test patch:
diff --git a/buffer/src/test/java/io/netty/buffer/PoolArenaTest.java b/buffer/src/test/java/io/netty/buffer/PoolArenaTest.java
index 2c1bd12db32..3ad331ddb30 100644
--- a/buffer/src/test/java/io/netty/buffer/PoolArenaTest.java
+++ b/buffer/src/test/java/io/netty/buffer/PoolArenaTest.java
@@ -20,6 +20,8 @@
 import org.junit.Assert;
 import org.junit.Test;
 
+import static org.junit.Assume.assumeTrue;
+
 import java.nio.ByteBuffer;
 
 public class PoolArenaTest {
@@ -46,6 +48,7 @@ public void testNormalizeAlignedCapacity() throws Exception {
 
     @Test
     public void testDirectArenaOffsetCacheLine() throws Exception {
+        assumeTrue(PlatformDependent.hasUnsafe());
         int capacity = 5;
         int alignment = 128;
 
@@ -64,7 +67,7 @@ public void testDirectArenaOffsetCacheLine() throws Exception {
     }
 
     @Test
-    public final void testAllocationCounter() {
+    public void testAllocationCounter() {
         final PooledByteBufAllocator allocator = new PooledByteBufAllocator(
                 true,   // preferDirect
                 0,      // nHeapArena
@@ -107,4 +110,26 @@ public final void testAllocationCounter() {
         Assert.assertEquals(1, metric.numNormalDeallocations());
         Assert.assertEquals(1, metric.numNormalAllocations());
     }
+
+    @Test
+    public void testDirectArenaMemoryCopy() {
+        ByteBuf src = PooledByteBufAllocator.DEFAULT.directBuffer(512);
+        ByteBuf dst = PooledByteBufAllocator.DEFAULT.directBuffer(512);
+
+        PooledByteBuf<ByteBuffer> pooledSrc = unwrapIfNeeded(src);
+        PooledByteBuf<ByteBuffer> pooledDst = unwrapIfNeeded(dst);
+
+        // This causes the internal reused ByteBuffer duplicate limit to be set to 128
+        pooledDst.writeBytes(ByteBuffer.allocate(128));
+        // Ensure internal ByteBuffer duplicate limit is properly reset (used in memoryCopy non-Unsafe case)
+        pooledDst.chunk.arena.memoryCopy(pooledSrc.memory, 0, pooledDst, 512);
+
+        src.release();
+        dst.release();
+    }
+
+    @SuppressWarnings("unchecked")
+    private PooledByteBuf<ByteBuffer> unwrapIfNeeded(ByteBuf buf) {
+        return (PooledByteBuf<ByteBuffer>) (buf instanceof PooledByteBuf ? buf : buf.unwrap());
+    }
 }