Copilot commented on code in PR #13924:
URL: https://github.com/apache/trafficserver/pull/13924#discussion_r4399686971
##########
src/proxy/http3/QPACK.cc:
##########
@@ -242,10 +253,32 @@ QPACK::encode(uint64_t stream_id, HTTPHdr &header_set,
MIOBuffer *header_block,
uint16_t largest_reference = 0;
uint16_t smallest_reference = 0;
IOBufferBlock *compressed_headers = new_IOBufferBlock();
+ IOBufferBlock *current_block = compressed_headers;
compressed_headers->alloc(BUFFER_SIZE_INDEX_2K);
for (auto &field : header_set) {
- int ret = this->_encode_header(field, base_index,
compressed_headers, referred_index);
+ // The field encoders fail when the field does not fit in the space left
in the block. Chain a block that is large
+ // enough for the whole field before encoding it, so that no field is left
out of the header block.
+ int64_t required = max_encoded_field_size(field);
+ if (required > current_block->write_avail()) {
+ if (required > DEFAULT_MAX_BUFFER_SIZE) {
+ compressed_headers->free();
+ return -1;
+ }
+ IOBufferBlock *next_block = new_IOBufferBlock();
+ next_block->alloc(std::max<int64_t>(BUFFER_SIZE_INDEX_2K,
iobuffer_size_to_index(required, MAX_BUFFER_SIZE_INDEX)));
+ if (current_block->read_avail() == 0) {
+ // Nothing has been written to the current block, so replace it rather
than leaving an empty block in the chain
+ ink_assert(current_block == compressed_headers);
+ compressed_headers->free();
+ compressed_headers = next_block;
+ } else {
+ current_block->next = next_block;
+ }
+ current_block = next_block;
+ }
+
+ int ret = this->_encode_header(field, base_index,
current_block, referred_index);
largest_reference = std::max(largest_reference, referred_index);
smallest_reference = std::min(smallest_reference, referred_index);
if (ret < 0) {
Review Comment:
`referred_index` is used to update `largest_reference`/`smallest_reference`
before checking whether `_encode_header()` succeeded. If `_encode_header()`
returns an error, `referred_index` may be stale from a prior iteration (or
otherwise not updated), which can corrupt the encoder stream prefix values. Fix
by only updating reference tracking after confirming `ret == 0`, or by
resetting `referred_index` to a known safe value (e.g., 0) before calling
`_encode_header()` and updating only on success.
##########
src/proxy/http3/test/test_QPACK.cc:
##########
@@ -479,6 +479,64 @@ TEST_CASE("decode() rejects oversized Required Insert
Count at entry", "[qpack-d
hdr.destroy();
}
+// QPACK::encode() compresses a header set into IOBufferBlocks that are then
+// framed as a HEADERS frame. A response whose compressed fields do not fit in
a
+// single 2 KB block must still be encoded in full; a field that is silently
+// dropped never reaches the client. Encode a large header set, decode the
+// resulting header block with a second QPACK instance and check that every
+// field survived the round trip.
+TEST_CASE("encode() keeps every field of a header set larger than one buffer
block", "[qpack-encode-large]")
+{
+ QUICApplicationDriver driver;
+ auto encoder =
std::make_unique<QPACK>(driver.get_connection(), UINT32_MAX, 0, 0,
MAX_FIELD_SIZE);
+ auto decoder =
std::make_unique<QPACK>(driver.get_connection(), UINT32_MAX, 0, 0,
MAX_FIELD_SIZE);
+ auto handler = std::make_unique<TestQPACKEventHandler>();
+
+ constexpr int NUM_FIELDS = 64;
+ constexpr size_t VALUE_LENGTH = 256;
+
+ HTTPHdr original;
+ original.create(HTTPType::RESPONSE);
+ for (int i = 0; i < NUM_FIELDS; ++i) {
+ char name[32];
+ snprintf(name, sizeof(name), "x-large-header-%02d", i);
+ std::string value(VALUE_LENGTH, static_cast<char>('a' + (i % 26)));
+ MIMEField *field = original.field_create(std::string_view{name});
+ original.field_attach(field);
+ original.field_value_set(field, value);
+ }
+
+ MIOBuffer *header_block = new_MIOBuffer(BUFFER_SIZE_INDEX_32K);
+ IOBufferReader *header_block_reader = header_block->alloc_reader();
+ uint64_t header_block_len = 0;
+
+ REQUIRE(encoder->encode(1, original, header_block, header_block_len) == 0);
+ REQUIRE(header_block_len ==
static_cast<uint64_t>(header_block_reader->read_avail()));
+
+ std::vector<uint8_t> block(header_block_len);
+ header_block_reader->memcpy(block.data(), block.size());
+
+ HTTPHdr decoded;
+ decoded.create(HTTPType::RESPONSE);
+ REQUIRE(decoder->decode(1, block.data(), block.size(), decoded,
handler.get(), eventProcessor.all_ethreads[0]) == 0);
+ sleep(1);
+ REQUIRE(handler->last_event() == QPACK_EVENT_DECODE_COMPLETE);
Review Comment:
Using `sleep(1)` to wait for async decode completion is timing-dependent and
can make the test flaky (slow CI hosts may need longer; fast hosts waste time).
Prefer a deterministic wait mechanism (e.g., a condition variable/latch in
`TestQPACKEventHandler`, polling with a bounded timeout + short yields, or
driving the event loop until `QPACK_EVENT_DECODE_COMPLETE` is observed).
--
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]
