From 97bf3c0a9bcd6e3f7f9ad5465779242c0edde390 Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Tue, 27 Sep 2016 12:56:56 +0200 Subject: [PATCH] Correctly throw IndexOutOfBoundsException when dst.remaining() is too big. Motivation: In some ByteBuf implementations we not correctly implement getBytes(index, ByteBuffer). Modifications: Correct code to do what is defined in the javadocs and adding test. Result: Implementation works as described. --- .../io/netty/buffer/PooledDirectByteBuf.java | 5 ++-- .../io/netty/buffer/PooledHeapByteBuf.java | 4 +-- .../netty/buffer/UnpooledDirectByteBuf.java | 8 ++---- .../io/netty/buffer/UnpooledHeapByteBuf.java | 4 +-- .../io/netty/buffer/UnsafeByteBufUtil.java | 13 +++++----- .../io/netty/buffer/AbstractByteBufTest.java | 14 ++++++++++ .../io/netty/buffer/SlicedByteBufTest.java | 15 +++++++++++ .../java/io/netty/buffer/UnpooledTest.java | 26 +++++++++++++++++++ 8 files changed, 69 insertions(+), 20 deletions(-) diff --git a/buffer/src/main/java/io/netty/buffer/PooledDirectByteBuf.java b/buffer/src/main/java/io/netty/buffer/PooledDirectByteBuf.java index b301e483c3..7c6192bf07 100644 --- a/buffer/src/main/java/io/netty/buffer/PooledDirectByteBuf.java +++ b/buffer/src/main/java/io/netty/buffer/PooledDirectByteBuf.java @@ -158,8 +158,7 @@ final class PooledDirectByteBuf extends PooledByteBuf { } private void getBytes(int index, ByteBuffer dst, boolean internal) { - checkIndex(index); - int bytesToCopy = Math.min(capacity() - index, dst.remaining()); + checkIndex(index, dst.remaining()); ByteBuffer tmpBuf; if (internal) { tmpBuf = internalNioBuffer(); @@ -167,7 +166,7 @@ final class PooledDirectByteBuf extends PooledByteBuf { tmpBuf = memory.duplicate(); } index = idx(index); - tmpBuf.clear().position(index).limit(index + bytesToCopy); + tmpBuf.clear().position(index).limit(index + dst.remaining()); dst.put(tmpBuf); } diff --git a/buffer/src/main/java/io/netty/buffer/PooledHeapByteBuf.java b/buffer/src/main/java/io/netty/buffer/PooledHeapByteBuf.java index 0ea9598530..9878f309f8 100644 --- a/buffer/src/main/java/io/netty/buffer/PooledHeapByteBuf.java +++ b/buffer/src/main/java/io/netty/buffer/PooledHeapByteBuf.java @@ -117,8 +117,8 @@ class PooledHeapByteBuf extends PooledByteBuf { @Override public final ByteBuf getBytes(int index, ByteBuffer dst) { - checkIndex(index); - dst.put(memory, idx(index), Math.min(capacity() - index, dst.remaining())); + checkIndex(index, dst.remaining()); + dst.put(memory, idx(index), dst.remaining()); return this; } diff --git a/buffer/src/main/java/io/netty/buffer/UnpooledDirectByteBuf.java b/buffer/src/main/java/io/netty/buffer/UnpooledDirectByteBuf.java index ad2add41ed..6c813da3c5 100644 --- a/buffer/src/main/java/io/netty/buffer/UnpooledDirectByteBuf.java +++ b/buffer/src/main/java/io/netty/buffer/UnpooledDirectByteBuf.java @@ -341,19 +341,15 @@ public class UnpooledDirectByteBuf extends AbstractReferenceCountedByteBuf { } private void getBytes(int index, ByteBuffer dst, boolean internal) { - checkIndex(index); - if (dst == null) { - throw new NullPointerException("dst"); - } + checkIndex(index, dst.remaining()); - int bytesToCopy = Math.min(capacity() - index, dst.remaining()); ByteBuffer tmpBuf; if (internal) { tmpBuf = internalNioBuffer(); } else { tmpBuf = buffer.duplicate(); } - tmpBuf.clear().position(index).limit(index + bytesToCopy); + tmpBuf.clear().position(index).limit(index + dst.remaining()); dst.put(tmpBuf); } diff --git a/buffer/src/main/java/io/netty/buffer/UnpooledHeapByteBuf.java b/buffer/src/main/java/io/netty/buffer/UnpooledHeapByteBuf.java index 29ac970556..a8764fb359 100644 --- a/buffer/src/main/java/io/netty/buffer/UnpooledHeapByteBuf.java +++ b/buffer/src/main/java/io/netty/buffer/UnpooledHeapByteBuf.java @@ -180,8 +180,8 @@ public class UnpooledHeapByteBuf extends AbstractReferenceCountedByteBuf { @Override public ByteBuf getBytes(int index, ByteBuffer dst) { - ensureAccessible(); - dst.put(array, index, Math.min(capacity() - index, dst.remaining())); + checkIndex(index, dst.remaining()); + dst.put(array, index, dst.remaining()); return this; } diff --git a/buffer/src/main/java/io/netty/buffer/UnsafeByteBufUtil.java b/buffer/src/main/java/io/netty/buffer/UnsafeByteBufUtil.java index 5865a0ed98..dca75cd551 100644 --- a/buffer/src/main/java/io/netty/buffer/UnsafeByteBufUtil.java +++ b/buffer/src/main/java/io/netty/buffer/UnsafeByteBufUtil.java @@ -517,9 +517,8 @@ final class UnsafeByteBufUtil { } static void getBytes(AbstractByteBuf buf, long addr, int index, ByteBuffer dst) { - buf.checkIndex(index); - int bytesToCopy = Math.min(buf.capacity() - index, dst.remaining()); - if (bytesToCopy == 0) { + buf.checkIndex(index, dst.remaining()); + if (dst.remaining() == 0) { return; } @@ -530,12 +529,12 @@ final class UnsafeByteBufUtil { } // Copy to direct memory long dstAddress = PlatformDependent.directBufferAddress(dst); - PlatformDependent.copyMemory(addr, dstAddress + dst.position(), bytesToCopy); - dst.position(dst.position() + bytesToCopy); + PlatformDependent.copyMemory(addr, dstAddress + dst.position(), dst.remaining()); + dst.position(dst.position() + dst.remaining()); } else if (dst.hasArray()) { // Copy to array - PlatformDependent.copyMemory(addr, dst.array(), dst.arrayOffset() + dst.position(), bytesToCopy); - dst.position(dst.position() + bytesToCopy); + PlatformDependent.copyMemory(addr, dst.array(), dst.arrayOffset() + dst.position(), dst.remaining()); + dst.position(dst.position() + dst.remaining()); } else { dst.put(buf.nioBuffer()); } diff --git a/buffer/src/test/java/io/netty/buffer/AbstractByteBufTest.java b/buffer/src/test/java/io/netty/buffer/AbstractByteBufTest.java index 53ec4b1156..e16ab1652c 100644 --- a/buffer/src/test/java/io/netty/buffer/AbstractByteBufTest.java +++ b/buffer/src/test/java/io/netty/buffer/AbstractByteBufTest.java @@ -3249,6 +3249,20 @@ public abstract class AbstractByteBufTest { } } + @Test(expected = IndexOutOfBoundsException.class) + public void testGetBytesByteBuffer() { + byte[] bytes = {'a', 'b', 'c', 'd', 'e', 'f', 'g'}; + // Ensure destination buffer is bigger then what is in the ByteBuf. + ByteBuffer nioBuffer = ByteBuffer.allocate(bytes.length + 1); + ByteBuf buffer = newBuffer(bytes.length); + try { + buffer.writeBytes(bytes); + buffer.getBytes(buffer.readerIndex(), nioBuffer); + } finally { + buffer.release(); + } + } + private static void assertTrueAndRelease(ByteBuf buf, boolean actual) { try { assertTrue(actual); diff --git a/buffer/src/test/java/io/netty/buffer/SlicedByteBufTest.java b/buffer/src/test/java/io/netty/buffer/SlicedByteBufTest.java index fab85944ac..965f19e7eb 100644 --- a/buffer/src/test/java/io/netty/buffer/SlicedByteBufTest.java +++ b/buffer/src/test/java/io/netty/buffer/SlicedByteBufTest.java @@ -19,6 +19,7 @@ import io.netty.util.internal.ThreadLocalRandom; import org.junit.Test; import java.io.IOException; +import java.nio.ByteBuffer; import static org.junit.Assert.*; @@ -195,4 +196,18 @@ public class SlicedByteBufTest extends AbstractByteBufTest { assertEquals(0, slice1.refCnt()); assertEquals(0, slice2.refCnt()); } + + @Override + @Test(expected = IndexOutOfBoundsException.class) + public void testGetBytesByteBuffer() { + byte[] bytes = {'a', 'b', 'c', 'd', 'e', 'f', 'g'}; + // Ensure destination buffer is bigger then what is wrapped in the ByteBuf. + ByteBuffer nioBuffer = ByteBuffer.allocate(bytes.length + 1); + ByteBuf wrappedBuffer = Unpooled.wrappedBuffer(bytes).slice(0, bytes.length - 1); + try { + wrappedBuffer.getBytes(wrappedBuffer.readerIndex(), nioBuffer); + } finally { + wrappedBuffer.release(); + } + } } diff --git a/buffer/src/test/java/io/netty/buffer/UnpooledTest.java b/buffer/src/test/java/io/netty/buffer/UnpooledTest.java index e79b52e207..d48b818284 100644 --- a/buffer/src/test/java/io/netty/buffer/UnpooledTest.java +++ b/buffer/src/test/java/io/netty/buffer/UnpooledTest.java @@ -646,4 +646,30 @@ public class UnpooledTest { assertEquals(0, buffer4.refCnt()); assertEquals(0, wrapped.refCnt()); } + + @Test(expected = IndexOutOfBoundsException.class) + public void testGetBytesByteBuffer() { + byte[] bytes = {'a', 'b', 'c', 'd', 'e', 'f', 'g'}; + // Ensure destination buffer is bigger then what is wrapped in the ByteBuf. + ByteBuffer nioBuffer = ByteBuffer.allocate(bytes.length + 1); + ByteBuf wrappedBuffer = wrappedBuffer(bytes); + try { + wrappedBuffer.getBytes(wrappedBuffer.readerIndex(), nioBuffer); + } finally { + wrappedBuffer.release(); + } + } + + @Test(expected = IndexOutOfBoundsException.class) + public void testGetBytesByteBuffer2() { + byte[] bytes = {'a', 'b', 'c', 'd', 'e', 'f', 'g'}; + // Ensure destination buffer is bigger then what is wrapped in the ByteBuf. + ByteBuffer nioBuffer = ByteBuffer.allocate(bytes.length + 1); + ByteBuf wrappedBuffer = wrappedBuffer(bytes, 0, bytes.length); + try { + wrappedBuffer.getBytes(wrappedBuffer.readerIndex(), nioBuffer); + } finally { + wrappedBuffer.release(); + } + } }