Copilot commented on code in PR #13621:
URL: https://github.com/apache/trafficserver/pull/13621#discussion_r3917700271
##########
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:
On Insert With Name Reference, a failed name lookup now aborts decoding, but
there is no test exercising this encoder-stream failure path (e.g.
dynamic-table name ref that is out of range / not present). Adding a focused
unit test would prevent regressions and ensure the new abort behavior is
observable.
##########
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:
StaticTable::lookup now guards against out-of-range indexes, but the API
takes a uint16_t index. Call sites decode the peer-supplied index into a
uint64_t (e.g. _decode_indexed_header_field() around QPACK.cc:696-717) and then
implicitly narrow it when calling lookup(), which means values > 0xFFFF can
wrap into a small in-range number and bypass the bounds check. Additionally,
_read_insert_with_name_ref() currently assigns a decoded uint64_t to a uint16_t
(QPACK.cc:1526-1531), so the same wraparound risk exists on the encoder stream
path. Consider changing the lookup/index plumbing to use a wider integer type
(uint64_t/size_t) and explicitly reject values >= countof(STATIC_HEADER_FIELDS)
before any narrowing.
##########
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:
The new test adds another unconditional sleep(1), which makes the unit test
suite slower and can still be flaky on slow/loaded systems. Prefer polling for
the event with a bounded timeout so the test completes quickly when the event
arrives promptly and still fails deterministically if it never arrives.
--
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]
