Copilot commented on code in PR #13629:
URL: https://github.com/apache/trafficserver/pull/13629#discussion_r3966533779
##########
src/proxy/http3/test/test_Http3FrameDispatcher.cc:
##########
@@ -385,6 +390,73 @@ TEST_CASE("ignore unknown frames", "[http3]")
}
}
+TEST_CASE("Http3StreamDataVIOAdaptor buffers only the current DATA frame
payload", "[http3]")
+{
+ // The dispatcher hands each handler a reader whose size_limit covers just
+ // one frame. The adaptor must respect that limit, so the body it collects
+ // is the concatenated payloads and nothing else, no matter how the frames
+ // are split across reads.
+ MIOBuffer *sink_buffer = new_MIOBuffer(BUFFER_SIZE_INDEX_512);
+ IOBufferReader *sink_reader = sink_buffer->alloc_reader();
Review Comment:
Both `sink_reader` and `reader` are allocated via `alloc_reader()` but are
never deallocated. In IOBuffer/MIOBuffer patterns, readers often hold
references back to the buffer; freeing the buffer while readers still exist can
prevent buffers from being reclaimed (leak) or trigger debug assertions
depending on build flags. Deallocate readers (e.g.,
`buf->dealloc_reader(reader)` / `sink_buffer->dealloc_reader(sink_reader)`)
before calling `free_MIOBuffer()`.
##########
src/proxy/http3/test/test_Http3FrameDispatcher.cc:
##########
@@ -385,6 +390,73 @@ TEST_CASE("ignore unknown frames", "[http3]")
}
}
+TEST_CASE("Http3StreamDataVIOAdaptor buffers only the current DATA frame
payload", "[http3]")
+{
+ // The dispatcher hands each handler a reader whose size_limit covers just
+ // one frame. The adaptor must respect that limit, so the body it collects
+ // is the concatenated payloads and nothing else, no matter how the frames
+ // are split across reads.
+ MIOBuffer *sink_buffer = new_MIOBuffer(BUFFER_SIZE_INDEX_512);
+ IOBufferReader *sink_reader = sink_buffer->alloc_reader();
+ VIO sink_vio;
+
+ sink_vio.mutex = new_ProxyMutex();
+ sink_vio.set_writer(sink_buffer);
+
+ Http3FrameDispatcher http3FrameDispatcher;
+ Http3StreamDataVIOAdaptor adaptor(&sink_vio);
+ http3FrameDispatcher.add_handler(&adaptor);
+
+ MIOBuffer *buf = new_MIOBuffer(BUFFER_SIZE_INDEX_512);
+ IOBufferReader *reader = buf->alloc_reader();
Review Comment:
Both `sink_reader` and `reader` are allocated via `alloc_reader()` but are
never deallocated. In IOBuffer/MIOBuffer patterns, readers often hold
references back to the buffer; freeing the buffer while readers still exist can
prevent buffers from being reclaimed (leak) or trigger debug assertions
depending on build flags. Deallocate readers (e.g.,
`buf->dealloc_reader(reader)` / `sink_buffer->dealloc_reader(sink_reader)`)
before calling `free_MIOBuffer()`.
##########
src/proxy/http3/test/test_Http3FrameDispatcher.cc:
##########
@@ -385,6 +390,73 @@ TEST_CASE("ignore unknown frames", "[http3]")
}
}
+TEST_CASE("Http3StreamDataVIOAdaptor buffers only the current DATA frame
payload", "[http3]")
+{
+ // The dispatcher hands each handler a reader whose size_limit covers just
+ // one frame. The adaptor must respect that limit, so the body it collects
+ // is the concatenated payloads and nothing else, no matter how the frames
+ // are split across reads.
+ MIOBuffer *sink_buffer = new_MIOBuffer(BUFFER_SIZE_INDEX_512);
+ IOBufferReader *sink_reader = sink_buffer->alloc_reader();
+ VIO sink_vio;
+
+ sink_vio.mutex = new_ProxyMutex();
+ sink_vio.set_writer(sink_buffer);
Review Comment:
The test allocates a `ProxyMutex` via `new_ProxyMutex()` but never releases
it. Please ensure the mutex’s refcount is decremented (e.g.,
`sink_vio.mutex->release()` when done) or use an RAII/refcounted wrapper
consistently used in the codebase to avoid leaks across the test suite.
##########
src/proxy/http3/test/test_Http3FrameDispatcher.cc:
##########
@@ -385,6 +390,73 @@ TEST_CASE("ignore unknown frames", "[http3]")
}
}
+TEST_CASE("Http3StreamDataVIOAdaptor buffers only the current DATA frame
payload", "[http3]")
+{
+ // The dispatcher hands each handler a reader whose size_limit covers just
+ // one frame. The adaptor must respect that limit, so the body it collects
+ // is the concatenated payloads and nothing else, no matter how the frames
+ // are split across reads.
+ MIOBuffer *sink_buffer = new_MIOBuffer(BUFFER_SIZE_INDEX_512);
+ IOBufferReader *sink_reader = sink_buffer->alloc_reader();
+ VIO sink_vio;
+
+ sink_vio.mutex = new_ProxyMutex();
+ sink_vio.set_writer(sink_buffer);
+
+ Http3FrameDispatcher http3FrameDispatcher;
+ Http3StreamDataVIOAdaptor adaptor(&sink_vio);
+ http3FrameDispatcher.add_handler(&adaptor);
+
+ MIOBuffer *buf = new_MIOBuffer(BUFFER_SIZE_INDEX_512);
+ IOBufferReader *reader = buf->alloc_reader();
+ uint64_t nread = 0;
+
+ uint8_t input[] = {// 1st frame (DATA)
+ 0x00, 0x04, 'A', 'A', 'A', 'A',
+ // 2nd frame (DATA)
+ 0x00, 0x04, 'B', 'B', 'B', 'B'};
+
+ constexpr std::string_view expected_body = "AAAABBBB";
+
+ SECTION("Both frames arrive in a single read")
+ {
+ // The first frame's payload is followed in the same buffer by the second
+ // frame, so an unbounded copy would pull the trailing header and payload
+ // into the body as well.
+ buf->write(input, sizeof(input));
+
+ Http3ErrorUPtr error = http3FrameDispatcher.on_read_ready(0,
Http3StreamType::UNKNOWN, *reader, nread);
+ CHECK(!error);
+ CHECK(nread == sizeof(input));
+ }
+
+ SECTION("Frames arrive one byte at a time")
+ {
+ // on_read_ready resets nread on every call, so total it up as we go.
+ uint64_t consumed = 0;
+
+ for (uint8_t *it{input}; it < input + sizeof(input); ++it) {
+ buf->write(it, 1);
+
+ Http3ErrorUPtr error = http3FrameDispatcher.on_read_ready(0,
Http3StreamType::UNKNOWN, *reader, nread);
+ CHECK(!error);
+ consumed += nread;
+ }
+ CHECK(consumed == sizeof(input));
+ }
+
+ adaptor.finalize();
+ REQUIRE(sink_reader->read_avail() ==
static_cast<int64_t>(expected_body.size()));
Review Comment:
The PR description mentions adding `total_data_length()` and using it in
this test to assert on accumulated body length, but in the shown test the
assertion is only against `sink_reader->read_avail()`. If `total_data_length()`
is intended to be part of the regression check, consider asserting on it here
as well (or update the PR description to match the implemented assertions).
##########
src/proxy/http3/test/test_Http3FrameDispatcher.cc:
##########
@@ -385,6 +390,73 @@ TEST_CASE("ignore unknown frames", "[http3]")
}
}
+TEST_CASE("Http3StreamDataVIOAdaptor buffers only the current DATA frame
payload", "[http3]")
+{
+ // The dispatcher hands each handler a reader whose size_limit covers just
+ // one frame. The adaptor must respect that limit, so the body it collects
+ // is the concatenated payloads and nothing else, no matter how the frames
+ // are split across reads.
+ MIOBuffer *sink_buffer = new_MIOBuffer(BUFFER_SIZE_INDEX_512);
+ IOBufferReader *sink_reader = sink_buffer->alloc_reader();
+ VIO sink_vio;
+
+ sink_vio.mutex = new_ProxyMutex();
+ sink_vio.set_writer(sink_buffer);
+
+ Http3FrameDispatcher http3FrameDispatcher;
+ Http3StreamDataVIOAdaptor adaptor(&sink_vio);
+ http3FrameDispatcher.add_handler(&adaptor);
+
+ MIOBuffer *buf = new_MIOBuffer(BUFFER_SIZE_INDEX_512);
+ IOBufferReader *reader = buf->alloc_reader();
+ uint64_t nread = 0;
+
+ uint8_t input[] = {// 1st frame (DATA)
+ 0x00, 0x04, 'A', 'A', 'A', 'A',
+ // 2nd frame (DATA)
+ 0x00, 0x04, 'B', 'B', 'B', 'B'};
+
+ constexpr std::string_view expected_body = "AAAABBBB";
+
+ SECTION("Both frames arrive in a single read")
+ {
+ // The first frame's payload is followed in the same buffer by the second
+ // frame, so an unbounded copy would pull the trailing header and payload
+ // into the body as well.
+ buf->write(input, sizeof(input));
+
+ Http3ErrorUPtr error = http3FrameDispatcher.on_read_ready(0,
Http3StreamType::UNKNOWN, *reader, nread);
+ CHECK(!error);
+ CHECK(nread == sizeof(input));
+ }
+
+ SECTION("Frames arrive one byte at a time")
+ {
+ // on_read_ready resets nread on every call, so total it up as we go.
+ uint64_t consumed = 0;
+
+ for (uint8_t *it{input}; it < input + sizeof(input); ++it) {
+ buf->write(it, 1);
+
+ Http3ErrorUPtr error = http3FrameDispatcher.on_read_ready(0,
Http3StreamType::UNKNOWN, *reader, nread);
+ CHECK(!error);
+ consumed += nread;
+ }
+ CHECK(consumed == sizeof(input));
+ }
+
+ adaptor.finalize();
+ REQUIRE(sink_reader->read_avail() ==
static_cast<int64_t>(expected_body.size()));
+
+ std::array<char, expected_body.size()> body;
+
+ sink_reader->memcpy(body.data(), body.size());
+ CHECK(std::string_view(body.data(), body.size()) == expected_body);
+
Review Comment:
Both `sink_reader` and `reader` are allocated via `alloc_reader()` but are
never deallocated. In IOBuffer/MIOBuffer patterns, readers often hold
references back to the buffer; freeing the buffer while readers still exist can
prevent buffers from being reclaimed (leak) or trigger debug assertions
depending on build flags. Deallocate readers (e.g.,
`buf->dealloc_reader(reader)` / `sink_buffer->dealloc_reader(sink_reader)`)
before calling `free_MIOBuffer()`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
