Prevent sharing the index of the continuation frame header ByteBuf. (#…

…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.
netty · Jan 18, 2024 · d9ca50d · d9ca50d
1 parent 0e7c27c
commit d9ca50d
Show file tree

Hide file tree

Showing 2 changed files with 39 additions and 12 deletions.
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2FrameWriter.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2FrameWriter.java
@@ -567,7 +567,7 @@ private ChannelFuture writeContinuationFrames(ChannelHandlerContext ctx, int str
                 ByteBuf fragment = headerBlock.readRetainedSlice(fragmentReadableBytes);
 
                 if (headerBlock.isReadable()) {
-                    ctx.write(buf.retain(), promiseAggregator.newPromise());
+                    ctx.write(buf.retainedSlice(), promiseAggregator.newPromise());
                 } else {
                     // The frame header is different for the last frame, so re-allocate and release the old buffer
                     flags = flags.endOfHeaders(true);

diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2FrameWriterTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2FrameWriterTest.java
@@ -189,7 +189,7 @@ public void writeLargeHeaders() throws Exception {
         int streamId = 1;
         Http2Headers headers = new DefaultHttp2Headers()
                 .method("GET").path("/").authority("foo.com").scheme("https");
-        headers = dummyHeaders(headers, 20);
+        headers = dummyHeaders(headers, 60);
 
         http2HeadersEncoder.configuration().maxHeaderListSize(Integer.MAX_VALUE);
         frameWriter.headersConfiguration().maxHeaderListSize(Integer.MAX_VALUE);
@@ -198,7 +198,7 @@ public void writeLargeHeaders() throws Exception {
 
         byte[] expectedPayload = headerPayload(streamId, headers);
 
-        // First frame: HEADER(length=0x4000, flags=0x01)
+        // First frame: HEADER(length=0x4000, type=0x01, flags=0x01)
         assertEquals(Http2CodecUtil.MAX_FRAME_SIZE_LOWER_BOUND,
                      outbound.readUnsignedMedium());
         assertEquals(0x01, outbound.readByte());
@@ -207,22 +207,49 @@ public void writeLargeHeaders() throws Exception {
 
         byte[] firstPayload = new byte[Http2CodecUtil.MAX_FRAME_SIZE_LOWER_BOUND];
         outbound.readBytes(firstPayload);
+        int index = 0;
+        assertArrayEquals(Arrays.copyOfRange(expectedPayload, index, index + firstPayload.length),
+                          firstPayload);
+        index += firstPayload.length;
 
-        int remainPayloadLength = expectedPayload.length - Http2CodecUtil.MAX_FRAME_SIZE_LOWER_BOUND;
-        // Second frame: CONTINUATION(length=remainPayloadLength, flags=0x04)
+        // Second frame: HEADER(length=0x4000, type=0x09, flags=0x00)
+        assertEquals(Http2CodecUtil.MAX_FRAME_SIZE_LOWER_BOUND,
+                     outbound.readUnsignedMedium());
+        assertEquals(0x09, outbound.readByte());
+        assertEquals(0x00, outbound.readByte());
+        assertEquals(streamId, outbound.readInt());
+
+        byte[] secondPayload = new byte[Http2CodecUtil.MAX_FRAME_SIZE_LOWER_BOUND];
+        outbound.readBytes(secondPayload);
+        assertArrayEquals(Arrays.copyOfRange(expectedPayload, index, index + secondPayload.length),
+                          secondPayload);
+        index += secondPayload.length;
+
+        // third frame: HEADER(length=0x4000, type=0x09, flags=0x00)
+        assertEquals(Http2CodecUtil.MAX_FRAME_SIZE_LOWER_BOUND,
+                     outbound.readUnsignedMedium());
+        assertEquals(0x09, outbound.readByte());
+        assertEquals(0x00, outbound.readByte());
+        assertEquals(streamId, outbound.readInt());
+
+        byte[] thirdPayload = new byte[Http2CodecUtil.MAX_FRAME_SIZE_LOWER_BOUND];
+        outbound.readBytes(thirdPayload);
+        assertArrayEquals(Arrays.copyOfRange(expectedPayload, index, index + thirdPayload.length),
+                          thirdPayload);
+        index += thirdPayload.length;
+
+        int remainPayloadLength = expectedPayload.length - index;
+        // Second frame: CONTINUATION(length=remainPayloadLength, type=0x09, flags=0x04)
         assertEquals(remainPayloadLength, outbound.readUnsignedMedium());
         assertEquals(0x09, outbound.readByte());
         assertEquals(0x04, outbound.readByte());
         assertEquals(streamId, outbound.readInt());
 
-        byte[] secondPayload = new byte[remainPayloadLength];
-        outbound.readBytes(secondPayload);
+        byte[] fourthPayload = new byte[remainPayloadLength];
+        outbound.readBytes(fourthPayload);
 
-        assertArrayEquals(Arrays.copyOfRange(expectedPayload, 0, firstPayload.length),
-                          firstPayload);
-        assertArrayEquals(Arrays.copyOfRange(expectedPayload, firstPayload.length,
-                                             expectedPayload.length),
-                          secondPayload);
+        assertArrayEquals(Arrays.copyOfRange(expectedPayload, index, index + fourthPayload.length),
+                          fourthPayload);
     }
 
     @Test