Skip to content

track remaining length while decoding qpack header block - #13361

Merged
JosiahWI merged 1 commit into
apache:masterfrom
dxbjavid:qpack-header-block-remain-len
Jul 7, 2026
Merged

track remaining length while decoding qpack header block#13361
JosiahWI merged 1 commit into
apache:masterfrom
dxbjavid:qpack-header-block-remain-len

Conversation

@dxbjavid

@dxbjavid dxbjavid commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

The QPACK header decoder takes a header block off an HTTP/3 request stream and walks it field by field. In _decode_header the remaining-length counter is set once to the full block size and then never reduced as the cursor moves forward, so every per-field decoder is handed a buffer end of pos plus the whole block length rather than pos plus what is actually left. After the first field is consumed that end sits past the real end of the buffer, by however many bytes have already been read. A client can lean on this by sending a block whose later field declares a string length that reaches beyond the true end but still fits inside the inflated bound, which slips past the length check and makes the following copy read off the end of the allocation. I noticed it while tracing why the end pointer handed to the sub-decoders did not match the block the frame layer had buffered, and an address-sanitiser run on a crafted block confirmed the over-read. The fix keeps remain_len in step with the cursor by subtracting each consumed length, so the derived end always points at the real end of the block. I kept the change inside _decode_header so the sub-decoders and the rest of the path are left alone.

@JosiahWI JosiahWI added the QUIC label Jul 6, 2026
@JosiahWI JosiahWI added this to the 11.0.0 milestone Jul 6, 2026
@masaori335

Copy link
Copy Markdown
Contributor

[approve ci]

@JosiahWI JosiahWI added HTTP/3 and removed QUIC labels Jul 6, 2026
@JosiahWI

JosiahWI commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

The AuTests proxy_serve_stale_dns and subcookie spuriously failed.

@dxbjavid

dxbjavid commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

yeah, those two look unrelated to this change. proxy_serve_stale_dns sits in the cache/dns path and subcookie is a cookie_remap plugin test, neither goes anywhere near the http/3 decode, so I'm fairly sure it's just flake. happy to have ci re-run if that clears it.

if it'd help the fix land, I can add a small decode regression test that feeds a crafted header block through the public decode api and checks it's rejected rather than over-reading past the block. let me know if you'd want that alongside this.

@JosiahWI

JosiahWI commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

It's awesome to have more tests, but it's up to you whether you think the test is worth it. I'm happy to approve either way.

We can rerun CI once it checks off the Rocky build. I think if you reapprove individual CI jobs with our [approve ci ...] command, it stalls pending jobs.

@dxbjavid

dxbjavid commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

Thanks! Sounds good. I'll wait for the Rocky build to finish and then we can rerun CI if needed.

@JosiahWI

JosiahWI commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

The Rocky job ran into a network issue and couldn't publish its success. I'll see if we can restart it now.

@JosiahWI
JosiahWI merged commit 082e875 into apache:master Jul 7, 2026
15 checks passed
@dxbjavid

dxbjavid commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Thank you for the review and merge. Appreciated.

cmcfarlen pushed a commit to cmcfarlen/trafficserver that referenced this pull request Jul 29, 2026
cmcfarlen pushed a commit to cmcfarlen/trafficserver that referenced this pull request Jul 29, 2026
@cmcfarlen cmcfarlen moved this to Picked v10.2.0 in ATS v10.2.x Jul 29, 2026
@github-project-automation github-project-automation Bot moved this from Picked v10.2.0 to For v10.2.0 in ATS v10.2.x Jul 29, 2026
@cmcfarlen cmcfarlen modified the milestones: 11.0.0, 10.2.0 Jul 29, 2026
@cmcfarlen cmcfarlen moved this from For v10.2.0 to Picked v10.2.0 in ATS v10.2.x Jul 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: Picked v10.2.0

Development

Successfully merging this pull request may close these issues.

4 participants