maskit commented on code in PR #13621:
URL: https://github.com/apache/trafficserver/pull/13621#discussion_r3961788391
##########
src/proxy/http3/QPACK.cc:
##########
@@ -1514,10 +1532,9 @@ QPACK::_read_insert_with_name_ref(IOBufferReader
&reader, bool &is_static, uint1
// Name Index
uint64_t tmp;
Review Comment:
With the index decode now writing `index` directly, nothing writes `tmp`
before the value guard two lines down:
```cpp
if ((ret = xpack_decode_string(arena, value, tmp, ...)) < 0 && tmp > 0xFF) {
```
`xpack_decode_string` leaves `str_length` untouched on every failure return,
so that condition now reads an uninitialized `uint64_t`. The `&&` also needs to
be a plain `ret < 0` — the same fix you applied above.
Verified on master, where `tmp` held the index and the fall-through was
therefore deterministic: encoder-stream bytes `c0 7f 0a` — Insert With Name
Reference, static, index 0, declared value length 137, zero value bytes — make
the string decode fail, the guard not fire, then `read_len += -1` yields
`read_len == 0` and `reader.consume(0)`, so `_on_encoder_stream_read_ready`
re-reads the same instruction forever: 364,583 iterations in 3 seconds pinning
ET_NET 0. The unset `value` pointer also took `Arena::str_free()` to SIGSEGV in
the ASan build.
```cpp
if ((ret = xpack_decode_string(arena, value, tmp, input + read_len, input
+ input_len, _header_field_max_size, 7)) < 0) {
return -1;
}
```
##########
src/proxy/http3/test/test_QPACK.cc:
##########
@@ -405,6 +478,71 @@ 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);
+
+ CHECK(wait_for_event(event_handler, QPACK_EVENT_DECODE_FAILED));
+
+ hdr.destroy();
+}
+
+TEST_CASE("An out-of-range encoder stream name reference invalidates the
decoder", "[qpack-decode]")
+{
+ QUICApplicationDriver driver;
+ QPACK qpack(driver.get_connection(), UINT32_MAX, 1024, 1,
MAX_FIELD_SIZE);
+ TestQUICStream encoder_stream(0);
+ TestQPACKEventHandler event_handler;
+ HTTPHdr hdr;
+
+ hdr.create(HTTPType::REQUEST);
+
+ const uint8_t insert_with_name_ref[] = {
+ 0xff, // Insert With Name Reference, static, with an extended 6-bit index.
+ 0x25, // Index 100, one past the end of the static table.
+ 0x01, // A one byte, unencoded value follows.
+ 'x',
+ };
+ TestQPACKStreamWriter writer(qpack, encoder_stream, insert_with_name_ref,
sizeof(insert_with_name_ref));
+
+ eventProcessor.all_ethreads[0]->schedule_imm(&writer);
+
+ // The rejected insert has to leave the decoder invalid rather than insert an
+ // entry built from an out-of-range lookup. The encoder stream is read on an
+ // event thread, so poll until that has happened.
+ const uint8_t empty_header_block[] = {
+ 0x00, // Required Insert Count.
+ 0x00, // Delta Base.
+ };
+ int ret = 0;
+
+ for (int i = 0; i < 500; ++i) {
Review Comment:
This calls `qpack.decode()` from the Catch2 main thread while ET_NET 0 runs
`_on_encoder_stream_read_ready` on the same object — `_invalid`, `_arena`,
`_dynamic_table` and `_blocked_list` are all touched from both, and QPACK's
`Continuation` mutex is never taken. It passes today but will surface under
TSAN. Having `TestQPACKStreamWriter` do the write and then the decode on the
same thread would remove both the race and the poll loop.
Same test: `writer` is a stack `Continuation` handed to `schedule_imm` and
never cancelled, so on the timeout path it's destroyed while the event may
still be queued.
##########
src/proxy/http3/QPACK.cc:
##########
@@ -1142,20 +1146,30 @@ QPACK::_on_encoder_stream_read_ready(IOBufferReader
&reader)
reader.memcpy(&buf, 1);
if (buf & 0x80) { // Insert With Name Reference
bool is_static;
- uint16_t index;
- const char *name;
- size_t name_len;
- const char *dummy;
- size_t dummy_len;
+ uint64_t index;
+ const char *name = nullptr;
+ size_t name_len = 0;
+ const char *dummy = nullptr;
+ size_t dummy_len = 0;
char *value;
size_t value_len;
if (this->_read_insert_with_name_ref(reader, is_static, index,
this->_arena, &value, value_len) < 0) {
this->_abort_decode();
return EVENT_DONE;
}
- 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);
+ QPACKDebug("Received Insert With Name Ref: is_static=%d, index=%" PRIu64
", value=%.*s", is_static, index,
+ static_cast<int>(value_len), value);
+ XpackLookupResult result;
+ if (is_static) {
+ result = StaticTable::lookup(index, &name, &name_len, &dummy,
&dummy_len);
+ } else if (index <= std::numeric_limits<uint32_t>::max()) {
+ result = this->_dynamic_table.lookup(static_cast<uint32_t>(index),
&name, &name_len, &dummy, &dummy_len);
Review Comment:
When T=0 the Name Index is a **relative** index (RFC 9204 §4.3.2), and on
the encoder stream relative 0 is the most recently inserted entry (§3.2.5).
`XpackDynamicTable::lookup()` takes an absolute index, so this resolves the
wrong entry — or returns `NONE` and kills the connection. `lookup_relative()`
is the matching API; QPACK already does the equivalent conversion on the
field-section path via `_calc_absolute_index_from_relative_index`.
It can't be observed today, since `HTTP3_DEFAULT_HEADER_TABLE_SIZE` is 0 and
the table stays empty — but that's a temporary mitigation, and this would land
as a fresh bug for whoever re-enables it.
The `uint32_t` bound goes away with the fix: it's a C++ type limit standing
in for a protocol rule, and after narrowing it can't reject aliasing anyway.
```suggestion
} else {
result = this->_dynamic_table.lookup_relative(index, &name,
&name_len, &dummy, &dummy_len);
```
One prerequisite in XPACK: `lookup_relative` dereferences
`_entries[_entries_head]` before `lookup`'s `is_empty()` check, and with
capacity 0 the constructor leaves `_entries_head == UINT32_MAX` — so as it
stands that call would be a wild read on the configuration we ship. The
`count()` guard here is load-bearing, not defensive:
```cpp
const XpackLookupResult
XpackDynamicTable::lookup_relative(uint64_t relative_index, const char
**name, size_t *name_len, const char **value,
size_t *value_len) const
{
if (relative_index >= this->count()) {
return {0, XpackLookupResult::MatchType::NONE};
}
return this->lookup(this->largest_index() -
static_cast<uint32_t>(relative_index), name, name_len, value, value_len);
}
```
`count()` returns 0 when empty, so that covers the empty case and
`largest_index()`'s assert can't fire. No behavior change for HPACK, which
already bounds the index with the same quantity at its call site
(`HPACK.cc:344`).
--
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]
