bneradt commented on code in PR #13621:
URL: https://github.com/apache/trafficserver/pull/13621#discussion_r4075645301
##########
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:
Good catch, and the rebase onto #13655 covers most of it. That guard is now
`... < 0 || tmp > 0xFFFF`, so a failed string decode short-circuits before
`tmp` is read and `_read_insert_with_name_ref()` returns -1, which aborts the
decode instead of spinning on `read_len == 0`.
I also moved the `tmp` declaration down to the value decode and initialized
it, so the index decode no longer leaves an unset variable in scope ahead of
the guard:
```cpp
// Name Index
if ((ret = xpack_decode_integer(index, input, input + input_len, 6)) < 0) {
return -1;
}
read_len += ret;
// Value
uint64_t tmp = 0;
```
I left the `|| tmp > 0xFFFF` half of the value guard as #13655 wrote it
rather than reducing it to a plain `ret < 0`. It is unreachable there per that
commit's reasoning, and the failure rejection you asked for is what the `||`
already provides, so narrowing it further felt like churn on someone else's
change. Happy to take it out if you would rather the two guards in this
function read the same way.
Thanks for the repro — the 364,583 iterations pinning ET_NET 0 is the part I
would not have found from reading.
##########
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:
Applied, including the XPACK prerequisite exactly as you wrote it. The
encoder stream branch is now:
```cpp
} else {
result = this->_dynamic_table.lookup_relative(index, &name,
&name_len, &dummy, &dummy_len);
}
```
and the `uint32_t` bound is gone. `lookup_relative()` takes a `uint64_t`
relative index now, so the encoder stream index reaches it without narrowing,
matching the static lookup.
I added coverage for the wild read in `test_XPACK.cc`, since the guard is
load-bearing on the configuration we ship: the "Zero-size Dynamic Table"
section now does an index-based `lookup_relative(0, ...)` and expects `NONE`,
and the populated section checks `lookup_relative(count(), ...)`. Verified
against an ASan build that the empty-table case is what the guard prevents —
with the old body restored it is a SEGV in `XpackDynamicTable::lookup_relative`
rather than a silent bad read:
```
AddressSanitizer: SEGV on unknown address 0x01f5000005f3
SUMMARY: AddressSanitizer: SEGV src/proxy/hdrs/XPACK.cc:337 in
XpackDynamicTable::lookup_relative(...)
```
You are right that the relative resolution itself is not observable end to
end while `HTTP3_DEFAULT_HEADER_TABLE_SIZE` is 0. Distinguishing relative from
absolute by behavior needs a table that has evicted, so that the smallest
absolute index is above zero; I did not build that out here.
##########
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:
Restructured both ways you suggested. `TestQPACKEncoderStreamDriver` now
runs the whole sequence on one event thread: the first call opens the stream
and writes the instruction, then reschedules itself, which lands behind the
read ready event the write queued, and the second call does the decode. Nothing
touches the QPACK object from the test's thread any more, and the decode poll
loop is gone — the test waits on one flag.
On the lifetime point: the continuation is heap-allocated and retires itself
with `delete this` at the end of its last step, so nothing is destroyed
underneath a queued event. What it references lives in a `TestQPACKFixture`
held by `shared_ptr`, one reference in the continuation and one in the test, so
on the timeout path the fixture survives until the queued event runs — the
earlier version would have left the write handler pointing at a destroyed
`QPACK` on the test's stack.
`test_qpack` passes under ASan with no leak or error output, and the suite
runs in 0.046s.
--
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]
