Prevent sharing the index of the continuation frame header ByteBuf. - #13786
Conversation
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](https://github.com/netty/netty/blob/d027ba7320d430743992d613e52596b0182ca854/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2FrameWriter.java#L570) 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.
In which occasion it happens? what means "usage" in this context? |
|
@franz1981 I think this fix is correct as you need to either call @minwoox great catch! Did you sign our icla yet ? https://netty.io/s/icla |
|
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 |
|
hi all, would it be possible to cut a Netty release after this PR is merged? thanks! |
|
Hi @franz1981
This is how we discovered it: line/armeria#5385 |
|
Many thanks for sharing @Lincong !!! |
I used my debugger to confirm that this is the case. In 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]:( 
After being copied to the cumulation ByteBuf, it contains no readable bytes: |
Done it. 😉 Thanks @Lincong for sharing the issue. 😉
It only happens when TLS is used and the header length exceeds 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 |
…13786) 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](https://github.com/netty/netty/blob/d027ba7320d430743992d613e52596b0182ca854/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2FrameWriter.java#L570) 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.
…etty#13786) 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](https://github.com/netty/netty/blob/d027ba7320d430743992d613e52596b0182ca854/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2FrameWriter.java#L570) 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.
6 participants
Motivation:
The current implementation uses the
byteBuffor a continuation frame header multiple times if the header length exceeds3 * maxFrameLength. However, it fails to slice thebyteBufduring usage. ReferenceModification:
ByteBuf.retainedSlice()for a continuation frame header when it's used to prevent sharing the index.Result: