SABITHSAHEB commented on code in PR #13924:
URL: https://github.com/apache/trafficserver/pull/13924#discussion_r4408780463
##########
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:
Good catch. `_generate_header_block()` now checks the result: on failure it
frees the block, generates no HEADERS frame and doesn't retry. The error goes
back through `Http3FrameCollector::on_write_ready()`, whose `Http3ErrorUPtr`
return was always null before, to `Http3Transaction::_process_write_vio()`.
I couldn't fail just the stream. `QUICStream::reset()` and
`QUICStreamManager::reset_stream()` are both empty, so there's no RESET_STREAM
to send. Instead it closes the connection with H3_INTERNAL_ERROR, which RFC
9114 Section 8 allows. I didn't route it through `_handle_error()`: that marks
the transaction done, and on the write side `_delete_if_possible()` would then
free it while the HttpSM still holds it. In practice this only fires when a
single field's encoded bound passes 2 MB, so it's a backstop. If you'd rather
keep it a stream error until RESET_STREAM is wired up, I can switch it.
##########
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:
Handled it here, since this PR is what makes it reachable.
`generate_frame()` now puts the whole block in one HEADERS frame.
`Http3HeadersFrame::to_io_buffer_block()` already sizes its block from the
frame length and falls back to an xmalloc'd block above 32K, so a large header
block is copied in one piece.
It's reachable even at the default 32 KB `response_header_max_size`:
`xpack_encode_string()` always Huffman codes, and obs-text bytes take 20 to 26
bit codes, so the encoded block can pass 64 KB.
##########
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:
Added an `x-oversized-header` field with a 1 KB value ahead of the other 64.
Its bound is 4201 bytes, which doesn't fit the initial 2 KB block, so
`encode()` takes the replace branch. I confirmed that branch runs in the test.
I left the `ret == 0` guards around `ref_entry()` without a test. Because
each field is pre-sized, an encoder can't fail when called through `encode()`,
so reaching them would need a direct `_encode_header()` call on a full block.
That didn't seem worth a test hook.
##########
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:
When `_encode_header()` fails, `encode()` frees the blocks and returns
before `largest_reference`/`smallest_reference` are used for the prefix or
stored in `_references`. A stale value never gets out, so I've left this as is.
##########
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:
This matches how the existing decode tests in this file wait for
`QPACK_EVENT_DECODE_COMPLETE`. Replacing that wait is better done for all of
them in a separate change.
--
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]
