Reject invalid QPACK static indexes

[bneradt]

Validate peer-supplied static table indexes before reading the QPACK
static table. Also honor failed encoder-stream name-reference lookups
before inserting dynamic table entries.

The new encoder stream test is the first to exercise
QPACK::on_stream_open, which allocated a QUICStreamVCAdapter::IOInfo
that nothing owned or freed. Hold it in a per-stream map and erase it in
on_stream_close, the way the other QUICApplication implementations do.

[Copilot]

🟡 Changes recommended

The new static-index validation can still be bypassed via implicit narrowing/truncation of decoded indexes (uint64_t → uint16_t), so invalid peer-supplied indexes may not be reliably rejected.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR strengthens HTTP/3 QPACK robustness by rejecting invalid peer-supplied static table indexes and by treating failed encoder-stream name-reference lookups as fatal before attempting dynamic table insertion.

Changes:

• Add bounds-checking to QPACK static table lookups to prevent out-of-range access.
• Abort decoding when an encoder-stream “Insert With Name Reference” lookup does not resolve to an EXACT match (static or dynamic).
• Add a unit test asserting that decoding fails for an out-of-range static table index.

File summaries

File	Description
src/proxy/http3/QPACK.cc	Adds static-table bounds checking and enforces lookup success for encoder-stream insert-with-name-ref before inserting into the dynamic table.
src/proxy/http3/test/test_QPACK.cc	Adds a decode-failure test case for an out-of-range static-table index.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 3
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[bneradt]

The clang-analyzer job's Clang-Tidy stage failure here is not from this change. The only diagnostic is pre-existing on master:

src/mgmt/config/ConfigRegistry.cc:241:34: error: 'entry' used after it was moved [bugprone-use-after-move,-warnings-as-errors]
src/mgmt/config/ConfigRegistry.cc:220:34: note: move occurred here

That is fixed by #13622. I will rebase this branch once that merges.

[bryancall]

[approve ci clang-analyzer]

[Copilot]

🟡 Changes recommended

There is a concrete error-path handling bug in _read_insert_with_name_ref() that can fall through on decode failure (risking incorrect buffer consumption), and the new polling-based tests introduce a cross-thread data race without atomic/mutex protection.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

src/proxy/http3/QPACK.cc:1542

• The value decode error check can fall through on xpack_decode_string() failure: tmp is uninitialized on error, and if the condition is false the code continues with a negative ret, which can wrap read_len (size_t) and consume the wrong amount of data. Treat any ret < 0 from xpack_decode_string() as a hard failure before updating read_len.

  // Value
  if ((ret = xpack_decode_string(arena, value, tmp, input + read_len, input + input_len, _header_field_max_size, 7)) < 0 &&
      tmp > 0xFF) {
    return -1;
  }

src/proxy/http3/test/test_QPACK.cc:116

• TestQPACKEventHandler::_event is written from an event thread and read from the test thread (via wait_for_event polling). As a plain int this is a data race (UB) and can make the new polling helper flaky under TSAN or on weakly-ordered CPUs. Make _event atomic (or protect access with a mutex) and use load/store in the handler and accessor.

  TestQPACKEventHandler() : Continuation() { SET_HANDLER(&TestQPACKEventHandler::event_handler); }

  int
  event_handler(int event, Event * /* data ATS_UNUSED */)
  {
    this->_event = event;
    return 0;
  }

  int
  last_event()
  {
    return this->_event;
  }

private:
  int _event = 0;

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[bneradt]

The rocky ASan job's LeakSanitizer failure is fixed in the force push just now. Both leaks were reached for the first time by the new encoder stream test:

• QPACK::on_stream_open() allocated a QUICStreamVCAdapter::IOInfo with a bare new that nothing owned, and on_stream_close() was empty, so every stream open leaked 312 bytes. Http3App, Http09App, and quic_client all hold IOInfo by value in a _streams map; QPACK now does the same and erases the entry in on_stream_close(). This also makes teardown safer than the leak did, since ~QUICStreamVCAdapter() cancels the adapter's pending events.
• TestQUICStream leaked the MockQUICConnectionInfoProvider it passed to its base class. It owns that through a unique_ptr now, via a delegating constructor.

Both were pre-existing: nothing called QPACK::on_stream_open() outside the QIF-driven tests, and those are skipped in CI because the qifs data is not present.

Verified with a local ASan build (-DENABLE_ASAN=ON): test_qpack passes with no LeakSanitizer output, and reintroducing either allocation brings the corresponding report back.

[maskit]

Built and ran this against quiche + ASan (test_qpack): compiles clean, all three [qpack-decode] cases pass in 1.4s. I reverted each fix separately to confirm the new tests bite — without the countof check test 1 aborts, without the encoder-stream handling test 2 fails both assertions. STATIC_HEADER_FIELDS also matches RFC 9204 Appendix A entry for entry, so countof is the right bound.

Two blockers inline, plus a note on the new test's threading.

[maskit]

With the index decode now writing index directly, nothing writes tmp before the value guard two lines down:

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.

  if ((ret = xpack_decode_string(arena, value, tmp, input + read_len, input + input_len, _header_field_max_size, 7)) < 0) {
    return -1;
  }

[maskit]

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.

Suggested change

	} 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);
	} 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:

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).

[maskit]

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.