Repository navigation
http2: give HPACK DecodeResult an honest lifetime and copy headers out at the parser boundary - #31842
Jarred-Sumner wants to merge 3 commits into
Conversation
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Re the duplicate flag against #31488: the overlap is real (both remove the dishonest 'static on |
There was a problem hiding this comment.
Thanks for addressing the nit. I didn't find any bugs, but the core change here — lifetime semantics on HPACK DecodeResult and the new owned HeaderValue copy-out path in h2_frame_parser.rs — is in the per-header HTTP/2 hot path and deserves a human look (the ~200 other files are autofix.ci formatting noise).
Extended reasoning...
Overview
The substantive change is confined to three files: src/http/lshpack.rs (replacing the dishonest &'static [u8] on DecodeResult with DecodeResult<'a> tied to the &mut HPACK borrow), src/runtime/api/bun/h2_frame_parser.rs (a new owned HeaderValue struct with a 160-byte inline/heap split that copies name+value out of the thread-local scratch buffer inside the JsCell::with_mut closure, plus mechanical header.name → header.name() updates at every use site), and two new regression tests in test/js/node/http2/node-http2.test.js. The remaining ~200 files are an [autofix.ci] formatting sweep (trailing-newline removal, derive-attribute merging, import reordering) — purely mechanical and unrelated to the fix.
Security risks
HTTP/2 HPACK decoding processes untrusted remote input on every request. The change is strictly safety-increasing (it removes a fake 'static lifetime that allowed slices into a reusable scratch buffer to escape, and now copies bytes into owned storage before any path that could re-enter the parser). I see no new attack surface: the inline-buffer math (name.len() + value.len() vs 160, copy_from_slice bounds) is straightforward and bounds-checked by safe Rust. Still, this is exactly the kind of memory-lifetime change in network-protocol parsing where a second pair of eyes is warranted.
Level of scrutiny
High. This sits on the per-header decode path of the HTTP/2 server and client — both correctness-critical (header corruption) and performance-sensitive. The PR description itself calls for running the h2 headers-throughput bench before/after, and the 160-byte inline threshold and per-header Vec spill are design choices a human should sign off on. There is also explicit coordination with #31488 (which moves the scratch buffer per-instance in c-bindings.cpp); Jarred has stated the sequencing plan in-thread, but that's a maintainer-level decision.
Other factors
My earlier nit (test String.repeat → Buffer.alloc) was addressed in b6a63df. The bug-hunting system found nothing. CI shows a bunx.test.ts failure across all platforms on the autofix commit, which looks unrelated to this change but should be confirmed before merge. Given the combination of hot-path memory-safety logic, a pending performance bench, and cross-PR coordination, this is not a candidate for bot approval.
…t at the parser boundary lshpack::DecodeResult exposed name/value as &'static [u8] even though the slices point into a thread-local scratch buffer that the next decode or encode call (from any HPACK instance on the same thread) overwrites. The fake 'static lifetime let the slices outlive their validity with no compiler enforcement; the HTTP/2 frame parser also returned them out of its JsCell::with_mut closure and held them across error paths (send_go_away dispatches into JS, which can re-enter the parser and run an encode that clobbers the buffer). DecodeResult is now DecodeResult<'a>, borrowing the HPACK instance so a subsequent decode/encode through the same instance is rejected at compile time, and H2FrameParser::decode copies the name/value bytes into an owned HeaderValue (160-byte inline buffer for typical headers, heap spill for larger ones, split at name_len) inside the with_mut closure, so nothing referencing the scratch buffer escapes the cell. The h2_client decode loops in dispatch.rs already use the slices within a single iteration and compile unchanged against the new lifetime. Tested with a raw-socket test asserting a server answers a request carrying :status with GOAWAY PROTOCOL_ERROR (the decode-error-path that re-enters JS mid-block), and a subprocess test that round-trips 40 requests x 25 headers (dynamic-table indexed, literal, and never-index sensitive headers) through a same-thread server+client with forced GC between rounds, asserting every header comes back byte-exact. Both tests are regression guards for the hazard paths, not fix-distinguishing (the pre-fix usage was behaviorally correct; the compiler enforces the fix). Apply step must: cargo check the workspace (dispatch.rs verified by inspection only), run bun bd test http2, run the http2 suite under ASAN, and run the h2 headers-throughput bench before/after.
b6a63df to
2bbcab1
Compare
|
Warning Review limit reached
More reviews will be available in 5 minutes and 59 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
There was a problem hiding this comment.
I didn't find any correctness issues (and thanks for addressing the test nit), but this reworks the per-header copy semantics on the HTTP/2 decode hot path and overlaps with the alternative approach in #31488, so a human should sign off on the design/perf trade-off.
Extended reasoning...
Overview
This PR removes the dishonest &'static [u8] lifetime on lshpack::DecodeResult, replacing it with DecodeResult<'a> borrowed from the HPACK instance, and changes H2FrameParser::decode to copy each decoded header's name/value bytes into a new owned HeaderValue struct (160-byte inline buffer with heap spill) inside the JsCell::with_mut closure. All downstream uses in h2_frame_parser.rs are updated from field access (header.name) to method calls (header.name()). Two new regression tests are added to node-http2.test.js: a raw-socket test for the :status-in-request → GOAWAY path, and a 40×25-header round-trip test with interleaved encode/decode and forced GC.
Security risks
None introduced. The change is strictly safety-hardening: it replaces a fake 'static lifetime (which permitted use-after-overwrite of a thread-local scratch buffer) with compiler-enforced borrowing plus an eager copy. The new HeaderValue::new correctly bounds-checks via copy_from_slice and the inline/heap split at name_len is sound. No new untrusted-input parsing paths are added.
Level of scrutiny
This warrants human review. It is not a mechanical change:
- It sits on the per-header hot path of the HTTP/2 frame parser — every decoded header now incurs an extra copy (and a
Vecallocation when name+value > 160 bytes). The PR description calls for an h2 headers-throughput benchmark before/after, but no numbers are posted. - It introduces a new data structure (
HeaderValue/HeaderValueData) with a tunable inline threshold — a design decision worth a second pair of eyes. - It touches FFI lifetime semantics around a thread-local C buffer.
- There is an acknowledged overlapping PR (#31488) that takes a different approach (per-instance scratch buffer in c-bindings.cpp); the author has stated this PR proceeds first, but a maintainer should confirm that sequencing.
Other factors
The one nit I previously raised (.repeat() → Buffer.alloc().toString() in the test) has been addressed and the thread is resolved. No bugs were found by the bug-hunting system on this revision. CI shows musl LTO link failures and a bunx.test.ts failure that appear unrelated to this change, but the build is not green. Given the hot-path performance implications and the competing design in #31488, deferring to a human is the right call.
|
Thank you for this PR.
What I ran on
The round-trip test is the only part that I left this PR open. The author or a maintainer decides whether to close it. |
lshpack::DecodeResult exposed name/value as &'static [u8] even though the slices point into a thread-local scratch buffer that the next decode or encode call (from any HPACK instance on the same thread) overwrites. The fake 'static lifetime let the slices outlive their validity with no compiler enforcement; the HTTP/2 frame parser also returned them out of its JsCell::with_mut closure and held them across error paths (send_go_away dispatches into JS, which can re-enter the parser and run an encode that clobbers the buffer). DecodeResult is now DecodeResult<'a>, borrowing the HPACK instance so a subsequent decode/encode through the same instance is rejected at compile time, and H2FrameParser::decode copies the name/value bytes into an owned HeaderValue (160-byte inline buffer for typical headers, heap spill for larger ones, split at name_len) inside the with_mut closure, so nothing referencing the scratch buffer escapes the cell. The h2_client decode loops in dispatch.rs already use the slices within a single iteration and compile unchanged against the new lifetime. Tested with a raw-socket test asserting a server answers a request carrying :status with GOAWAY PROTOCOL_ERROR (the decode-error-path that re-enters JS mid-block), and a subprocess test that round-trips 40 requests x 25 headers (dynamic-table indexed, literal, and never-index sensitive headers) through a same-thread server+client with forced GC between rounds, asserting every header comes back byte-exact. Both tests are regression guards for the hazard paths, not fix-distinguishing (the pre-fix usage was behaviorally correct; the compiler enforces the fix). Apply step must: cargo check the workspace (dispatch.rs verified by inspection only), run bun bd test http2, run the http2 suite under ASAN, and run the h2 headers-throughput bench before/after.
Verification
Implemented and verified on a unified integration branch: full debug build (linux-x64, ASAN), cargo check across the workspace, and the affected test files run against the debug build (failures cross-checked against main's build to exclude pre-existing issues). Each change was reviewed twice (compile/API correctness and GC/concurrency/semantics lenses) with findings repaired before landing.