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]
