From 507e0a05b5685f0769174cec12bc92bcfc1f6dc6 Mon Sep 17 00:00:00 2001
From: Nick Hill <nickhill@us.ibm.com>
Date: Wed, 22 May 2019 02:07:06 -0700
Subject: [PATCH] Fix possible unsafe sharing of internal NIO buffer in
 CompositeByteBuf (#9169)

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 664cd7251f..c157ab2489 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 class CompositeByteBuf extends AbstractReferenceCountedByteBuf implements
         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 @@ public class CompositeByteBuf extends AbstractReferenceCountedByteBuf implements
             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 45409cb5ed..a0fdd2843f 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 @@ public abstract class AbstractCompositeByteBufTest extends AbstractByteBufTest {
     @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