Repository: netty/netty
Issue: Unpooled buffer acquired from pooled allocator may have .alloc() returning null
Labels: defect

### Expected behavior
- .alloc() returns allocator that buffer originated from. Backed by following behavior:
 - `allocator` field is reset on `deallocate()`.
 - `allocator` field is set properly in `initUnpooled()`
### Actual behavior
`.alloc()` returns null if buffer is initialized through `initUnpooled()` and was just created. It also may return wrong allocator if it was previously backed by chunk from different pooled buffer allocator.

### Steps to reproduce
https://github.com/netty/netty/blob/66b9be3a469a2cdcc5d18a8b94c679940ce002a9/buffer/src/main/java/io/netty/buffer/PooledByteBuf.java#L128 -- `alloc()` is backed by `allocator` field. As object is pooled it must be initialized during `init()` or `initUnpooled()`

https://github.com/netty/netty/blob/66b9be3a469a2cdcc5d18a8b94c679940ce002a9/buffer/src/main/java/io/netty/buffer/PooledByteBuf.java#L52 -- `allocator` initialized (as expected)

https://github.com/netty/netty/blob/66b9be3a469a2cdcc5d18a8b94c679940ce002a9/buffer/src/main/java/io/netty/buffer/PooledByteBuf.java#L60 -- `allocator` is not initialized.

### Netty version
current 4.1 branch code.

Issue comments:
Good catch let me fix it

> Am 23.02.2017 um 02:54 schrieb Max Gortman <notifications@github.com>:
> 
> Expected behavior
> 
> .alloc() returns allocator that buffer originated from. Backed by following behavior:
> allocator field is reset on deallocate().
> allocator field is set properly in initUnpooled()
> Actual behavior
> 
> .alloc() returns null if buffer is initialized through initUnpooled() and was just created. It also may return wrong allocator if it was previously backed by chunk from different pooled buffer allocator.
> 
> Steps to reproduce
> 
> https://github.com/netty/netty/blob/66b9be3a469a2cdcc5d18a8b94c679940ce002a9/buffer/src/main/java/io/netty/buffer/PooledByteBuf.java#L128 -- alloc() is backed by allocator field. As object is pooled it must be initialized during init() or initUnpooled()
> 
> https://github.com/netty/netty/blob/66b9be3a469a2cdcc5d18a8b94c679940ce002a9/buffer/src/main/java/io/netty/buffer/PooledByteBuf.java#L52 -- allocator initialized (as expected)
> 
> https://github.com/netty/netty/blob/66b9be3a469a2cdcc5d18a8b94c679940ce002a9/buffer/src/main/java/io/netty/buffer/PooledByteBuf.java#L60 -- allocator is not initialized.
> 
> Netty version
> 
> current 4.1 branch code.
> 
> —
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly, view it on GitHub, or mute the thread.
> 

@nayato PTAL https://github.com/netty/netty/pull/6440
Fixed by https://github.com/netty/netty/pull/6440

Pull request review comments:

Patch that resolved the issue:
diff --git a/buffer/src/main/java/io/netty/buffer/PooledByteBuf.java b/buffer/src/main/java/io/netty/buffer/PooledByteBuf.java
index aac5f531cfa..56a4be38723 100644
--- a/buffer/src/main/java/io/netty/buffer/PooledByteBuf.java
+++ b/buffer/src/main/java/io/netty/buffer/PooledByteBuf.java
@@ -43,30 +43,26 @@ protected PooledByteBuf(Recycler.Handle<? extends PooledByteBuf<T>> recyclerHand
     }
 
     void init(PoolChunk<T> chunk, long handle, int offset, int length, int maxLength, PoolThreadCache cache) {
+        init0(chunk, handle, offset, length, maxLength, cache);
+    }
+
+    void initUnpooled(PoolChunk<T> chunk, int length) {
+        init0(chunk, 0, chunk.offset, length, length, null);
+    }
+
+    private void init0(PoolChunk<T> chunk, long handle, int offset, int length, int maxLength, PoolThreadCache cache) {
         assert handle >= 0;
         assert chunk != null;
 
         this.chunk = chunk;
-        this.handle = handle;
         memory = chunk.memory;
         allocator = chunk.arena.parent;
+        this.cache = cache;
+        this.handle = handle;
         this.offset = offset;
         this.length = length;
         this.maxLength = maxLength;
         tmpNioBuf = null;
-        this.cache = cache;
-    }
-
-    void initUnpooled(PoolChunk<T> chunk, int length) {
-        assert chunk != null;
-
-        this.chunk = chunk;
-        handle = 0;
-        memory = chunk.memory;
-        offset = chunk.offset;
-        this.length = maxLength = length;
-        tmpNioBuf = null;
-        cache = null;
     }
 
     /**


Test patch:
diff --git a/buffer/src/test/java/io/netty/buffer/PooledByteBufAllocatorTest.java b/buffer/src/test/java/io/netty/buffer/PooledByteBufAllocatorTest.java
index 695d99095be..12c76098729 100644
--- a/buffer/src/test/java/io/netty/buffer/PooledByteBufAllocatorTest.java
+++ b/buffer/src/test/java/io/netty/buffer/PooledByteBufAllocatorTest.java
@@ -35,6 +35,7 @@
 import static java.util.concurrent.TimeUnit.MILLISECONDS;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
 
 public class PooledByteBufAllocatorTest extends AbstractByteBufAllocatorTest {
@@ -159,6 +160,26 @@ public void testTinySubpageMetric() {
         }
     }
 
+    @Test
+    public void testAllocNotNull() {
+        PooledByteBufAllocator allocator = new PooledByteBufAllocator(true, 1, 1, 8192, 11, 0, 0, 0);
+        // Huge allocation
+        testAllocNotNull(allocator, allocator.chunkSize() + 1);
+        // Normal allocation
+        testAllocNotNull(allocator, 1024);
+        // Small allocation
+        testAllocNotNull(allocator, 512);
+        // Tiny allocation
+        testAllocNotNull(allocator, 1);
+    }
+
+    private static void testAllocNotNull(PooledByteBufAllocator allocator, int capacity) {
+        ByteBuf buffer = allocator.heapBuffer(capacity);
+        assertNotNull(buffer.alloc());
+        assertTrue(buffer.release());
+        assertNotNull(buffer.alloc());
+    }
+
     @Test
     public void testFreePoolChunk() {
         int chunkSize = 16 * 1024 * 1024;