Repository: netty/netty
Issue: Writing to released ByteBuf
Labels: defect

What is the concept of checking the possibility of accessing to `ByteBuf`?

Probably a bug. A `writeCharSequence` (`setCharSequence`) can allow writing to released buffer:
```java
    ByteBuf buf = Unpooled.directBuffer(16);
    buf.release();
    buf.writeCharSequence("1", CharsetUtil.US_ASCII); // <-- no exceptions
```
With an `UnpooledHeapByteBuf` this code leads to `IllegalReferenceCountException` due to call  `ensureAccessible()` in the `capacity()`. IIUC this done only to prevent NPE for released buffed. Perhaps it's better to rewrite the method like this:
```java
    public int capacity() {
        byte[] array = this.array;
        if (array != null) {
            return array.length;
        }
        // array is null if refCnt == 0
        ensureAccessible();
        return 0;
    }
```

Also, a `setCharSequence` - the only `set` method which can increase capacity...

Issue comments:
We should check the ref count before writing to the underlying storage. Typically the write/set methods use [checIndex](https://github.com/netty/netty/blob/4.1/buffer/src/main/java/io/netty/buffer/AbstractByteBuf.java#L1346) to accomplish this. I'm AFK but can look in more detail later.
That's a bug... looking now

> Am 07.07.2017 um 15:27 schrieb Scott Mitchell <notifications@github.com>:
> 
> We should check the ref count before writing to the underlying storage. Typically method use checIndex to accomplish this. I'm AFK but can look in more detail later.
> 
> —
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly, view it on GitHub, or mute the thread.
> 

Ok I have a fix... stay tuned
@fenik17 please check https://github.com/netty/netty/pull/6953
@normanmaurer #6953 this is OK. Thanks! But what about this:
> a setCharSequence - the only set method which can increase capacity...

IMHO, this is not fine.. For example:
```java
        ByteBuf buf = Unpooled.directBuffer(1);
        buf.setCharSequence(0, "AB", CharsetUtil.US_ASCII); // no exception. capacity == 2

        buf.resetWriterIndex().capacity(1);
        buf.setBytes(0, new byte[]{1, 2}); // IndexOutOfBoundsException
```
I think only `write` methods should increase capacity.
> I think only write methods should increase capacity.

But if fix it now, someone's code can breaks...
@fenik17 let me investigate later today. Need to finish some other stuff first.
@fenik17 you are correct and this is a bug. I have fixed it here: https://github.com/netty/netty/pull/6957. I think its fine to change it here as it is clearly a bug and we can just mention it in the release note to give more visibility.
@normanmaurer good. Thanks!
Fixed by https://github.com/netty/netty/pull/6953

Pull request review comments:
this feels backwards, isn't it usually the public method eg. mymethod invoking the private mymethod0?
@johnou an `ensureAccessible()` method is protected, not is public.
I was looking at `public int ensureWritable(int minWritableBytes, boolean force)` but it even with the modifier as protected it still feels backwards.
@johnou you thinks that it's better to move the call of `ensureAccessible` from `ensureWritable0` to `ensureWritable`?
@johnou there is some sense in this..
@johnou I think it is fine as otherwise I will need to duplicate some code.
the implementation of this method may call `checkIndex` but can now call `checkIndex0`. For example `PooledDirectByteBuf#setBytes(..)` and `PooledHeapByteBuf#setBytes(..)` call `checkIndex`. We should inspect all usages of `checkIndex` in buffer implementations which inherit from `AbstractByteBuf` as suggested in https://github.com/netty/netty/pull/6953#discussion_r126544205.
Discussed with @normanmaurer offline ... both methods are public so we would have to create additional methods that bypass the additional call to ensureWritable (e.g `writeBytes` calls `setBytes0` which calls `checkIndex0`) ... lets not worry about this for now.
+1... 

Patch that resolved the issue:
diff --git a/buffer/src/main/java/io/netty/buffer/AbstractByteBuf.java b/buffer/src/main/java/io/netty/buffer/AbstractByteBuf.java
index a4efb2b97ac..87a512741da 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 ByteBuf ensureWritable(int minWritableBytes) {
         return this;
     }
 
-    private void ensureWritable0(int minWritableBytes) {
+    final void ensureWritable0(int minWritableBytes) {
+        ensureAccessible();
         if (minWritableBytes <= writableBytes()) {
             return;
         }
@@ -286,6 +287,7 @@ private void ensureWritable0(int minWritableBytes) {
 
     @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 ByteBuf setZero(int index, int length) {
     @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 ByteBuf writeBoolean(boolean value) {
 
     @Override
     public ByteBuf writeByte(int value) {
-        ensureAccessible();
         ensureWritable0(1);
         _setByte(writerIndex++, value);
         return this;
@@ -942,7 +943,6 @@ public ByteBuf writeByte(int value) {
 
     @Override
     public ByteBuf writeShort(int value) {
-        ensureAccessible();
         ensureWritable0(2);
         _setShort(writerIndex, value);
         writerIndex += 2;
@@ -951,7 +951,6 @@ public ByteBuf writeShort(int value) {
 
     @Override
     public ByteBuf writeShortLE(int value) {
-        ensureAccessible();
         ensureWritable0(2);
         _setShortLE(writerIndex, value);
         writerIndex += 2;
@@ -960,7 +959,6 @@ public ByteBuf writeShortLE(int value) {
 
     @Override
     public ByteBuf writeMedium(int value) {
-        ensureAccessible();
         ensureWritable0(3);
         _setMedium(writerIndex, value);
         writerIndex += 3;
@@ -969,7 +967,6 @@ public ByteBuf writeMedium(int value) {
 
     @Override
     public ByteBuf writeMediumLE(int value) {
-        ensureAccessible();
         ensureWritable0(3);
         _setMediumLE(writerIndex, value);
         writerIndex += 3;
@@ -978,7 +975,6 @@ public ByteBuf writeMediumLE(int value) {
 
     @Override
     public ByteBuf writeInt(int value) {
-        ensureAccessible();
         ensureWritable0(4);
         _setInt(writerIndex, value);
         writerIndex += 4;
@@ -987,7 +983,6 @@ public ByteBuf writeInt(int value) {
 
     @Override
     public ByteBuf writeIntLE(int value) {
-        ensureAccessible();
         ensureWritable0(4);
         _setIntLE(writerIndex, value);
         writerIndex += 4;
@@ -996,7 +991,6 @@ public ByteBuf writeIntLE(int value) {
 
     @Override
     public ByteBuf writeLong(long value) {
-        ensureAccessible();
         ensureWritable0(8);
         _setLong(writerIndex, value);
         writerIndex += 8;
@@ -1005,7 +999,6 @@ public ByteBuf writeLong(long value) {
 
     @Override
     public ByteBuf writeLongLE(long value) {
-        ensureAccessible();
         ensureWritable0(8);
         _setLongLE(writerIndex, value);
         writerIndex += 8;
@@ -1032,7 +1025,6 @@ public ByteBuf writeDouble(double value) {
 
     @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 ByteBuf writeBytes(ByteBuf src, int length) {
 
     @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 ByteBuf writeBytes(ByteBuf src, int srcIndex, int length) {
 
     @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 ByteBuf writeBytes(ByteBuffer src) {
     @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 int writeBytes(InputStream in, int length)
 
     @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 int writeBytes(ScatteringByteChannel in, int length) throws IOException {
 
     @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 ByteBuf writeZero(int length) {
 
         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 6d9667c7de4..d5e9239cabc 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 @@ public final ByteBuf setDouble(int index, double value) {
 
     @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 @@ public final ByteBuf writeShort(int value) {
 
     @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 @@ public final ByteBuf writeInt(int value) {
 
     @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 86898c686d3..8f6c5f33d1e 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 static int writeUtf8(ByteBuf buf, CharSequence seq) {
         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 static int writeAscii(ByteBuf buf, CharSequence seq) {
             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 242a653a3dd..1dcc3702c4b 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 @@ protected SwappedByteBuf newSwappedByteBuf() {
 
     @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 @@ public ByteBuf setZero(int index, int length) {
     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 d06e02f0bcc..a644450f5ce 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 @@ protected void _setLongLE(int index, long value) {
     @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 @@ public ByteBuf writeZero(int length) {
             // 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 e2637b0f439..07bd76b80b2 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 @@ protected SwappedByteBuf newSwappedByteBuf() {
 
     @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 ByteBuf setZero(int index, int length) {
     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 b88577cb6c0..51786f1567c 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 @@ protected void _setLongLE(int index, long value) {
     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 @@ public ByteBuf writeZero(int length) {
             // 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 332600d66ce..95c68a02942 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 @@ static void getBytes(AbstractByteBuf buf, long addr, int index, OutputStream out
         }
     }
 
-    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);
     }
 


Test patch:
diff --git a/buffer/src/test/java/io/netty/buffer/SlicedByteBufTest.java b/buffer/src/test/java/io/netty/buffer/SlicedByteBufTest.java
index 641d3426469..410f21aad3d 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 void testNioBufferExposeOnlyRegion() {
         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 88c2a0c2511..e8c0ed0746c 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 void testNioBufferExposeOnlyRegion() {
         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() {