Fix BufferOverflowException during non-Unsafe PooledDirectByteBuf resize (#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
This commit is contained in:
parent
ba140ac6bc
commit
607bc05a2c
@ -154,6 +154,8 @@ abstract class PooledByteBuf<T> extends AbstractReferenceCountedByteBuf {
|
|||||||
ByteBuffer tmpNioBuf = this.tmpNioBuf;
|
ByteBuffer tmpNioBuf = this.tmpNioBuf;
|
||||||
if (tmpNioBuf == null) {
|
if (tmpNioBuf == null) {
|
||||||
this.tmpNioBuf = tmpNioBuf = newInternalNioBuffer(memory);
|
this.tmpNioBuf = tmpNioBuf = newInternalNioBuffer(memory);
|
||||||
|
} else {
|
||||||
|
tmpNioBuf.clear();
|
||||||
}
|
}
|
||||||
return tmpNioBuf;
|
return tmpNioBuf;
|
||||||
}
|
}
|
||||||
|
@ -260,7 +260,7 @@ final class PooledDirectByteBuf extends PooledByteBuf<ByteBuffer> {
|
|||||||
}
|
}
|
||||||
|
|
||||||
index = idx(index);
|
index = idx(index);
|
||||||
tmpBuf.clear().position(index).limit(index + length);
|
tmpBuf.limit(index + length).position(index);
|
||||||
tmpBuf.put(src);
|
tmpBuf.put(src);
|
||||||
return this;
|
return this;
|
||||||
}
|
}
|
||||||
@ -274,7 +274,7 @@ final class PooledDirectByteBuf extends PooledByteBuf<ByteBuffer> {
|
|||||||
return readBytes;
|
return readBytes;
|
||||||
}
|
}
|
||||||
ByteBuffer tmpBuf = internalNioBuffer();
|
ByteBuffer tmpBuf = internalNioBuffer();
|
||||||
tmpBuf.clear().position(idx(index));
|
tmpBuf.position(idx(index));
|
||||||
tmpBuf.put(tmp, 0, readBytes);
|
tmpBuf.put(tmp, 0, readBytes);
|
||||||
return readBytes;
|
return readBytes;
|
||||||
}
|
}
|
||||||
|
@ -20,6 +20,8 @@ import io.netty.util.internal.PlatformDependent;
|
|||||||
import org.junit.Assert;
|
import org.junit.Assert;
|
||||||
import org.junit.Test;
|
import org.junit.Test;
|
||||||
|
|
||||||
|
import static org.junit.Assume.assumeTrue;
|
||||||
|
|
||||||
import java.nio.ByteBuffer;
|
import java.nio.ByteBuffer;
|
||||||
|
|
||||||
public class PoolArenaTest {
|
public class PoolArenaTest {
|
||||||
@ -46,6 +48,7 @@ public class PoolArenaTest {
|
|||||||
|
|
||||||
@Test
|
@Test
|
||||||
public void testDirectArenaOffsetCacheLine() throws Exception {
|
public void testDirectArenaOffsetCacheLine() throws Exception {
|
||||||
|
assumeTrue(PlatformDependent.hasUnsafe());
|
||||||
int capacity = 5;
|
int capacity = 5;
|
||||||
int alignment = 128;
|
int alignment = 128;
|
||||||
|
|
||||||
@ -64,7 +67,7 @@ public class PoolArenaTest {
|
|||||||
}
|
}
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
public final void testAllocationCounter() {
|
public void testAllocationCounter() {
|
||||||
final PooledByteBufAllocator allocator = new PooledByteBufAllocator(
|
final PooledByteBufAllocator allocator = new PooledByteBufAllocator(
|
||||||
true, // preferDirect
|
true, // preferDirect
|
||||||
0, // nHeapArena
|
0, // nHeapArena
|
||||||
@ -107,4 +110,26 @@ public class PoolArenaTest {
|
|||||||
Assert.assertEquals(1, metric.numNormalDeallocations());
|
Assert.assertEquals(1, metric.numNormalDeallocations());
|
||||||
Assert.assertEquals(1, metric.numNormalAllocations());
|
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());
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
Loading…
Reference in New Issue
Block a user