From 607bc05a2c1453323b2f8c67cbd4aff0d5b17531 Mon Sep 17 00:00:00 2001
From: Nick Hill <nickhill@us.ibm.com>
Date: Fri, 10 Jan 2020 21:04:32 -0800
Subject: [PATCH] Fix BufferOverflowException during non-Unsafe
 PooledDirectByteBuf resize (#9912)

Motivation

Recent optimization #9765 introduced a bug where the native indices of
the internal reused duplicate nio buffer are not properly reset prior to
using it to copy data during a reallocation operation. This can result
in BufferOverflowExceptions thrown during ByteBuf capacity changes.

The code path in question applies only to pooled direct buffers when
Unsafe is disabled or not available.

Modification

Ensure ByteBuffer#clear() is always called on the reused internal nio
buffer prior to returning it from PooledByteBuf#internalNioBuffer()
(protected method); add unit test that exposes the bug.

Result

Fixes #9911
---
 .../java/io/netty/buffer/PooledByteBuf.java   |  2 ++
 .../io/netty/buffer/PooledDirectByteBuf.java  |  4 +--
 .../java/io/netty/buffer/PoolArenaTest.java   | 27 ++++++++++++++++++-
 3 files changed, 30 insertions(+), 3 deletions(-)

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;
     }
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());
+    }
 }