brbzull0 commented on code in PR #13924:
URL: https://github.com/apache/trafficserver/pull/13924#discussion_r4399701918
##########
src/proxy/http3/QPACK.cc:
##########
@@ -265,7 +298,9 @@ QPACK::encode(uint64_t stream_id, HTTPHdr &header_set,
MIOBuffer *header_block,
header_block_len += header_data_prefix->size();
header_block->append_block(compressed_headers);
- header_block_len += compressed_headers->size();
+ for (IOBufferBlock *block = compressed_headers; block != nullptr; block =
block->next.get()) {
+ header_block_len += block->size();
+ }
Review Comment:
question: `header_block_len` can now be larger than the slice that
`Http3HeaderFramer::generate_frame()` puts in one frame:
https://github.com/apache/trafficserver/blob/a09837e5a0871542f5ef077907f9e9e235d3b44d/src/proxy/http3/Http3HeaderFramer.cc#L68
A larger header block then goes out as several HEADERS frames. RFC 9114,
Section 4.1 sends the header section as a single HEADERS frame, and HTTP/3 has
no CONTINUATION frame, so the peer cannot decode a block split across frames.
The slicing predates this PR, but it could not trigger while the block was
capped at one 2K block. Should it be handled here (one HEADERS frame for the
whole block, or a clean failure past a limit), or in a follow-up issue?
##########
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);
Review Comment:
suggestion: The new test does not reach two of the new branches. Every
field's `max_encoded_field_size()` fits in the initial block, so the branch
that replaces the empty first block (QPACK.cc L270-L274) never runs. With a
table size of 0 the dynamic table is unused, so the `ret == 0` checks around
`ref_entry()` never run either. Adding a first field that is too large for the
initial block on its own would cover the replace branch with the same round
trip.
##########
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;
+ }
Review Comment:
issue: This is the first path on which `encode()` can fail for a real
response, but the only production caller drops the result.
`Http3HeaderFramer::_generate_header_block()` ignores the return value of
`encode()`:
https://github.com/apache/trafficserver/blob/a09837e5a0871542f5ef077907f9e9e235d3b44d/src/proxy/http3/Http3HeaderFramer.cc#L139-L142
When `encode()` returns here, `_header_block` is already allocated and
`_header_block_len` is still 0. So `generate_frame()` builds a HEADERS frame
with an empty payload and marks the header as sent:
https://github.com/apache/trafficserver/blob/a09837e5a0871542f5ef077907f9e9e235d3b44d/src/proxy/http3/Http3HeaderFramer.cc#L67-L79
An empty payload has no Required Insert Count and Base prefix. It is not a
valid encoded field section (RFC 9204, Section 4.5.1), so the peer cannot
decode it. The failure is still swallowed, just one level up. Could
`_generate_header_block()` check the result and fail the stream instead of
producing a frame?
--
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]
