Repository: netty/netty
Issue: FixedCompositeByteBuf is broken copying bytes to direct buffers
Labels: defect

@normanmaurer @trustin 

In methods like

public ByteBuf getBytes(int index, ByteBuf dst, int dstIndex, int length) {

it uses 

int localLength = Math.min(length, s.capacity() - (index - adjustment));

instead of 

int localLength = Math.min(length, s.readableBytes() - (index - adjustment));

This looks like a rough copy and paste from CompositeByteBuf but unlike CompositeByteBuf it does not slice() buffers before they are added which makes capacity() == readableBytes()

This issue did not appear when copying bytes to an Unpooled buffer as that used

public ByteBuf getBytes(int index, byte[] dst, int dstIndex, int length) {

which correctly used readableBytes.

Test to demonstrate....

class FixedCompositeByteBufTest {
 ....

@Test
    public void testCopyingToOtherBuffer() {
        ByteBuf buf1 = Unpooled.directBuffer(10);
        ByteBuf buf2 = Unpooled.buffer(10);
        ByteBuf buf3 = Unpooled.directBuffer(10);
        buf1.writeBytes("a".getBytes(Charset.defaultCharset()));
        buf2.writeBytes("b".getBytes(Charset.defaultCharset()));
        buf3.writeBytes("c".getBytes(Charset.defaultCharset()));
        ByteBuf composite = unmodifiableBuffer(buf1, buf2, buf3);
        ByteBuf copy = Unpooled.directBuffer(3);
        ByteBuf copy2 = Unpooled.buffer(3);
        copy.setBytes(0, composite, 0, 3);
        copy2.setBytes(0, composite, 0, 3);
        copy.writerIndex(3);
        copy2.writerIndex(3);
        assertEquals(0, ByteBufUtil.compare(copy, composite));
        assertEquals(0, ByteBufUtil.compare(copy2, composite));
        assertEquals(0, ByteBufUtil.compare(copy, copy2));
    }
}

Looking at the code it seems like this issue would occur for other methods too

public ByteBuf getBytes(int index, OutputStream out, int length)
public ByteBuffer[] nioBuffers(int index, int length)


Issue comments:
@louiscryan sounds like a bug... would you mind creating a PR with fix + unit test?

already in progress... :)

@louiscryan yay :) Ping me once ready for review

Cherry-picked, thanks!


Pull request review comments:

Patch that resolved the issue:
diff --git a/buffer/src/main/java/io/netty/buffer/FixedCompositeByteBuf.java b/buffer/src/main/java/io/netty/buffer/FixedCompositeByteBuf.java
index 816c0b41de9..af28d682d8d 100644
--- a/buffer/src/main/java/io/netty/buffer/FixedCompositeByteBuf.java
+++ b/buffer/src/main/java/io/netty/buffer/FixedCompositeByteBuf.java
@@ -343,7 +343,7 @@ public ByteBuf getBytes(int index, ByteBuffer dst) {
             int adjustment = c.offset;
             ByteBuf s = c.buf;
             for (;;) {
-                int localLength = Math.min(length, s.capacity() - (index - adjustment));
+                int localLength = Math.min(length, s.readableBytes() - (index - adjustment));
                 dst.limit(dst.position() + localLength);
                 s.getBytes(index - adjustment, dst);
                 index += localLength;
@@ -372,7 +372,7 @@ public ByteBuf getBytes(int index, ByteBuf dst, int dstIndex, int length) {
         int adjustment = c.offset;
         ByteBuf s = c.buf;
         for (;;) {
-            int localLength = Math.min(length, s.capacity() - (index - adjustment));
+            int localLength = Math.min(length, s.readableBytes() - (index - adjustment));
             s.getBytes(index - adjustment, dst, dstIndex, localLength);
             index += localLength;
             dstIndex += localLength;
@@ -414,7 +414,7 @@ public ByteBuf getBytes(int index, OutputStream out, int length) throws IOExcept
         int adjustment = c.offset;
         ByteBuf s = c.buf;
         for (;;) {
-            int localLength = Math.min(length, s.capacity() - (index - adjustment));
+            int localLength = Math.min(length, s.readableBytes() - (index - adjustment));
             s.getBytes(index - adjustment, out, localLength);
             index += localLength;
             length -= localLength;
@@ -491,7 +491,7 @@ public ByteBuffer[] nioBuffers(int index, int length) {
             int adjustment = c.offset;
             ByteBuf s = c.buf;
             for (;;) {
-                int localLength = Math.min(length, s.capacity() - (index - adjustment));
+                int localLength = Math.min(length, s.readableBytes() - (index - adjustment));
                 switch (s.nioBufferCount()) {
                     case 0:
                         throw new UnsupportedOperationException();


Test patch:
diff --git a/buffer/src/test/java/io/netty/buffer/FixedCompositeByteBufTest.java b/buffer/src/test/java/io/netty/buffer/FixedCompositeByteBufTest.java
index 7dea5ebf567..812e140b25c 100644
--- a/buffer/src/test/java/io/netty/buffer/FixedCompositeByteBufTest.java
+++ b/buffer/src/test/java/io/netty/buffer/FixedCompositeByteBufTest.java
@@ -23,6 +23,7 @@
 import java.nio.ByteBuffer;
 import java.nio.ReadOnlyBufferException;
 import java.nio.channels.ScatteringByteChannel;
+import java.nio.charset.Charset;
 
 import static io.netty.buffer.Unpooled.*;
 import static io.netty.util.ReferenceCountUtil.*;
@@ -252,4 +253,58 @@ private static void testGatheringWritesSingleBuf(ByteBuf buf1) throws Exception
 
         buf.release();
     }
+
+    @Test
+    public void testCopyingToOtherBuffer() {
+        ByteBuf buf1 = Unpooled.directBuffer(10);
+        ByteBuf buf2 = Unpooled.buffer(10);
+        ByteBuf buf3 = Unpooled.directBuffer(10);
+        buf1.writeBytes("a".getBytes(Charset.defaultCharset()));
+        buf2.writeBytes("b".getBytes(Charset.defaultCharset()));
+        buf3.writeBytes("c".getBytes(Charset.defaultCharset()));
+        ByteBuf composite = unmodifiableBuffer(buf1, buf2, buf3);
+        ByteBuf copy = Unpooled.directBuffer(3);
+        ByteBuf copy2 = Unpooled.buffer(3);
+        copy.setBytes(0, composite, 0, 3);
+        copy2.setBytes(0, composite, 0, 3);
+        copy.writerIndex(3);
+        copy2.writerIndex(3);
+        assertEquals(0, ByteBufUtil.compare(copy, composite));
+        assertEquals(0, ByteBufUtil.compare(copy2, composite));
+        assertEquals(0, ByteBufUtil.compare(copy, copy2));
+    }
+
+    @Test
+    public void testCopyingToOutputStream() throws IOException {
+        ByteBuf buf1 = Unpooled.directBuffer(10);
+        ByteBuf buf2 = Unpooled.buffer(10);
+        ByteBuf buf3 = Unpooled.directBuffer(10);
+        buf1.writeBytes("a".getBytes(Charset.defaultCharset()));
+        buf2.writeBytes("b".getBytes(Charset.defaultCharset()));
+        buf3.writeBytes("c".getBytes(Charset.defaultCharset()));
+        ByteBuf composite = unmodifiableBuffer(buf1, buf2, buf3);
+        ByteBuf copy = Unpooled.directBuffer(3);
+        ByteBuf copy2 = Unpooled.buffer(3);
+        composite.getBytes(0, new ByteBufOutputStream(copy), 3);
+        composite.getBytes(0, new ByteBufOutputStream(copy2), 3);
+        assertEquals(0, ByteBufUtil.compare(copy, composite));
+        assertEquals(0, ByteBufUtil.compare(copy2, composite));
+        assertEquals(0, ByteBufUtil.compare(copy, copy2));
+    }
+
+    @Test
+    public void testExtractNioBuffers() {
+        ByteBuf buf1 = Unpooled.directBuffer(10);
+        ByteBuf buf2 = Unpooled.buffer(10);
+        ByteBuf buf3 = Unpooled.directBuffer(10);
+        buf1.writeBytes("a".getBytes(Charset.defaultCharset()));
+        buf2.writeBytes("b".getBytes(Charset.defaultCharset()));
+        buf3.writeBytes("c".getBytes(Charset.defaultCharset()));
+        ByteBuf composite = unmodifiableBuffer(buf1, buf2, buf3);
+        ByteBuffer[] byteBuffers = composite.nioBuffers(0, 3);
+        assertEquals(3, byteBuffers.length);
+        assertEquals(1, byteBuffers[0].limit());
+        assertEquals(1, byteBuffers[1].limit());
+        assertEquals(1, byteBuffers[2].limit());
+    }
 }