Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix BufferOverflowException during non-Unsafe PooledDirectByteBuf resize #9912

Merged
merged 5 commits into from Jan 11, 2020

Conversation

@njhill
Copy link
Member

njhill commented Dec 30, 2019

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

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
@netty-bot

This comment has been minimized.

Copy link

netty-bot commented Dec 30, 2019

Can one of the admins verify this patch?

@normanmaurer normanmaurer added this to the 4.1.45.Final milestone Jan 10, 2020
@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Jan 10, 2020

@netty-bot test this please

@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Jan 10, 2020

@netty-bot test this please

@Test
public void testDirectArenaMemoryCopy() {
PooledByteBuf<ByteBuffer> src = unwrapIfNeeded(PooledByteBufAllocator.DEFAULT.directBuffer(512));
PooledByteBuf<ByteBuffer> dst = unwrapIfNeeded(PooledByteBufAllocator.DEFAULT.directBuffer(512));

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Jan 10, 2020

Member

@njhill this may produce leaks as you loose the "wrapper".

You need to do something like this:

ByteBuf src =PooledByteBufAllocator.DEFAULT.directBuffer(512);
ByteBuf dst = PooledByteBufAllocator.DEFAULT.directBuffer(512);

// This causes the internal reused ByteBuffer duplicate limit to be set to 128
dst.writeBytes(ByteBuffer.allocate(128));
         
// Ensure internal ByteBuffer duplicate limit is properly reset (used in memoryCopy non-Unsafe case)
PooledByteBuf<ByteBuffer> pooledSrc = unwrapIfNeeded(src);
PooledByteBuf<ByteBuffer> pooledDst = unwrapIfNeeded(dst);

pooledDst.chunk.arena.memoryCopy(pooledSrc.memory, 0, pooledDst, 512);

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

This comment has been minimized.

Copy link
@njhill

njhill Jan 10, 2020

Author Member

@normanmaurer sorry I realized this after pushing but was already falling asleep :) now fixed

@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Jan 10, 2020

@netty-bot test this please

2 similar comments
@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Jan 10, 2020

@netty-bot test this please

@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Jan 10, 2020

@netty-bot test this please

@njhill

This comment has been minimized.

Copy link
Member Author

njhill commented Jan 10, 2020

@normanmaurer hopefully this will help #9943 :)

@normanmaurer normanmaurer merged commit 607bc05 into netty:4.1 Jan 11, 2020
2 of 3 checks passed
2 of 3 checks passed
pull request validation (centos6-java8) Build finished.
Details
pull request validation (centos6-java11) Build finished.
Details
pull request validation (centos6-java12) Build finished.
Details
@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Jan 11, 2020

@njhill thanks a lot

normanmaurer added a commit that referenced this pull request Jan 11, 2020
…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
@njhill njhill deleted the njhill:directpooled-realloc-fix branch Jan 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.