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.

[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