andreacavalli/netty5
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Fix possible unsafe sharing of internal NIO buffer in CompositeByteBuf (#9169)

Browse Source 
Motivation

A small thread-safety bug was introduced during the internal
optimizations of ComponentByteBuf made a while back in #8437. When there
is a single component which was added as a slice,
internalNioBuffer(int,int) will currently return the unwrapped slice's
un-duplicated internal NIO buffer. This is not safe since it could be
modified concurrently with other usage of that parent buffer.

Modifications

Delegate internalNioBuffer to nioBuffer in this case, which returns a
duplicate. This matches what's done in derived buffers in general
(for the same reason). Add unit test.

Result

Fixed possible thread-safety bug
This commit is contained in:
Nick Hill 2019-05-22 02:07:06 -07:00 committed by Norman Maurer
parent 73dae929e3
commit d29f91540d
2 changed files with 32 additions and 3 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

13
buffer/src/main/java/io/netty/buffer/CompositeByteBuf.java
View File

@ -1605,8 +1605,7 @@ public class CompositeByteBuf extends AbstractReferenceCountedByteBuf implements
case 0:
return EMPTY_NIO_BUFFER;
case 1:
Component c = components[0];
return c.buf.internalNioBuffer(c.idx(index), length);
return components[0].internalNioBuffer(index, length);
default:
throw new UnsupportedOperationException();
}
@ -1884,6 +1883,16 @@ public class CompositeByteBuf extends AbstractReferenceCountedByteBuf implements
return buf.duplicate().setIndex(idx(offset), idx(endOffset));
}
ByteBuffer internalNioBuffer(int index, int length) {
// We must not return the unwrapped buffer's internal buffer
// if it was originally added as a slice - this check of the
// slice field is threadsafe since we only care whether it
// was set upon Component construction, and we aren't
// attempting to access the referenced slice itself
return slice != null ? buf.nioBuffer(idx(index), length)
: buf.internalNioBuffer(idx(index), length);
}
void free() {
// Release the slice if present since it may have a different
// refcount to the unwrapped buf if it is a PooledSlicedByteBuf

22
buffer/src/test/java/io/netty/buffer/AbstractCompositeByteBufTest.java
View File

@ -967,7 +967,27 @@ public abstract class AbstractCompositeByteBufTest extends AbstractByteBufTest {
@Override
@Test
public void testInternalNioBuffer() {
// ignore
CompositeByteBuf buf = compositeBuffer();
assertEquals(0, buf.internalNioBuffer(0, 0).remaining());
// If non-derived buffer is added, its internal buffer should be returned
ByteBuf concreteBuffer = directBuffer().writeByte(1);
buf.addComponent(concreteBuffer);
assertSame(concreteBuffer.internalNioBuffer(0, 1), buf.internalNioBuffer(0, 1));
buf.release();
// In derived cases, the original internal buffer must not be used
buf = compositeBuffer();
concreteBuffer = directBuffer().writeByte(1);
buf.addComponent(concreteBuffer.slice());
assertNotSame(concreteBuffer.internalNioBuffer(0, 1), buf.internalNioBuffer(0, 1));
buf.release();
buf = compositeBuffer();
concreteBuffer = directBuffer().writeByte(1);
buf.addComponent(concreteBuffer.duplicate());
assertNotSame(concreteBuffer.internalNioBuffer(0, 1), buf.internalNioBuffer(0, 1));
buf.release();
}
@Test