Fix leak and corruption bugs in CompositeByteBuf (#8438)

Motivation:

I came across two bugs:
- Components removed due to capacity reduction aren't released
- Offsets aren't set correctly on empty components that are added
between existing components

Modifications:

Add unit tests which expose these bugs, fix them.

Result:

Bugs are fixed
This commit is contained in:
Nick Hill 2018-10-28 02:28:18 -07:00 committed by Norman Maurer
parent b6522927d7
commit 48c45cf4ac
2 changed files with 61 additions and 15 deletions

View File

@ -260,24 +260,18 @@ public class CompositeByteBuf extends AbstractReferenceCountedByteBuf implements
// No need to consolidate - just add a component to the list.
@SuppressWarnings("deprecation")
Component c = new Component(buffer.order(ByteOrder.BIG_ENDIAN).slice());
if (cIndex == components.size()) {
wasAdded = components.add(c);
if (cIndex == 0) {
c.endOffset = readableBytes;
} else {
Component prev = components.get(cIndex - 1);
c.offset = prev.endOffset;
c.endOffset = c.offset + readableBytes;
}
} else {
components.add(cIndex, c);
wasAdded = true;
if (readableBytes != 0) {
if (readableBytes > 0 && cIndex < components.size() - 1) {
updateComponentOffsets(cIndex);
}
} else if (cIndex == 0) {
c.endOffset = readableBytes;
} else {
c.offset = components.get(cIndex - 1).endOffset;
c.endOffset = c.offset + readableBytes;
}
if (increaseWriterIndex) {
writerIndex(writerIndex() + buffer.readableBytes());
writerIndex(writerIndex() + readableBytes);
}
return cIndex;
} finally {
@ -663,6 +657,7 @@ public class CompositeByteBuf extends AbstractReferenceCountedByteBuf implements
Component c = i.previous();
if (bytesToTrim >= c.length) {
bytesToTrim -= c.length;
c.freeIfNecessary();
i.remove();
continue;
}
@ -1635,6 +1630,7 @@ public class CompositeByteBuf extends AbstractReferenceCountedByteBuf implements
if (adjustment == c.length) {
// new slice would be empty, so remove instead
firstComponentId++;
c.freeIfNecessary();
} else {
Component newC = new Component(c.buf.slice(adjustment, c.length - adjustment));
components.set(firstComponentId, newC);

View File

@ -1018,6 +1018,33 @@ public abstract class AbstractCompositeByteBufTest extends AbstractByteBufTest {
cbuf.release();
}
@Test
public void testInsertEmptyBufferInMiddle() {
CompositeByteBuf cbuf = compositeBuffer();
ByteBuf buf1 = buffer().writeByte((byte) 1);
cbuf.addComponent(true, buf1);
ByteBuf buf2 = buffer().writeByte((byte) 2);
cbuf.addComponent(true, buf2);
// insert empty one between the first two
cbuf.addComponent(true, 1, EMPTY_BUFFER);
assertEquals(2, cbuf.readableBytes());
assertEquals((byte) 1, cbuf.readByte());
assertEquals((byte) 2, cbuf.readByte());
assertEquals(2, cbuf.capacity());
assertEquals(3, cbuf.numComponents());
byte[] dest = new byte[2];
// should skip over the empty one, not throw a java.lang.Error :)
cbuf.getBytes(0, dest);
assertArrayEquals(new byte[] {1, 2}, dest);
cbuf.release();
}
@Test
public void testIterator() {
CompositeByteBuf cbuf = compositeBuffer();
@ -1117,6 +1144,29 @@ public abstract class AbstractCompositeByteBufTest extends AbstractByteBufTest {
assertEquals(0, buffer.refCnt());
}
@Test
public void testReleasesOnShrink() {
ByteBuf b1 = Unpooled.buffer(2).writeShort(1);
ByteBuf b2 = Unpooled.buffer(2).writeShort(2);
// composite takes ownership of s1 and s2
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());
}
@Test
public void testAllocatorIsSameWhenCopy() {
testAllocatorIsSameWhenCopy(false);