Fix possible ByteBuf leak when CompositeByteBuf is resized (#8946)

Motivation:

The special case fixed in #8497 also requires that we keep a derived slice when trimming components in place, as done by the capacity(int) and discardReadBytes() methods.

Modifications:

Ensure that we keep a ref to trimmed components' original retained slice in capacity(int) and discardReadBytes() methods, so that it is released properly when the they are later freed. Add unit test which fails prior to the fix.

Result:

Edge case leak is eliminated.
This commit is contained in:
Nick Hill 2019-03-22 03:18:10 -07:00 committed by Norman Maurer
parent 9681842d54
commit a71e33ae35
2 changed files with 47 additions and 2 deletions

View File

@ -752,7 +752,12 @@ public class CompositeByteBuf extends AbstractReferenceCountedByteBuf implements
if (bytesToTrim < cLength) {
// Trim the last component
c.endOffset -= bytesToTrim;
c.slice = null;
ByteBuf slice = c.slice;
if (slice != null) {
// We must replace the cached slice with a derived one to ensure that
// it can later be released properly in the case of PooledSlicedByteBuf.
c.slice = slice.slice(0, c.length());
}
break;
}
c.free();
@ -1728,10 +1733,16 @@ public class CompositeByteBuf extends AbstractReferenceCountedByteBuf implements
}
firstComponentId++;
} else {
int trimmedBytes = readerIndex - c.offset;
c.offset = 0;
c.endOffset -= readerIndex;
c.adjustment += readerIndex;
c.slice = null;
ByteBuf slice = c.slice;
if (slice != null) {
// We must replace the cached slice with a derived one to ensure that
// it can later be released properly in the case of PooledSlicedByteBuf.
c.slice = slice.slice(trimmedBytes, c.length());
}
}
removeCompRange(0, firstComponentId);

View File

@ -1242,6 +1242,40 @@ public abstract class AbstractCompositeByteBufTest extends AbstractByteBufTest {
assertEquals(0, b2.refCnt());
}
@Test
public void testReleasesOnShrink2() {
// It is important to use a pooled allocator here to ensure
// the slices returned by readRetainedSlice are of type
// PooledSlicedByteBuf, which maintains an independent refcount
// (so that we can be sure to cover this case)
ByteBuf buffer = PooledByteBufAllocator.DEFAULT.buffer();
buffer.writeShort(1).writeShort(2);
ByteBuf b1 = buffer.readRetainedSlice(2);
ByteBuf b2 = b1.retainedSlice(b1.readerIndex(), 2);
// composite takes ownership of b1 and b2
ByteBuf composite = Unpooled.compositeBuffer()
.addComponents(b1, b2);
assertEquals(4, composite.capacity());
// reduce capacity down to two, will drop the second component
composite.capacity(2);
assertEquals(2, composite.capacity());
// releasing composite should release the components
composite.release();
assertEquals(0, composite.refCnt());
assertEquals(0, b1.refCnt());
assertEquals(0, b2.refCnt());
// release last remaining ref to buffer
buffer.release();
assertEquals(0, buffer.refCnt());
}
@Test
public void testAllocatorIsSameWhenCopy() {
testAllocatorIsSameWhenCopy(false);