phongn opened a new pull request, #13260:
URL: https://github.com/apache/trafficserver/pull/13260
# QPACK: RFC 9204 static-table compliance, decode robustness, and cleanup
## Summary
A set of correctness and cleanup changes to the home-grown HTTP/3 QPACK
implementation (`src/proxy/http3/QPACK.cc`, shared `src/proxy/hdrs/XPACK.cc`).
The headline is an **RFC 9204 static-table compliance fix** (an interop bug);
the rest hardens the decoder against malformed input, refuses a configuration
we don't actually implement, and speeds up the static-table lookup. Each is a
separate commit so they can be reviewed — or dropped — independently.
Let me know if this is too much for one PR, and it can be split up.
## What's in here
**1. Restore the RFC 9204 static table (drop zstd entries) — please review
closely.**
The QPACK static table (RFC 9204 Appendix A) is normative: its 99 indices
are wire values shared with every peer and must not be reordered or extended.
#12201 ("Add Zstandard compression support") inserted a 100th entry
(`content-encoding: zstd`) into the middle of the table and appended `zstd` to
entry 31's value. The insertion shifts RFC indices 42–98 to 43–99, so a
standard peer mis-resolves every static reference at or above 42 (content-type
variants, the second `:status` block, `user-agent`, `x-frame-options`, …) in
**both** directions, and `content-encoding: zstd` itself decodes as `br` on a
compliant client. This restores the table to RFC 9204 exactly. **zstd content
coding is unaffected** — it is still conveyed as a literal field, which is how
every compliant QPACK implementation encodes values absent from the static
table. (This reverts the static-table portion of #12201; cc: @JakeChampion)
**2. Add an RFC 9204 static-table conformance test.**
Decodes header blocks referencing static indices spanning the shifted range
(31, 42, 52, 71, 98) and asserts each yields the exact RFC name and value.
Fails if the table is ever reordered or extended again. (Kept as a separate
commit from the fix above so the two can be dropped together if needed.)
**3. Fix header-prefix decode bounds and validation.**
The Required Insert Count / Delta Base prefix decoding had three defects on
malformed input: it read an uninitialized value and used `&&` instead of `||`
(so a failed integer decode usually wasn't caught, then advanced the cursor by
a negative return); the Delta Base bound used an inverted comparison; and
`remain_len` was never updated as the cursor advanced, so per-instruction
decoders saw an end pointer past the real buffer (a potential over-read on a
crafted length). Now decodes against a fixed `end`, initializes the prefix
integers, rejects on `|| value > 0xFFFF`, and recomputes the remaining length
per instruction.
**4. Reject dynamic references when no dynamic table exists.**
A header block with a non-zero Required Insert Count references the dynamic
table. ATS only stores dynamic entries once a non-zero table capacity is
negotiated (and currently always advertises zero), so such a reference can
never be satisfied — the decoder would queue the stream as blocked forever.
Fail the decode when the table capacity is zero instead. Includes a test.
**5. Refuse unimplemented QPACK dynamic-table configuration.**
The dynamic table is not implemented (`XpackDynamicTable` never stores
entries — capacity is fixed at zero and never raised). Advertising a non-zero
`header_table_size` or `qpack_blocked_streams` would tell a peer it may insert
entries our decoder silently drops and then reference them, breaking decoding.
Fail at config load if either is set non-zero, rather than silently misbehaving
on the wire.
**6. Speed up the static-table lookup.**
Replace the unconditional 99-entry linear scan in the static-table name
lookup with a binary search over an auxiliary name-sorted index (the table
itself can't be reordered, so the index is built once and sorted by name).
Behavior is unchanged — it returns the sole exact match or the highest-indexed
name match, exactly as the linear scan did.
## Behavior changes worth calling out
- `proxy.config.http3.header_table_size > 0` or
`proxy.config.http3.qpack_blocked_streams > 0` now fails at config load (both
default to 0). These never worked correctly; failing loudly is preferable to
advertising a capability we can't honor.
- A header block referencing the dynamic table is now rejected (decode
error) rather than blocked indefinitely.
- The static table changes the wire representation back to RFC 9204 — i.e.,
it *corrects* the indices ATS emits/accepts for static references ≥ 42.
## Testing
Built and tested against the BoringSSL H3 toolchain
(`-DENABLE_QUICHE=ON -DOPENSSL_ROOT_DIR=.../boringssl
-Dquiche_ROOT=.../quiche`):
- `test_qpack` — 12 assertions / 4 cases pass. The conformance test
confirms, through the real decode path, that index
42 → `content-encoding: br`,
52 → `content-type: text/html; charset=utf-8`,
71 → `:status 500`,
98 → `x-frame-options: sameorigin`.
- `test_http3` — 125 assertions / 14 cases pass.
The new static-table lookup was additionally checked for bit-identical
behavior
against the previous linear scan over an exhaustive set of inputs.
## Out of scope / follow-ups
- QPACK decode-error propagation is incomplete independent of this PR: the
`QPACK_EVENT_DECODE_FAILED` handler and the `res < 0` path in
`Http3HeaderVIOAdaptor` are currently `// FIXME` no-ops (failures log but don't
reset the stream/connection). Worth a separate change.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
