Prevent sharing the index of the continuation frame header ByteBuf.

[minwoox]

Motivation:
The current implementation uses the byteBuf for a continuation frame header multiple times if the header length exceeds 3 * maxFrameLength. However, it fails to slice the byteBuf during usage. Reference

Modification:

• Introduce ByteBuf.retainedSlice() for a continuation frame header when it's used to prevent sharing the index.

Result:

• Correctly send continuation frame headers to the remote peer, addressing the issue of reusing the index of the ByteBuf.

[franz1981]

However, it fails to slice the byteBuf during usage

In which occasion it happens? what means "usage" in this context?

[normanmaurer]

@franz1981 I think this fix is correct as you need to either call slice() or duplicate() if you pass the same buffer down the pipeline multiple times as otherwise you might end up with a buffer that had its indexes modified by a previous write operation.

@minwoox great catch! Did you sign our icla yet ? https://netty.io/s/icla

[franz1981]

Yep @normanmaurer I was just curious to know how he found it and how we didn't have coverage for that (including our vertx test suite) - cc @vietj

[franz1981]

Thanks and well done!

[alexc-db]

hi all, would it be possible to cut a Netty release after this PR is merged? thanks!

[Lincong]

Hi @franz1981

I was just curious to know how he found it

This is how we discovered it: line/armeria#5385

[franz1981]

Many thanks for sharing @Lincong !!!

[Lincong]

I think this fix is correct as you need to either call slice() or duplicate() if you pass the same buffer down the pipeline multiple times as otherwise you might end up with a buffer that had its indexes modified by a previous write operation.

I used my debugger to confirm that this is the case.

In AbstractCoalescingBufferQueue, this is the point before sending the first CONTINUATION frame header:

After sending the first CONTINUATION frame header. This is the point before sending the second CONTINUATION frame header. As we can see, there is no more readable bytes in the header:

All readable bytes from the 1st CONTINUATION frame header ByteBuf is fully consumed [here]:(

netty/transport/src/main/java/io/netty/channel/AbstractCoalescingBufferQueue.java

Line 316 in b194741

newCumulation.writeBytes(cumulation).writeBytes(next);

):

After being copied to the cumulation ByteBuf, it contains no readable bytes:

[minwoox]

Did you sign our icla yet ? netty.io/s/icla

Done it. 😉

Thanks @Lincong for sharing the issue. 😉

how we didn't have coverage for that (including our vertx test suite)

It only happens when TLS is used and the header length exceeds 3 * maxFrameLength. That's why it isn't found yet. (Armeria also does not cover this case.)

When TLS is used, the byteBuf is read in the queue of the SslHandler as @Lincong illustrated.

When TLS is not used, the reader and writer indexes of the byteBuf are used directly instead of reading the bytebuf in the ChannelOutboudBuffer so we couldn't find that.