node:http2: remove unsafe from H2FrameParser

[Jarred-Sumner]

What

Same programme as #40055 … #40236. Stacked on #40139 (base branch claude/socket-zero-unsafe; retarget to main once that lands) and carries #40210's two SelfRoot/new_cyclic commits. src/runtime/api/bun/h2_frame_parser.rs 54 → 0.

• Stream: all fields Cell/JsCell, the map holds Rc<Stream>; &mut Stream → &Stream everywhere; queue_frame/flush_queue take one queue borrow and dispatch outside it (nothing is held across a JS re-entry); batch segments are Payload { off, len } resolved against send_data's payload instead of raw Ext { ptr }.
• Parser refs: self_ref: SelfRoot<Self> via RefPtr::new_cyclic; typed slots cork_ref / auto_flush_ref / writeonly_socket_ref, NativeCallbacks::H2(RefPtr) (attach checks has_native_callback() first); Keepalive is ScopedRef + main's native_keepalives counter and release_refs_stranded_by_exit keeps main's semantics exactly (a process.exit() from inside any h2 JS callback still lets finalize bring the count to zero). The hive pool is gone; impl Drop (asserts no attached socket); the constructor is split into constructor + configure with an error path that detaches before the final release (the old path could free a still-attached parser).
• Cork slot: CORKED_H2: Cell<Option<BackRef<H2FrameParser, Root>>> for identity, the owner holds cork_ref and clears the slot before releasing; cork() is one-shot as on main; on_auto_flush goes through HasAutoFlusher and returns false on the fatal/compression paths (no re-entrant unregister from inside the queue run).
• Abort signals: SignalRef holds an AbortListenerRegistration (same AbortSignal::{listen_native, retain} API as fetch: remove unsafe from FetchTasklet and fetch.rs #40202) + the parser RefPtr, dropped inside free_resources (main's order); on_abort holds a tracked keepalive for the dispatch.
• C++: FfiSlice.h shared with WebSocket.h; Bun__h2__materializeHeaders takes two typed slices (bun_jsc::h2_headers::materialize), the per-header bound is a debug ASSERT.

Testing

Debug+ASAN: node-http2.test.js 362/362 (also under BUN_JSC_validateExceptionChecks=1), h2-conformance, continuation, invalid-padding, streams-rehash, syscall-fault, upgrade, late-rst, push-refusal; fetch-http2-client/adversarial/leak; grpc-js server/retry/channel-credentials/call-propagation/server-errors/outlier-detection (test-client's 3 failures identical on baseline); node parallel test-http2-* 255/256 (test-http2-pipe flaky only under 16-way parallel; forget-closed-streams ~90 s on debug for both binaries). Hand driver: 100 concurrent 3 MB streams both directions + trailers, ping/settings, rst both sides, AbortSignal, goaway + destroy with open streams, destroy mid-upload, raw socket destroyed with 20 streams, server closed with 5 sessions, 50 short sessions — exit 0, no ASAN output, 120 created / 120 dropped / 120 finalized; process.exit() from inside stream/response/data/write callbacks leaves no h2 frames in LSAN output (same as baseline). clippy clean.

[robobun]

^{Updated 5:49 PM PT - Sep 6th, 2026}

❌ @Jarred-Sumner, your commit a292a10 has 5 failures in Build #111831 (All Failures):

• test/js/node/dgram/node-dgram.test.js - 1 failing in the parallel batch on 🍎 any x64
• test/js/node/http2/node-http2.test.js - code 1 on 🍎 any aarch64
• test/js/node/test/parallel/test-crypto-dh-leak.js - code 1 on 🐧 13 x64-asan
• test/js/bun/http/serve-pending-promise-abort-leak.test.ts - 1 failing in the parallel batch on 🐧 3.23 x64
• test/cli/install/isolated-install.test.ts - code 1 on 🐧 3.23 x64

---

🧪   To try this PR locally:

bunx bun-pr 40240

That installs a local version of the PR into your bun-40240 executable, so you can run:

bun-40240 --bun

[claude]

d8cc46a addresses both concerns from the last round: cork() is now a bounded two-step (one uncork() that may run JS, then displace_cork() which moves the re-corked owner's bytes to its own write_buffer without running JS — so it always terminates and preserves the cork_ref ⇔ CORKED_H2 invariant), and the test's connected() helper now wires error/close to reject, with phase 3 exercising an uncapped B↔C cycle. No further issues found this pass.

Given the scope — a full &mut Stream→Rc<Stream> / hive-pool→RefPtr lifetime rewrite of H2FrameParser, new SelfRoot/new_cyclic primitives in bun_ptr, new AbortSignal::listen_native API, and a cork-slot protocol that took five rounds to converge — plus the stacked base on #40139, a human look is still warranted.

Checked this round: displace_cork() doesn't re-enter JS (only write_buffer.write + register_auto_flush, which early-returns when registered) and releases cork_ref after auto_flush_ref is in place, so it can't drop the last ref; the invariant's four writers (cork/uncork/displace_cork/release_refs_stranded_by_exit) each leave cork_ref and CORKED_H2 consistent; has_nonnative_backpressure is set for BunSocket::None so displaced bytes aren't overtaken.

Extended reasoning...

Overview

This PR eliminates all 54 unsafe blocks from src/runtime/api/bun/h2_frame_parser.rs by: converting *mut Stream map entries to Rc<Stream> with all fields Cell/JsCell; replacing the hive-pool allocator + manual ref_()/deref() with RefPtr::new_cyclic + a SelfRoot token + typed Cell<Option<RefPtr>> slots (cork_ref/auto_flush_ref/writeonly_socket_ref); adding impl Drop; converting BatchSegment::Ext { ptr } to Payload { off, len }; and moving abort-signal listeners to a new AbortSignal::listen_native + AbortListenerRegistration (RAII unregister). Supporting changes: new bun_ptr::SelfRoot/RefPtr::new_cyclic/DanglingOk, a shared FfiSlice.h header, a typed bun_jsc::h2_headers::materialize wrapper, and a HasAutoFlusher impl in webcore.rs. It's stacked on #40139.

Security risks

None new. The change is a memory-safety refactor of existing behavior; the materialize() wrapper adds a bounds assertion the C++ side previously trusted implicitly (now also ASSERTed per-field). No auth/crypto/permission surfaces touched.

Level of scrutiny

High. This is production-critical (every node:http2 session), rewrites the parser's ownership model end to end, and the cork-slot re-entrancy protocol went through five review iterations (c885faf → a8e3f80 → 713e18d → 6b7b22c → d8cc46a), each fixing a distinct leak/livelock/desync in the previous. d8cc46a's two-step approach (one JS-reentrant uncork(), then a non-reentrant displace_cork()) is bounded and preserves the stated invariant, and the new test's phase 3 now exercises the uncapped B↔C cycle that broke 6b7b22c — but the number of iterations to reach a stable design is itself a signal that a human should read the final shape.

Other factors

The PR is stacked on an unmerged base (#40139) and carries commits from #40210, so the effective diff against main is larger than what's shown here. The new bun_ptr primitives (SelfRoot, new_cyclic, the DanglingOk sealed trait gating BackRef::dangling) are foundation-crate additions that other zero-unsafe PRs in the series will depend on. Test coverage for the cork re-entrancy class is now solid (three phases, whole-frame assertions on every wire, uncapped cross-session pings), and the author reports 362/362 on node-http2 + the h2 conformance/grpc suites under debug+ASAN, but I have not independently verified CI is green on d8cc46a.

[claude]

5bc3307 addresses both open findings from the last round: the retirement block at h2_frame_parser.rs:2908-2917 now guards on !pending_header_compression_error, and phase (4) asserts h2AutoFlushRegistered directly so deleting the retirement block breaks it. Beyond the inline nit, nothing new surfaced this pass.

Extended reasoning...

The two unresolved inline comments from the previous review pass (the 🔴 on the retirement block missing the pending_header_compression_error guard, and the 🟡 on phase (4) not exercising the retirement block) are both addressed by the tip commit 5bc3307: the retirement condition now includes && !self.pending_header_compression_error.get(), and the test fixture asserts h2AutoFlushRegistered(nativeOf(c)) before and after the wait via the new bun:internal-for-testing hook. This run's only new finding is a test-quality nit on phase (4)'s wait shape. Not approving — this is a ~1800-line refactor of refcounting, cork-slot re-entrancy, and abort-listener lifetime in node:http2, stacked on #40139, and has surfaced real bugs across six review rounds; a human pass is warranted.

[claude]

I reviewed this PR at 0c8ec0d and found no bugs. Given the scope — a ~1000-line refactor of h2_frame_parser.rs touching refcounting, cork-slot handover, stream lifecycle, and new SelfRoot/new_cyclic/AbortListenerRegistration abstractions in bun_ptr — a human look is still warranted before merge.

What was reviewed:

• The retirement block at on_auto_flush's tail now guards on both pending_header_compression_error and transport_write_fatal (5bc3307, 0c8ec0d), matching unregister_auto_flush's invariant.
• cork()'s two-step displacement (displace_cork) preserves wire ordering without running JS; the phase-4 test's h2AutoFlushRegistered poll now fails with the retirement block deleted.
• release_refs_stranded_by_exit's stranded-keepalive loop holds a scoped ref so it cannot free self mid-loop; Drop asserts no attached socket.
• BatchSegment::Payload { off, len } replaces the raw Ext { ptr } — payload slices are resolved against the caller's &[u8] at flush time, no lifetime erasure.

Extended reasoning...

Overview

This PR eliminates all 54 unsafe blocks from src/runtime/api/bun/h2_frame_parser.rs by: (1) making Stream fully interior-mutable and Rc-shared instead of raw *mut Stream map entries; (2) replacing hand-rolled ref/deref pairs with typed RefPtr slots (cork_ref, auto_flush_ref, writeonly_socket_ref) plus a SelfRoot/RefPtr::new_cyclic self-pointer; (3) reworking the thread-local cork slot to a typed BackRef<H2FrameParser, Root> with a two-step cork() that displaces re-entrant foreign owners without running JS; (4) introducing AbortListenerRegistration/NativeAbortListener in bun_jsc::abort_signal; (5) removing the HiveArray pool in favor of plain RefPtr allocation and impl Drop; (6) replacing raw batch-segment pointers with payload offsets. Supporting changes touch bun_ptr (SelfRoot, new_cyclic, OwnedThis, DanglingOk), the FFI slice header, H2HeadersMaterializer.cpp, and add a 220-line multi-phase re-entrancy test plus a bun:internal-for-testing hook.

Security risks

None identified. The change is a memory-safety refactor of an existing subsystem; no new user-facing surface, no auth/crypto/permissions. The HPACK header materializer's per-header bounds check moved from a runtime check to a debug ASSERT, but the Rust wrapper (h2_headers::materialize) now asserts described <= packed.len() before the FFI call, so untrusted-input validation is still on the release path.

Level of scrutiny

High. h2_frame_parser.rs is production-critical native code where refcount imbalances, re-entrancy under user JS callbacks, and cork-slot ownership races have direct UAF/leak consequences. Five prior review rounds on this PR each found a real re-entrancy or lifecycle bug (double-cork assert, unbounded cork loop, displaced auto-flush never retiring, retirement dropping a latched compression/transport error), all now addressed. The new bun_ptr primitives (SelfRoot, new_cyclic, OwnedThis) are also load-bearing abstractions carried from #40210 that a maintainer should sign off on.

Other factors

The PR is stacked on #40139 (base claude/socket-zero-unsafe). Test coverage is substantial (362/362 node-http2, grpc-js, node parallel test-http2-*, ASAN clean, hand-driver leak checks per the description), and the new phase-(4) fixture asserts the retirement block via the internal-for-testing hook so deleting the fix breaks a test. All prior inline findings are resolved. Given the size, the number of subtle interactions already found, and the new cross-cutting abstractions, this warrants human review before merge.

[claude]

Re-reviewed after b29af8a and found nothing new — release_refs_stranded_by_exit now bypasses unregister_auto_flush()'s latched-error guard and drops auto_flush_ref directly, which closes the last open thread. Given the scope (~1500 lines of ref-count/lifetime rework in the h2 parser, new SelfRoot/new_cyclic primitives in bun_ptr, and the FfiSlice FFI signature change), a human pass over the final shape is still worthwhile.

What was reviewed

• b29af8a's inline unregister mirrors unregister_auto_flush's effect minus the pending_header_compression_error early return; that function has no other guards, so nothing else is bypassed.
• Re-traced cork()/uncork()/displace_cork() and on_auto_flush's retirement conditions after the 0c1fd13 rebase — the invariants from the earlier rounds still hold.
• FfiSlice<T> layout across FfiSlice.h, bun_jsc::h2_headers, and the WebSocket.{h,cpp} adoption — field order/repr line up on both sides.

Extended reasoning...

Overview

This PR removes all unsafe from src/runtime/api/bun/h2_frame_parser.rs (54 → 0) by replacing raw-pointer self-references, a hand-rolled HiveArrayFallback pool, and manual ref_()/deref() with typed RefPtr/SelfRoot/BackRef handles from bun_ptr. It introduces RefPtr::new_cyclic + SelfRoot<T> in src/ptr/, an RAII AbortListenerRegistration in src/jsc/AbortSignal.rs, moves Bun__h2__materializeHeaders behind a shared Bun::FfiSlice<T> header (also adopted by WebSocket.{h,cpp}), converts Stream to Cell/JsCell fields under Rc<Stream>, replaces BatchSegment::Ext { ptr, len } with an offset+len pair, and threads cork_ref/auto_flush_ref/keepalive_refs as typed self-ref slots. AutoFlusher call sites in webcore.rs, cron.rs, and socket_body.rs are updated to the new back-ref API, and a test hook is added via internal-for-testing.ts. test/js/node/http2/node-http2.test.js gains ~216 lines exercising the cork/displace/auto-flush re-entry chain.

Security risks

The change is entirely internal to the runtime's HTTP/2 frame parser and its ref-count plumbing; no new user-facing surface, auth, crypto, or permissions paths are touched. The FfiSlice change tightens rather than loosens the FFI boundary (typed ptr+len instead of separate scalar args, with a Rust-side length assertion). The security-relevant risk class here is memory safety — use-after-free or ref-count leaks under re-entrancy — which is exactly what the ten prior review rounds iterated on. No injection, data-exposure, or auth-bypass surfaces are in scope.

Level of scrutiny

High. This is a large refactor of lifetime-critical code: manual ref counting, a per-thread cork slot shared across sessions, deferred-microtask auto-flush registration, abort-signal listener lifecycle, and a process.exit() stranded-ref cleanup path. Over the review history I raised ten distinct re-entrancy/ref-leak findings across the cork()/uncork()/displace_cork()/on_auto_flush/release_refs_stranded_by_exit chain, each of which was fixed in a follow-up commit; b29af8a addresses the last one. The number of iterations alone argues for a maintainer read of the final composed shape rather than automated approval — each fix was locally correct, but the interaction surface between the cork slot, the auto-flush registration, latched session errors, and VM-shutdown finalization is dense enough that a human should confirm the invariants documented at the auto_flush_ref/cork_ref fields match what the code now does end-to-end.

Other factors

The PR description reports full debug+ASAN passes on node-http2.test.js (362/362), the h2 conformance/adversarial/leak suites, and the grpc-js suite, plus a hand driver with LSAN-clean exit under process.exit() from inside callbacks. The new test in node-http2.test.js exercises the multi-session cork displacement path and asserts the auto-flush registration retires via the new h2AutoFlushRegistered hook. It is stacked on #40139 and carries #40210's SelfRoot/new_cyclic commits, so a maintainer will also want to confirm the base has landed before retargeting. No third-party CHANGES_REQUESTED reviews are outstanding in the timeline.

[claude]

Code review found no issues

No high-confidence issues detected in this change.

[claude]

Code review found no issues

No high-confidence issues detected in this change.