bneradt commented on code in PR #13621:
URL: https://github.com/apache/trafficserver/pull/13621#discussion_r3919631351
##########
src/proxy/http3/QPACK.cc:
##########
@@ -1221,6 +1227,10 @@ QPACK::estimate_header_block_size(const HTTPHdr & /* hdr
ATS_UNUSED */)
const XpackLookupResult
QPACK::StaticTable::lookup(uint16_t index, const char **name, size_t
*name_len, const char **value, size_t *value_len)
{
+ if (index >= countof(STATIC_HEADER_FIELDS)) {
+ return {index, XpackLookupResult::MatchType::NONE};
+ }
+
Review Comment:
Fixed. \`StaticTable::lookup()\` now takes a \`uint64_t\` index, and
\`_read_insert_with_name_ref()\` reports the index as \`uint64_t\`, so the
decoded value reaches the bounds check at full width instead of wrapping into
an in-range slot first. The \`> 0xFFFF\` clamp in
\`_read_insert_with_name_ref()\` existed only to protect the \`uint16_t\`
out-parameter, so it is gone; range validation is the table lookup's job now.
The encoder-stream dynamic branch is bounded to \`uint32_t\` explicitly before
calling into the dynamic table.
The decoder's dynamic-table branch still narrows through
\`_calc_absolute_index_from_relative_index()\`, which is \`uint16_t\`
throughout and wraps on underflow. That is a pre-existing problem in the
relative-index arithmetic rather than the static-table bounds check, so I left
it for separate work.
##########
src/proxy/http3/QPACK.cc:
##########
@@ -1155,7 +1155,13 @@ QPACK::_on_encoder_stream_read_ready(IOBufferReader
&reader)
}
QPACKDebug("Received Insert With Name Ref: is_static=%d, index=%d,
value=%.*s", is_static, index, static_cast<int>(value_len),
value);
- StaticTable::lookup(index, &name, &name_len, &dummy, &dummy_len);
+ auto result = is_static ? StaticTable::lookup(index, &name, &name_len,
&dummy, &dummy_len) :
+ this->_dynamic_table.lookup(index, &name,
&name_len, &dummy, &dummy_len);
+ if (result.match_type != XpackLookupResult::MatchType::EXACT) {
+ this->_arena.str_free(value);
+ this->_abort_decode();
+ return EVENT_DONE;
+ }
Review Comment:
Added "An out-of-range encoder stream name reference invalidates the
decoder", which feeds an Insert With Name Reference for static index 100 over
the encoder stream and asserts the decoder is left invalid. Verified it fails
without the new \`match_type != EXACT\` guard: the process aborts on the entry
built from the out-of-range lookup.
The stream has to be opened and written from an event thread.
\`QUICStreamVCAdapter\` schedules its read-ready event on \`this_ethread()\`
and will not schedule another until that one is handled, so doing either from
the test's own thread leaves the event queued on a thread that never runs an
event loop.
##########
src/proxy/http3/test/test_QPACK.cc:
##########
@@ -405,6 +405,31 @@ test_decode(const char *enc_file, const char *out_file,
int dts, int mbs)
return ret;
}
+TEST_CASE("Decoding out-of-range static table indexes fails", "[qpack-decode]")
+{
+ QUICApplicationDriver driver;
+ QPACK qpack(driver.get_connection(), UINT32_MAX, 0, 0,
MAX_FIELD_SIZE);
+ TestQPACKEventHandler event_handler;
+ HTTPHdr hdr;
+
+ hdr.create(HTTPType::REQUEST);
+
+ const uint8_t header_block[] = {
+ 0x00, // Required Insert Count.
+ 0x00, // Delta Base.
+ 0xff, // Indexed static field with an extended 6-bit index.
+ 0x25, // Index 100.
+ };
+
+ CHECK(qpack.decode(1, header_block, sizeof(header_block), hdr,
&event_handler, eventProcessor.all_ethreads[0]) == 0);
+
+ sleep(1);
+
+ CHECK(event_handler.last_event() == QPACK_EVENT_DECODE_FAILED);
Review Comment:
Replaced with a bounded poll. There is now a \`wait_for_event()\` helper
that polls at 10ms up to a 5s timeout and returns whether the expected event
arrived, so the test finishes as soon as the callback lands and still fails
deterministically if it never does. \`test_qpack\` went from ~1.0s to 0.04s.
I left the pre-existing \`sleep(1)\` in \`test_decode()\` alone since it
waits on a different condition and is outside this 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]
