From d125adec38a0fa64011e5d77c5e4eadad69d4368 Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Mon, 17 Jul 2017 15:44:46 +0200 Subject: [PATCH] AbstractByteBuf.ensureWritable(...) should check if buffer was released Motivation: AbstractByteBuf.ensureWritable(...) should check if buffer was released and if so throw an IllegalReferenceCountException Modifications: Ensure we throw in all cases. Result: More consistent and correct behaviour --- .../java/io/netty/buffer/AbstractByteBuf.java | 29 +++++-------------- .../buffer/AbstractUnsafeSwappedByteBuf.java | 6 ++-- .../java/io/netty/buffer/ByteBufUtil.java | 4 +-- .../buffer/PooledUnsafeDirectByteBuf.java | 5 ++-- .../netty/buffer/PooledUnsafeHeapByteBuf.java | 10 ++----- .../buffer/UnpooledUnsafeDirectByteBuf.java | 5 ++-- .../buffer/UnpooledUnsafeHeapByteBuf.java | 10 ++----- .../io/netty/buffer/UnsafeByteBufUtil.java | 3 +- .../io/netty/buffer/SlicedByteBufTest.java | 12 -------- .../WrappedUnpooledUnsafeByteBufTest.java | 12 -------- 10 files changed, 26 insertions(+), 70 deletions(-) diff --git a/buffer/src/main/java/io/netty/buffer/AbstractByteBuf.java b/buffer/src/main/java/io/netty/buffer/AbstractByteBuf.java index a4efb2b97a..87a512741d 100644 --- a/buffer/src/main/java/io/netty/buffer/AbstractByteBuf.java +++ b/buffer/src/main/java/io/netty/buffer/AbstractByteBuf.java @@ -266,7 +266,8 @@ public abstract class AbstractByteBuf extends ByteBuf { return this; } - private void ensureWritable0(int minWritableBytes) { + final void ensureWritable0(int minWritableBytes) { + ensureAccessible(); if (minWritableBytes <= writableBytes()) { return; } @@ -286,6 +287,7 @@ public abstract class AbstractByteBuf extends ByteBuf { @Override public int ensureWritable(int minWritableBytes, boolean force) { + ensureAccessible(); if (minWritableBytes < 0) { throw new IllegalArgumentException(String.format( "minWritableBytes: %d (expected: >= 0)", minWritableBytes)); @@ -668,16 +670,16 @@ public abstract class AbstractByteBuf extends ByteBuf { @Override public int setCharSequence(int index, CharSequence sequence, Charset charset) { if (charset.equals(CharsetUtil.UTF_8)) { - ensureWritable(ByteBufUtil.utf8MaxBytes(sequence)); + ensureWritable0(ByteBufUtil.utf8MaxBytes(sequence)); return ByteBufUtil.writeUtf8(this, index, sequence, sequence.length()); } if (charset.equals(CharsetUtil.US_ASCII) || charset.equals(CharsetUtil.ISO_8859_1)) { int len = sequence.length(); - ensureWritable(len); + ensureWritable0(len); return ByteBufUtil.writeAscii(this, index, sequence, len); } byte[] bytes = sequence.toString().getBytes(charset); - ensureWritable(bytes.length); + ensureWritable0(bytes.length); setBytes(index, bytes); return bytes.length; } @@ -934,7 +936,6 @@ public abstract class AbstractByteBuf extends ByteBuf { @Override public ByteBuf writeByte(int value) { - ensureAccessible(); ensureWritable0(1); _setByte(writerIndex++, value); return this; @@ -942,7 +943,6 @@ public abstract class AbstractByteBuf extends ByteBuf { @Override public ByteBuf writeShort(int value) { - ensureAccessible(); ensureWritable0(2); _setShort(writerIndex, value); writerIndex += 2; @@ -951,7 +951,6 @@ public abstract class AbstractByteBuf extends ByteBuf { @Override public ByteBuf writeShortLE(int value) { - ensureAccessible(); ensureWritable0(2); _setShortLE(writerIndex, value); writerIndex += 2; @@ -960,7 +959,6 @@ public abstract class AbstractByteBuf extends ByteBuf { @Override public ByteBuf writeMedium(int value) { - ensureAccessible(); ensureWritable0(3); _setMedium(writerIndex, value); writerIndex += 3; @@ -969,7 +967,6 @@ public abstract class AbstractByteBuf extends ByteBuf { @Override public ByteBuf writeMediumLE(int value) { - ensureAccessible(); ensureWritable0(3); _setMediumLE(writerIndex, value); writerIndex += 3; @@ -978,7 +975,6 @@ public abstract class AbstractByteBuf extends ByteBuf { @Override public ByteBuf writeInt(int value) { - ensureAccessible(); ensureWritable0(4); _setInt(writerIndex, value); writerIndex += 4; @@ -987,7 +983,6 @@ public abstract class AbstractByteBuf extends ByteBuf { @Override public ByteBuf writeIntLE(int value) { - ensureAccessible(); ensureWritable0(4); _setIntLE(writerIndex, value); writerIndex += 4; @@ -996,7 +991,6 @@ public abstract class AbstractByteBuf extends ByteBuf { @Override public ByteBuf writeLong(long value) { - ensureAccessible(); ensureWritable0(8); _setLong(writerIndex, value); writerIndex += 8; @@ -1005,7 +999,6 @@ public abstract class AbstractByteBuf extends ByteBuf { @Override public ByteBuf writeLongLE(long value) { - ensureAccessible(); ensureWritable0(8); _setLongLE(writerIndex, value); writerIndex += 8; @@ -1032,7 +1025,6 @@ public abstract class AbstractByteBuf extends ByteBuf { @Override public ByteBuf writeBytes(byte[] src, int srcIndex, int length) { - ensureAccessible(); ensureWritable(length); setBytes(writerIndex, src, srcIndex, length); writerIndex += length; @@ -1064,7 +1056,6 @@ public abstract class AbstractByteBuf extends ByteBuf { @Override public ByteBuf writeBytes(ByteBuf src, int srcIndex, int length) { - ensureAccessible(); ensureWritable(length); setBytes(writerIndex, src, srcIndex, length); writerIndex += length; @@ -1073,9 +1064,8 @@ public abstract class AbstractByteBuf extends ByteBuf { @Override public ByteBuf writeBytes(ByteBuffer src) { - ensureAccessible(); int length = src.remaining(); - ensureWritable(length); + ensureWritable0(length); setBytes(writerIndex, src); writerIndex += length; return this; @@ -1084,7 +1074,6 @@ public abstract class AbstractByteBuf extends ByteBuf { @Override public int writeBytes(InputStream in, int length) throws IOException { - ensureAccessible(); ensureWritable(length); int writtenBytes = setBytes(writerIndex, in, length); if (writtenBytes > 0) { @@ -1095,7 +1084,6 @@ public abstract class AbstractByteBuf extends ByteBuf { @Override public int writeBytes(ScatteringByteChannel in, int length) throws IOException { - ensureAccessible(); ensureWritable(length); int writtenBytes = setBytes(writerIndex, in, length); if (writtenBytes > 0) { @@ -1106,7 +1094,6 @@ public abstract class AbstractByteBuf extends ByteBuf { @Override public int writeBytes(FileChannel in, long position, int length) throws IOException { - ensureAccessible(); ensureWritable(length); int writtenBytes = setBytes(writerIndex, in, position, length); if (writtenBytes > 0) { @@ -1123,7 +1110,7 @@ public abstract class AbstractByteBuf extends ByteBuf { ensureWritable(length); int wIndex = writerIndex; - checkIndex(wIndex, length); + checkIndex0(wIndex, length); int nLong = length >>> 3; int nBytes = length & 7; diff --git a/buffer/src/main/java/io/netty/buffer/AbstractUnsafeSwappedByteBuf.java b/buffer/src/main/java/io/netty/buffer/AbstractUnsafeSwappedByteBuf.java index 6d9667c7de..d5e9239cab 100644 --- a/buffer/src/main/java/io/netty/buffer/AbstractUnsafeSwappedByteBuf.java +++ b/buffer/src/main/java/io/netty/buffer/AbstractUnsafeSwappedByteBuf.java @@ -122,7 +122,7 @@ abstract class AbstractUnsafeSwappedByteBuf extends SwappedByteBuf { @Override public final ByteBuf writeShort(int value) { - wrapped.ensureWritable(2); + wrapped.ensureWritable0(2); _setShort(wrapped, wrapped.writerIndex, nativeByteOrder ? (short) value : Short.reverseBytes((short) value)); wrapped.writerIndex += 2; return this; @@ -130,7 +130,7 @@ abstract class AbstractUnsafeSwappedByteBuf extends SwappedByteBuf { @Override public final ByteBuf writeInt(int value) { - wrapped.ensureWritable(4); + wrapped.ensureWritable0(4); _setInt(wrapped, wrapped.writerIndex, nativeByteOrder ? value : Integer.reverseBytes(value)); wrapped.writerIndex += 4; return this; @@ -138,7 +138,7 @@ abstract class AbstractUnsafeSwappedByteBuf extends SwappedByteBuf { @Override public final ByteBuf writeLong(long value) { - wrapped.ensureWritable(8); + wrapped.ensureWritable0(8); _setLong(wrapped, wrapped.writerIndex, nativeByteOrder ? value : Long.reverseBytes(value)); wrapped.writerIndex += 8; return this; diff --git a/buffer/src/main/java/io/netty/buffer/ByteBufUtil.java b/buffer/src/main/java/io/netty/buffer/ByteBufUtil.java index 86898c686d..8f6c5f33d1 100644 --- a/buffer/src/main/java/io/netty/buffer/ByteBufUtil.java +++ b/buffer/src/main/java/io/netty/buffer/ByteBufUtil.java @@ -482,7 +482,7 @@ public final class ByteBufUtil { for (;;) { if (buf instanceof AbstractByteBuf) { AbstractByteBuf byteBuf = (AbstractByteBuf) buf; - byteBuf.ensureWritable(utf8MaxBytes(seq)); + byteBuf.ensureWritable0(utf8MaxBytes(seq)); int written = writeUtf8(byteBuf, byteBuf.writerIndex, seq, seq.length()); byteBuf.writerIndex += written; return written; @@ -583,7 +583,7 @@ public final class ByteBufUtil { for (;;) { if (buf instanceof AbstractByteBuf) { AbstractByteBuf byteBuf = (AbstractByteBuf) buf; - byteBuf.ensureWritable(len); + byteBuf.ensureWritable0(len); int written = writeAscii(byteBuf, byteBuf.writerIndex, seq, len); byteBuf.writerIndex += written; return written; diff --git a/buffer/src/main/java/io/netty/buffer/PooledUnsafeDirectByteBuf.java b/buffer/src/main/java/io/netty/buffer/PooledUnsafeDirectByteBuf.java index 242a653a3d..1dcc3702c4 100644 --- a/buffer/src/main/java/io/netty/buffer/PooledUnsafeDirectByteBuf.java +++ b/buffer/src/main/java/io/netty/buffer/PooledUnsafeDirectByteBuf.java @@ -374,7 +374,8 @@ final class PooledUnsafeDirectByteBuf extends PooledByteBuf { @Override public ByteBuf setZero(int index, int length) { - UnsafeByteBufUtil.setZero(this, addr(index), index, length); + checkIndex(index, length); + UnsafeByteBufUtil.setZero(addr(index), length); return this; } @@ -382,7 +383,7 @@ final class PooledUnsafeDirectByteBuf extends PooledByteBuf { public ByteBuf writeZero(int length) { ensureWritable(length); int wIndex = writerIndex; - setZero(wIndex, length); + UnsafeByteBufUtil.setZero(addr(wIndex), length); writerIndex = wIndex + length; return this; } diff --git a/buffer/src/main/java/io/netty/buffer/PooledUnsafeHeapByteBuf.java b/buffer/src/main/java/io/netty/buffer/PooledUnsafeHeapByteBuf.java index d06e02f0bc..a644450f5c 100644 --- a/buffer/src/main/java/io/netty/buffer/PooledUnsafeHeapByteBuf.java +++ b/buffer/src/main/java/io/netty/buffer/PooledUnsafeHeapByteBuf.java @@ -131,8 +131,9 @@ final class PooledUnsafeHeapByteBuf extends PooledHeapByteBuf { @Override public ByteBuf setZero(int index, int length) { if (PlatformDependent.javaVersion() >= 7) { + checkIndex(index, length); // Only do on java7+ as the needed Unsafe call was only added there. - _setZero(index, length); + UnsafeByteBufUtil.setZero(memory, idx(index), length); return this; } return super.setZero(index, length); @@ -144,18 +145,13 @@ final class PooledUnsafeHeapByteBuf extends PooledHeapByteBuf { // Only do on java7+ as the needed Unsafe call was only added there. ensureWritable(length); int wIndex = writerIndex; - _setZero(wIndex, length); + UnsafeByteBufUtil.setZero(memory, idx(wIndex), length); writerIndex = wIndex + length; return this; } return super.writeZero(length); } - private void _setZero(int index, int length) { - checkIndex(index, length); - UnsafeByteBufUtil.setZero(memory, idx(index), length); - } - @Override @Deprecated protected SwappedByteBuf newSwappedByteBuf() { diff --git a/buffer/src/main/java/io/netty/buffer/UnpooledUnsafeDirectByteBuf.java b/buffer/src/main/java/io/netty/buffer/UnpooledUnsafeDirectByteBuf.java index e2637b0f43..07bd76b80b 100644 --- a/buffer/src/main/java/io/netty/buffer/UnpooledUnsafeDirectByteBuf.java +++ b/buffer/src/main/java/io/netty/buffer/UnpooledUnsafeDirectByteBuf.java @@ -517,7 +517,8 @@ public class UnpooledUnsafeDirectByteBuf extends AbstractReferenceCountedByteBuf @Override public ByteBuf setZero(int index, int length) { - UnsafeByteBufUtil.setZero(this, addr(index), index, length); + checkIndex(index, length); + UnsafeByteBufUtil.setZero(addr(index), length); return this; } @@ -525,7 +526,7 @@ public class UnpooledUnsafeDirectByteBuf extends AbstractReferenceCountedByteBuf public ByteBuf writeZero(int length) { ensureWritable(length); int wIndex = writerIndex; - setZero(wIndex, length); + UnsafeByteBufUtil.setZero(addr(wIndex), length); writerIndex = wIndex + length; return this; } diff --git a/buffer/src/main/java/io/netty/buffer/UnpooledUnsafeHeapByteBuf.java b/buffer/src/main/java/io/netty/buffer/UnpooledUnsafeHeapByteBuf.java index b88577cb6c..51786f1567 100644 --- a/buffer/src/main/java/io/netty/buffer/UnpooledUnsafeHeapByteBuf.java +++ b/buffer/src/main/java/io/netty/buffer/UnpooledUnsafeHeapByteBuf.java @@ -245,7 +245,8 @@ class UnpooledUnsafeHeapByteBuf extends UnpooledHeapByteBuf { public ByteBuf setZero(int index, int length) { if (PlatformDependent.javaVersion() >= 7) { // Only do on java7+ as the needed Unsafe call was only added there. - _setZero(index, length); + checkIndex(index, length); + UnsafeByteBufUtil.setZero(array, index, length); return this; } return super.setZero(index, length); @@ -257,18 +258,13 @@ class UnpooledUnsafeHeapByteBuf extends UnpooledHeapByteBuf { // Only do on java7+ as the needed Unsafe call was only added there. ensureWritable(length); int wIndex = writerIndex; - _setZero(wIndex, length); + UnsafeByteBufUtil.setZero(array, wIndex, length); writerIndex = wIndex + length; return this; } return super.writeZero(length); } - private void _setZero(int index, int length) { - checkIndex(index, length); - UnsafeByteBufUtil.setZero(array, index, length); - } - @Override @Deprecated protected SwappedByteBuf newSwappedByteBuf() { diff --git a/buffer/src/main/java/io/netty/buffer/UnsafeByteBufUtil.java b/buffer/src/main/java/io/netty/buffer/UnsafeByteBufUtil.java index 332600d66c..95c68a0294 100644 --- a/buffer/src/main/java/io/netty/buffer/UnsafeByteBufUtil.java +++ b/buffer/src/main/java/io/netty/buffer/UnsafeByteBufUtil.java @@ -581,12 +581,11 @@ final class UnsafeByteBufUtil { } } - static void setZero(AbstractByteBuf buf, long addr, int index, int length) { + static void setZero(long addr, int length) { if (length == 0) { return; } - buf.checkIndex(index, length); PlatformDependent.setMemory(addr, length, ZERO); } diff --git a/buffer/src/test/java/io/netty/buffer/SlicedByteBufTest.java b/buffer/src/test/java/io/netty/buffer/SlicedByteBufTest.java index 641d342646..410f21aad3 100644 --- a/buffer/src/test/java/io/netty/buffer/SlicedByteBufTest.java +++ b/buffer/src/test/java/io/netty/buffer/SlicedByteBufTest.java @@ -94,18 +94,6 @@ public class SlicedByteBufTest extends AbstractByteBufTest { super.testNioBufferExposeOnlyRegion(); } - @Test(expected = IndexOutOfBoundsException.class) - @Override - public void testEnsureWritableAfterRelease() { - super.testEnsureWritableAfterRelease(); - } - - @Test(expected = IndexOutOfBoundsException.class) - @Override - public void testWriteZeroAfterRelease() throws IOException { - super.testWriteZeroAfterRelease(); - } - @Test(expected = IndexOutOfBoundsException.class) @Override public void testGetReadOnlyDirectDst() { diff --git a/buffer/src/test/java/io/netty/buffer/WrappedUnpooledUnsafeByteBufTest.java b/buffer/src/test/java/io/netty/buffer/WrappedUnpooledUnsafeByteBufTest.java index 88c2a0c251..e8c0ed0746 100644 --- a/buffer/src/test/java/io/netty/buffer/WrappedUnpooledUnsafeByteBufTest.java +++ b/buffer/src/test/java/io/netty/buffer/WrappedUnpooledUnsafeByteBufTest.java @@ -88,18 +88,6 @@ public class WrappedUnpooledUnsafeByteBufTest extends BigEndianUnsafeDirectByteB super.testNioBufferExposeOnlyRegion(); } - @Test(expected = IndexOutOfBoundsException.class) - @Override - public void testEnsureWritableAfterRelease() { - super.testEnsureWritableAfterRelease(); - } - - @Test(expected = IndexOutOfBoundsException.class) - @Override - public void testWriteZeroAfterRelease() throws IOException { - super.testWriteZeroAfterRelease(); - } - @Test(expected = IndexOutOfBoundsException.class) @Override public void testGetReadOnlyDirectDst() {