From 4e467f5c6fe9254ae3c143788dbc3593cb2bb8fc Mon Sep 17 00:00:00 2001
From: Norman Maurer <norman_maurer@apple.com>
Date: Sat, 12 Dec 2015 19:42:45 +0100
Subject: [PATCH] Throw ReadOnlyBufferException if unsafe buffer is used and
 dst is direct

Motivation:

We missed to check if the dst is ready only before using unsafe to copy data into it which lead to data-corruption. We need to ensure we respect ready only ByteBuffer.

Modifications:

- Correctly check if the dst is ready only before copy data into it in UnsafeByteBufUtil
- Also make it work for buffers that are not direct and not have an array

Result:

No more data corruption possible if the dst buffer is readonly and unsafe buffer implementation is used.
---
 .../io/netty/buffer/UnsafeByteBufUtil.java    | 13 +++++++--
 .../io/netty/buffer/AbstractByteBufTest.java  | 28 +++++++++++++++++++
 .../io/netty/buffer/SlicedByteBufTest.java    | 12 ++++++++
 3 files changed, 50 insertions(+), 3 deletions(-)

diff --git a/buffer/src/main/java/io/netty/buffer/UnsafeByteBufUtil.java b/buffer/src/main/java/io/netty/buffer/UnsafeByteBufUtil.java
index aade5a0d8a..5cbd676537 100644
--- a/buffer/src/main/java/io/netty/buffer/UnsafeByteBufUtil.java
+++ b/buffer/src/main/java/io/netty/buffer/UnsafeByteBufUtil.java
@@ -22,6 +22,7 @@ import java.io.InputStream;
 import java.io.OutputStream;
 import java.nio.ByteBuffer;
 import java.nio.ByteOrder;
+import java.nio.ReadOnlyBufferException;
 
 import static io.netty.util.internal.MathUtil.isOutOfBounds;
 import static io.netty.util.internal.ObjectUtil.checkNotNull;
@@ -514,15 +515,21 @@ final class UnsafeByteBufUtil {
         }
 
         if (dst.isDirect()) {
+            if (dst.isReadOnly()) {
+                // We need to check if dst is ready-only so we not write something in it by using Unsafe.
+                throw new ReadOnlyBufferException();
+            }
             // Copy to direct memory
             long dstAddress = PlatformDependent.directBufferAddress(dst);
             PlatformDependent.copyMemory(addr, dstAddress + dst.position(), bytesToCopy);
-        } else {
+            dst.position(dst.position() + bytesToCopy);
+        } else if (dst.hasArray()) {
             // Copy to array
             PlatformDependent.copyMemory(addr, dst.array(), dst.arrayOffset() + dst.position(), bytesToCopy);
+            dst.position(dst.position() + bytesToCopy);
+        } else  {
+            dst.put(buf.nioBuffer());
         }
-
-        dst.position(dst.position() + bytesToCopy);
     }
 
     static void setBytes(AbstractByteBuf buf, long addr, int index, ByteBuf src, int srcIndex, int length) {
diff --git a/buffer/src/test/java/io/netty/buffer/AbstractByteBufTest.java b/buffer/src/test/java/io/netty/buffer/AbstractByteBufTest.java
index f8c347a6f4..975943296f 100644
--- a/buffer/src/test/java/io/netty/buffer/AbstractByteBufTest.java
+++ b/buffer/src/test/java/io/netty/buffer/AbstractByteBufTest.java
@@ -29,6 +29,7 @@ import java.io.ByteArrayInputStream;
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
 import java.nio.ByteBuffer;
+import java.nio.ReadOnlyBufferException;
 import java.nio.channels.Channels;
 import java.nio.channels.GatheringByteChannel;
 import java.nio.channels.ScatteringByteChannel;
@@ -2866,6 +2867,33 @@ public abstract class AbstractByteBufTest {
         assertFalse(nioBuffers[0].hasRemaining());
     }
 
+    @Test
+    public void testGetReadOnlyDirectDst() {
+        testGetReadOnlyDst(true);
+    }
+
+    @Test
+    public void testGetReadOnlyHeapDst() {
+        testGetReadOnlyDst(false);
+    }
+
+    private void testGetReadOnlyDst(boolean direct) {
+        byte[] bytes = { 'a', 'b', 'c', 'd' };
+
+        ByteBuf buffer = releaseLater(newBuffer(bytes.length));
+        buffer.writeBytes(bytes);
+
+        ByteBuffer dst = direct ? ByteBuffer.allocateDirect(bytes.length) : ByteBuffer.allocate(bytes.length);
+        ByteBuffer readOnlyDst = dst.asReadOnlyBuffer();
+        try {
+            buffer.getBytes(0, readOnlyDst);
+            fail();
+        } catch (ReadOnlyBufferException e) {
+            // expected
+        }
+        assertEquals(0, readOnlyDst.position());
+    }
+
     private void testRefCnt0(final boolean parameter) throws Exception {
         for (int i = 0; i < 10; i++) {
             final CountDownLatch latch = new CountDownLatch(1);
diff --git a/buffer/src/test/java/io/netty/buffer/SlicedByteBufTest.java b/buffer/src/test/java/io/netty/buffer/SlicedByteBufTest.java
index a953a36de3..88c6e56ca7 100644
--- a/buffer/src/test/java/io/netty/buffer/SlicedByteBufTest.java
+++ b/buffer/src/test/java/io/netty/buffer/SlicedByteBufTest.java
@@ -102,6 +102,18 @@ public class SlicedByteBufTest extends AbstractByteBufTest {
         super.testWriteZeroAfterRelease();
     }
 
+    @Test(expected = IndexOutOfBoundsException.class)
+    @Override
+    public void testGetReadOnlyDirectDst() {
+        super.testGetReadOnlyDirectDst();
+    }
+
+    @Test(expected = IndexOutOfBoundsException.class)
+    @Override
+    public void testGetReadOnlyHeapDst() {
+        super.testGetReadOnlyHeapDst();
+    }
+
     @Test
     @Override
     public void testLittleEndianWithExpand() {