Skip to content

Commit

Permalink
Fix BufferOverflowException during non-Unsafe PooledDirectByteBuf res…
Browse files Browse the repository at this point in the history
…ize (#9912)


Motivation

Recent optimization #9765 introduced a bug where the native indices of
the internal reused duplicate nio buffer are not properly reset prior to
using it to copy data during a reallocation operation. This can result
in BufferOverflowExceptions thrown during ByteBuf capacity changes.

The code path in question applies only to pooled direct buffers when
Unsafe is disabled or not available.

Modification

Ensure ByteBuffer#clear() is always called on the reused internal nio
buffer prior to returning it from PooledByteBuf#internalNioBuffer()
(protected method); add unit test that exposes the bug.

Result

Fixes #9911
  • Loading branch information
njhill authored and normanmaurer committed Jan 11, 2020
1 parent ba140ac commit 607bc05
Show file tree
Hide file tree
Showing 3 changed files with 30 additions and 3 deletions.
2 changes: 2 additions & 0 deletions buffer/src/main/java/io/netty/buffer/PooledByteBuf.java
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,8 @@ protected final ByteBuffer internalNioBuffer() {
ByteBuffer tmpNioBuf = this.tmpNioBuf;
if (tmpNioBuf == null) {
this.tmpNioBuf = tmpNioBuf = newInternalNioBuffer(memory);
} else {
tmpNioBuf.clear();
}
return tmpNioBuf;
}
Expand Down
4 changes: 2 additions & 2 deletions buffer/src/main/java/io/netty/buffer/PooledDirectByteBuf.java
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,7 @@ public ByteBuf setBytes(int index, ByteBuffer src) {
}

index = idx(index);
tmpBuf.clear().position(index).limit(index + length);
tmpBuf.limit(index + length).position(index);
tmpBuf.put(src);
return this;
}
Expand All @@ -274,7 +274,7 @@ public int setBytes(int index, InputStream in, int length) throws IOException {
return readBytes;
}
ByteBuffer tmpBuf = internalNioBuffer();
tmpBuf.clear().position(idx(index));
tmpBuf.position(idx(index));
tmpBuf.put(tmp, 0, readBytes);
return readBytes;
}
Expand Down
27 changes: 26 additions & 1 deletion buffer/src/test/java/io/netty/buffer/PoolArenaTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@
import org.junit.Assert;
import org.junit.Test;

import static org.junit.Assume.assumeTrue;

import java.nio.ByteBuffer;

public class PoolArenaTest {
Expand All @@ -46,6 +48,7 @@ public void testNormalizeAlignedCapacity() throws Exception {

@Test
public void testDirectArenaOffsetCacheLine() throws Exception {
assumeTrue(PlatformDependent.hasUnsafe());
int capacity = 5;
int alignment = 128;

Expand All @@ -64,7 +67,7 @@ public void testDirectArenaOffsetCacheLine() throws Exception {
}

@Test
public final void testAllocationCounter() {
public void testAllocationCounter() {
final PooledByteBufAllocator allocator = new PooledByteBufAllocator(
true, // preferDirect
0, // nHeapArena
Expand Down Expand Up @@ -107,4 +110,26 @@ public final void testAllocationCounter() {
Assert.assertEquals(1, metric.numNormalDeallocations());
Assert.assertEquals(1, metric.numNormalAllocations());
}

@Test
public void testDirectArenaMemoryCopy() {
ByteBuf src = PooledByteBufAllocator.DEFAULT.directBuffer(512);
ByteBuf dst = PooledByteBufAllocator.DEFAULT.directBuffer(512);

PooledByteBuf<ByteBuffer> pooledSrc = unwrapIfNeeded(src);
PooledByteBuf<ByteBuffer> pooledDst = unwrapIfNeeded(dst);

// This causes the internal reused ByteBuffer duplicate limit to be set to 128
pooledDst.writeBytes(ByteBuffer.allocate(128));
// Ensure internal ByteBuffer duplicate limit is properly reset (used in memoryCopy non-Unsafe case)
pooledDst.chunk.arena.memoryCopy(pooledSrc.memory, 0, pooledDst, 512);

src.release();
dst.release();
}

@SuppressWarnings("unchecked")
private PooledByteBuf<ByteBuffer> unwrapIfNeeded(ByteBuf buf) {
return (PooledByteBuf<ByteBuffer>) (buf instanceof PooledByteBuf ? buf : buf.unwrap());
}
}

0 comments on commit 607bc05

Please sign in to comment.