QPACK: RFC 9204 static-table compliance, decode robustness, and cleanup#13260
Draft
phongn wants to merge 7 commits into
Draft
QPACK: RFC 9204 static-table compliance, decode robustness, and cleanup#13260phongn wants to merge 7 commits into
phongn wants to merge 7 commits into
Conversation
Replace the unconditional 99-entry linear scan in the static-table name lookup with a binary search over an auxiliary name-sorted index. The static table cannot be reordered (its indices are wire values), so the index is built once and sorted by name. Behavior is unchanged: the lookup returns the sole exact match or the highest-indexed name match, exactly as the linear scan did. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The decoder's Required Insert Count / Delta Base prefix decoding had three defects on malformed input: - _decode_header read an uninitialized `tmp` and used `&&` instead of `||`, so a failed integer decode was usually not caught and `pos` was then advanced by a negative return value. - The Delta Base check compared `< 0xFFFF` (inverted), failing to reject oversized values. - `remain_len` was never updated as `pos` advanced, so per-instruction decoders received an end pointer past the real buffer, allowing an over-read on a crafted length. Decode against a fixed `end` pointer, initialize the prefix integers, reject on `|| value > 0xFFFF`, and recompute the remaining length each instruction. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The QPACK dynamic table is not implemented (entries are never stored), so advertising a non-zero header_table_size or qpack_blocked_streams would make peers insert entries we drop and then reference, breaking decoding. Fail at config load if either is set non-zero instead of silently misbehaving on the wire. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The QPACK static table is normative: its 99 indices (RFC 9204 Appendix A) are wire values shared with every peer and cannot be extended or reordered. apache#12201 added a 100th entry ("content-encoding: zstd") in the middle and appended "zstd" to entry 31's value, shifting RFC indices 42-98 to 43-99 and altering index 31. A peer using the standard table then mis-resolves every static reference at or above 42 (content-type variants, the second :status block, user-agent, etc.), corrupting headers in both directions; it also makes "content-encoding: zstd" itself decode as "br" on a standard client. Restore the table to RFC 9204 exactly. zstd content-coding is unaffected: it is still encoded as a literal field, which is how every compliant QPACK implementation conveys values absent from the static table. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A header block with a non-zero Required Insert Count references the dynamic table. Entries are only stored once a non-zero table capacity is negotiated, and ATS advertises zero, so such a reference can never be satisfied -- the decoder would queue the stream as blocked forever. Fail the decode instead when the table capacity is zero. Adds a test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Decode header blocks that reference static-table indices spanning the range a prior change shifted (31, 42, 52, 71, 98) and assert each yields the exact RFC 9204 Appendix A name and value. This pins the normative table and fails if an entry is ever inserted or reordered. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Member
|
@phongn failure in qpack test: |
The two QPACK tests added on this branch allocated a QPACK and a TestQPACKEventHandler with new but never freed them; LeakSanitizer flagged the leaks in CI. Delete both at the end of each test. The conformance test schedules an async decode-completion event that references the handler, so wait for it to run before tearing the handler down; the hardening test fails before scheduling anything and frees immediately. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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, sharedsrc/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 appendedzstdto 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:statusblock,user-agent,x-frame-options, …) in both directions, andcontent-encoding: zstditself decodes asbron 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; andremain_lenwas 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 fixedend, 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 (
XpackDynamicTablenever stores entries — capacity is fixed at zero and never raised). Advertising a non-zeroheader_table_sizeorqpack_blocked_streamswould 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 > 0orproxy.config.http3.qpack_blocked_streams > 0now fails at config load (both default to 0). These never worked correctly; failing loudly is preferable to advertising a capability we can't honor.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 index42 →
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_EVENT_DECODE_FAILEDhandler and theres < 0path inHttp3HeaderVIOAdaptorare currently// FIXMEno-ops (failures log but don't reset the stream/connection). Worth a separate change.🤖 Generated with Claude Code