From 7a636b5968b9a5d3f2f7d35dc92362691cf26bec Mon Sep 17 00:00:00 2001
From: nickhill <nickhill@us.ibm.com>
Date: Tue, 21 May 2019 13:37:22 -0700
Subject: [PATCH] Fix possible unsafe sharing of internal NIO buffer in
 CompositeByteBuf

Motivation

A small thread-safety bug was introduced during the internal
optimizations of ComponentByteBuf made a while back in #8437. When there
is a single component which was added as a slice,
internalNioBuffer(int,int) will currently return the unwrapped slice's
un-duplicated internal NIO buffer. This is not safe since it could be
modified concurrently with other usage of that parent buffer.

Modifications

Delegate internalNioBuffer to nioBuffer in this case, which returns a
duplicate. This matches what's done in derived buffers in general
(for the same reason). Add unit test.

Result

Fixed possible thread-safety bug
---
 .../io/netty/buffer/CompositeByteBuf.java     | 13 +++++++++--
 .../buffer/AbstractCompositeByteBufTest.java  | 22 ++++++++++++++++++-
 2 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/buffer/src/main/java/io/netty/buffer/CompositeByteBuf.java b/buffer/src/main/java/io/netty/buffer/CompositeByteBuf.java
index 664cd7251f3..c157ab24894 100644
--- a/buffer/src/main/java/io/netty/buffer/CompositeByteBuf.java
+++ b/buffer/src/main/java/io/netty/buffer/CompositeByteBuf.java
@@ -1606,8 +1606,7 @@ public ByteBuffer internalNioBuffer(int index, int length) {
         case 0:
             return EMPTY_NIO_BUFFER;
         case 1:
-            Component c = components[0];
-            return c.buf.internalNioBuffer(c.idx(index), length);
+            return components[0].internalNioBuffer(index, length);
         default:
             throw new UnsupportedOperationException();
         }
@@ -1889,6 +1888,16 @@ ByteBuf duplicate() {
             return buf.duplicate().setIndex(idx(offset), idx(endOffset));
         }
 
+        ByteBuffer internalNioBuffer(int index, int length) {
+            // We must not return the unwrapped buffer's internal buffer
+            // if it was originally added as a slice - this check of the
+            // slice field is threadsafe since we only care whether it
+            // was set upon Component construction, and we aren't
+            // attempting to access the referenced slice itself
+            return slice != null ? buf.nioBuffer(idx(index), length)
+                    : buf.internalNioBuffer(idx(index), length);
+        }
+
         void free() {
             // Release the slice if present since it may have a different
             // refcount to the unwrapped buf if it is a PooledSlicedByteBuf
diff --git a/buffer/src/test/java/io/netty/buffer/AbstractCompositeByteBufTest.java b/buffer/src/test/java/io/netty/buffer/AbstractCompositeByteBufTest.java
index 45409cb5edd..a0fdd2843fe 100644
--- a/buffer/src/test/java/io/netty/buffer/AbstractCompositeByteBufTest.java
+++ b/buffer/src/test/java/io/netty/buffer/AbstractCompositeByteBufTest.java
@@ -977,7 +977,27 @@ private static void testGatheringWritesSingleBuf(ByteBuf buf1) throws Exception
     @Override
     @Test
     public void testInternalNioBuffer() {
-        // ignore
+        CompositeByteBuf buf = compositeBuffer();
+        assertEquals(0, buf.internalNioBuffer(0, 0).remaining());
+
+        // If non-derived buffer is added, its internal buffer should be returned
+        ByteBuf concreteBuffer = directBuffer().writeByte(1);
+        buf.addComponent(concreteBuffer);
+        assertSame(concreteBuffer.internalNioBuffer(0, 1), buf.internalNioBuffer(0, 1));
+        buf.release();
+
+        // In derived cases, the original internal buffer must not be used
+        buf = compositeBuffer();
+        concreteBuffer = directBuffer().writeByte(1);
+        buf.addComponent(concreteBuffer.slice());
+        assertNotSame(concreteBuffer.internalNioBuffer(0, 1), buf.internalNioBuffer(0, 1));
+        buf.release();
+
+        buf = compositeBuffer();
+        concreteBuffer = directBuffer().writeByte(1);
+        buf.addComponent(concreteBuffer.duplicate());
+        assertNotSame(concreteBuffer.internalNioBuffer(0, 1), buf.internalNioBuffer(0, 1));
+        buf.release();
     }
 
     @Test