Conversation
Worker VM teardown runs Heap::lastChanceToFinalize, which sweeps the QuicEndpoint / QuicSession / QuicStream wrappers in unspecified order. QuicEndpoint::finalize -> lsquic_engine_destroy drives on_close / on_conn_closed / on_mini_conn_failed into per-stream and per-session context pointers that lsquic still holds; when those wrappers were swept first the ctx is a freed box and the shim's vtable read is a heap-use-after-free (nq_on_stream_close, nq_on_conn_closed, or QuicSession::push_event via on_mini_conn_failed). QuicStream::finalize and QuicSession::finalize now clear lsquic's stream/conn ctx before their box drops (the lsquic object is still live whenever that field is non-null, since on_close/teardown null it first). QuicEndpoint::finalize clears its raw session-pointer registries and nulls the vtable owner so owner-keyed callbacks no-op, and swaps on_stream_close / on_conn_closed for variants that only clear the lsquic back-reference, so a callback that does reach a live box cannot follow its session/endpoint back-pointer into a box the sweep already freed.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review. WalkthroughChangesQUIC finalization safety
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The Worker teardown path now safely detaches QUIC callback state before native release; no actionable merge risk remains. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Reopened and brought up to date with main. The current head is c14090c. Buildkite #117396 passed and every GitHub check is green. The branch merges into main with no conflict, and no commit on main since the merge touches Still reproduces on current main. Release build of fd8422c: Fail-before/pass-after on the merged tree, both directions with the gate's own procedure (
The merge resolved two conflicts. #40516 removed the empty Two review findings landed after the merge. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
The two inline findings are nits (test diagnostics/comment). I'm deferring on the native change itself: this is finalize-order UAF plumbing across the Rust↔lsquic FFI boundary with vtable hot-swapping and raw-pointer registry clearing — REVIEW.md's most-blocked category — and the load-bearing invariant ("non-null raw/conn ⇒ the lsquic struct is still live, so set_ctx(NULL) is safe from finalize") deserves a maintainer's eyes.
Beyond the inline findings, I checked that the swapped vtable is per-endpoint (self.vtable is owned by the endpoint box, still live inside its own finalize), and that the shim's *(us_nq_vtable**)ctx read is covered because already-swept stream/session boxes null their lsquic ctx before dropping so the shim's if (h) guard skips them.
Extended reasoning...
Overview
Fixes a heap-use-after-free at Worker VM teardown when a node:quic endpoint + session + stream are still live. Heap::lastChanceToFinalize sweeps the three wrapper types in unspecified order; QuicEndpoint::finalize runs lsquic_engine_destroy, which fires on_close/on_conn_closed/on_mini_conn_failed back into ctx pointers that may point at boxes the sweep already freed. The fix is three-pronged: (1) QuicStream::finalize / QuicSession::finalize null lsquic's ctx before their box drops; (2) QuicEndpoint::finalize clears its raw *mut QuicSession registries and nulls vtable.owner so owner-keyed callbacks short-circuit; (3) it swaps on_stream_close/on_conn_closed in the vtable for detached variants that only null the back-reference and never follow session/endpoint pointers into possibly-freed boxes. A subprocess fixture spins up six Worker rounds churning QUIC connections and terminate()s mid-flight; robobun confirmed it UAFs 3/3 before and passes 5/5 after under ASAN.
Security risks
None user-facing. This is a crash-safety fix in the teardown path; no input parsing, auth, or trust-boundary changes.
Level of scrutiny
High. This is native memory-safety code in the FFI layer — raw pointers, unsafe extern "C" callbacks, vtable mutation, and reasoning about JSC GC sweep order vs. lsquic's internal destroy sequence. The correctness of set_ctx(NULL) inside finalize rests on the documented invariant that a non-null raw/conn implies the lsquic object hasn't been freed yet (because the normal on_close/on_conn_closed and teardown() paths null it first). That invariant reads correctly to me against node_quic_shim.c and the existing teardown() bodies, but it's exactly the kind of cross-file lifetime contract a maintainer who owns this subsystem should confirm. I also verified the vtable being mutated is per-endpoint (owned by self, still live inside its own finalizer), so the swap doesn't affect other endpoints.
Other factors
The two findings from the bug-hunting pass are both nits on the test (a missing rationale comment on the intentional 200–500ms sleep, and stderr being drained-then-discarded so an ASAN backtrace wouldn't appear in the CI failure diff on regression). Neither affects the fix's correctness. The PR description and in-code comments are unusually thorough about the mechanism, and the ASAN repro is verified both ways — but per the approval guidelines, complex native lifetime changes are not auto-approvable regardless of how well they're written up.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
States the sweep-order invariant once on QuicEndpoint::detach_for_finalize and has the stream/session sites reference it instead of restating.
There was a problem hiding this comment.
No bugs found, but this is a finalize-order UAF fix in native lsquic FFI — the memory-safety category REVIEW.md flags as most-blocked — so it warrants a human look.
What was reviewed:
- Traced all six sweep orderings (endpoint/session/stream first) through the shim's ctx null-checks, the vtable-box lifetime during
release_native, and the "non-null raw/conn ⇒ lsquic object live" invariant the newfinalizebodies rely on. - Confirmed
lsquic_engine_destroy's close path only driveson_close/on_conn_closed(ctx-keyed) plus owner-keyed callbacks — the two swapped stubs plusvt.owner = nullcover them;on_reset/on_hsk_done/etc. don't fire during destroy. - The two earlier nits (surface stderr in the assertion; comment the fixture sleep) are addressed in ddb0441; the
detach_for_finalizedoc comment reads as an invariant contract, not a workaround justification.
Extended reasoning...
Overview
The PR fixes a heap-use-after-free at Worker VM teardown when lastChanceToFinalize sweeps live node:quic wrappers in unspecified order. Three Rust files gain finalize-time detach logic (QuicStream::finalize / QuicSession::finalize null their lsquic ctx; QuicEndpoint::finalize gains detach_for_finalize which clears raw-pointer registries, nulls vtable.owner, and swaps on_stream_close/on_conn_closed for back-pointer-free stubs before lsquic_engine_destroy runs). A new subprocess fixture spins Workers churning QUIC sessions and terminates them mid-flight; ASAN caught the UAF 3/3 before and passes 5/5 after.
Security risks
None introduced. This is a defensive detach on the teardown path; no new user-reachable surface, no input parsing. The failure mode being fixed is itself the security-relevant one (UAF).
Level of scrutiny
High. This is raw-pointer lifetime management across an FFI boundary during GC finalization — exactly the class REVIEW.md calls out as most-blocked. The correctness argument depends on three non-local invariants: (1) the shim null-checks ctx before dereferencing to read the vtable pointer (verified in node_quic_shim.c), (2) the vtable Box lives inside the endpoint and stays alive through release_native because Box<Self> drops only after finalize returns, and (3) a non-null raw/conn always denotes a live lsquic object because every path that frees it (on_close, on_close_detached, teardown) nulls the field first. I traced each and they hold, but a human should confirm — particularly that lsquic_engine_destroy cannot drive any ctx-keyed callback other than the two that were swapped (I checked ietf_full_conn_ci_destroy → lsquic_stream_destroy and believe it cannot, but this is vendored-C reasoning).
Other factors
The two nits from the earlier bug-hunt run (stderr in the assertion object, comment on the fixture sleep) are addressed in the current diff. The comment-cop bot flagged the detach_for_finalize doc comment; the author's response is reasonable — it's the single place stating a cross-file invariant that five SAFETY: sites reference, not a workaround justification. The fixture loops 10 rounds (vs. 6 in the description) with a 120s test timeout, which is on the slow side but justified for an ASAN UAF repro. robobun independently confirmed the test fails without the src/ change and passes with it.
There was a problem hiding this comment.
Both earlier nits are addressed (stderr now surfaces in the failure diff; the fixture sleep is documented). No further issues found, but this is a finalize-order UAF fix across raw lsquic FFI back-pointers — worth a human pass on the invariants before merge.
Traced all six sweep orderings of endpoint/session/stream through detach_for_finalize → lsquic_engine_destroy: each either sees a nulled ctx (already-finalized wrapper cleared it) or reaches a still-live box via the swapped on_*_detached callback that only nulls its own raw/conn and never follows the possibly-freed back-pointer.
Confirmed ctx_ref returns None on null, so vtable.owner = null_mut() short-circuits the owner-keyed callbacks (on_mini_conn_failed, packets_out, on_new_conn/on_new_stream).
Checked that during ordinary (non-lastChanceToFinalize) GC the self-Strong means finalize only runs after teardown() has already nulled raw/conn, so the new set_ctx(null) is a no-op there and cannot touch a freed lsquic object.
Extended reasoning...
Overview
The PR fixes a heap-use-after-free at Worker VM teardown when node:quic wrappers are live. Heap::lastChanceToFinalize sweeps QuicEndpoint / QuicSession / QuicStream in unspecified order; the endpoint's finalizer runs lsquic_engine_destroy, which calls back through per-conn/per-stream ctx pointers that may point at already-freed session/stream boxes. The fix is three-pronged: (1) session/stream finalize null their lsquic ctx before dropping so the shim skips them; (2) endpoint finalize clears its raw *mut QuicSession registries and nulls vtable.owner so owner-keyed callbacks short-circuit in ctx_ref; (3) endpoint swaps on_stream_close/on_conn_closed for detached variants that only null the wrapper's own lsquic handle and never follow the session/endpoint back-pointer into a possibly-freed sibling. An ASAN subprocess test (10 worker rounds, 3 concurrent connect loops churning 1 MiB streams, then terminate()) reproduces 3/3 without the fix and passes 5/5 with it.
Security risks
None introduced. This closes a memory-safety hole (UAF reachable from JS via worker.terminate()); the change adds no new user-facing surface and only narrows what callbacks may touch during teardown.
Level of scrutiny
High. This is squarely in REVIEW.md's most-blocked category — native memory safety, raw pointers across an FFI boundary, GC finalize ordering, and cross-object back-pointers. The correctness argument depends on invariants stated in comments ("a non-null raw/conn is live because on_close/on_conn_closed/teardown null it first") that hold across three Rust files and a C shim. I traced the six sweep permutations and each reads sound, but a maintainer who owns the QUIC subsystem should confirm — in particular that no other lsquic callback beyond the ones handled (stream close, conn closed, mini-conn-failed, packets_out, new_conn/new_stream, SSL_CTX lookup) can fire during lsquic_engine_destroy and reach a wrapper box.
Other factors
My two earlier inline nits were addressed in ddb0441 (stderr in the asserted object via expect.any(String)) and the fixture now carries the required comment on the ≥50 ms sleep. The comment-cop flags were resolved by consolidating the rationale into a single doc comment on detach_for_finalize with one-line SAFETY references at each site, which robobun defended and I agree is the right shape — the doc comment states a non-local invariant, not a workaround. The test follows harness conventions (drains both pipes concurrently, bunEnv, subprocess fixture for the crash repro) and the PR evidence shows it fails on main under ASAN and passes with the fix.
…Subspace lower-tier Hold twelve live sessions+streams so the session/stream wrappers spill past numberOfLowerTierPreciseCells into a MarkedBlock while the two endpoints stay in lower-tier PreciseAllocations; lastChanceToFinalize sweeps MarkedBlocks first, so the endpoint finalizer's lsquic_engine_destroy always sees already-freed ctx pointers without the fix. 8/8 fail-before, 8/8 pass-after, ~10s per run.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/node/quic/quic-endpoint.test.ts`:
- Around line 277-294: Update the worker.terminate test declaration to use
test.concurrent because it runs an independent subprocess. Replace the fixed
30000ms timeout with the established ASAN/debug-scaled timeout, using isASAN and
isDebug from the test harness: 120000ms for either lane and 60000ms otherwise.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ee80c9fc-07bb-40ec-91f7-7d8c186e3116
📒 Files selected for processing (5)
src/runtime/node/quic/endpoint.rssrc/runtime/node/quic/session.rssrc/runtime/node/quic/stream.rstest/js/node/quic/quic-endpoint.test.tstest/js/node/quic/quic-worker-terminate-fixture.ts
There was a problem hiding this comment.
Both earlier nits are addressed (stderr now surfaces in the assertion; the fixture was made deterministic via the IsoSubspace lower-tier trick, so the sleep concern is moot). I didn't find bugs, but this is a finalize-order UAF fix across three wrapper types, an FFI vtable swap, and lsquic's engine-destroy callback path — worth a human pass on the memory-safety reasoning.
What was reviewed:
finalizectx-nulling: verified the "non-null ⇒ live" invariant holds —on_stream_close/on_conn_closedandteardown()each nullraw/connbefore lsquic frees the underlying object, soset_ctx(NULL)at finalize can't touch a freed lsquic struct.- Detached callbacks:
on_close_detached/on_closed_detachedonly write the localraw/conncell and never followsession/endpointback-pointers;ctx_refshort-circuits on the null ctx that already-swept wrappers leave behind. on_stream_reset/on_stream_read/on_stream_writeare not swapped, but engine-destroy drives only the close path (lsquic_stream_destroy→on_close), so they don't fire during the sweep.
Extended reasoning...
Overview
The PR fixes a heap-use-after-free at Worker VM teardown when a node:quic endpoint, session, and stream are all live. Heap::lastChanceToFinalize sweeps the three wrapper types in unspecified order; the previous QuicStream::finalize/QuicSession::finalize were empty, so their boxes dropped while lsquic still held raw ctx pointers to them. When QuicEndpoint::finalize then ran lsquic_engine_destroy, lsquic's force-close path called back through nq_on_stream_close/nq_on_conn_closed into freed memory. The fix has three parts: (1) each wrapper's finalize nulls its lsquic ctx before dropping; (2) QuicEndpoint::detach_for_finalize clears the raw-pointer session registries and nulls vtable.owner so owner-keyed callbacks short-circuit; (3) the vtable's on_stream_close/on_conn_closed are swapped for detached variants that never follow back-pointers into possibly-swept siblings. A subprocess test spawns Workers with 12 live sessions each (pushing wrappers past the 8-cell IsoSubspace lower tier so MarkedBlocks sweep before the endpoint's PreciseAllocation) and terminates them; ASAN debug fails 3/3 without the fix and passes 5/5 with it.
Security risks
None introduced. This is a memory-safety hardening of an existing teardown path; no new user-facing surface, no input parsing, no auth/crypto changes. The change strictly narrows what the lsquic callbacks can reach during VM teardown.
Level of scrutiny
High. This is native FFI lifetime management at the intersection of JSC's finalize-order sweep, lsquic's engine-destroy callback graph, and raw *mut back-pointers between three wrapper types — the exact shape REVIEW.md flags as the most-blocked category. The reasoning is subtle (e.g. the detached callbacks are needed in addition to the per-wrapper ctx-nulling because a not-yet-swept stream's ctx is still valid but its session back-pointer may point at a freed box). The PR description and detach_for_finalize doc comment lay out the invariant clearly, and I verified the "non-null ⇒ live" claim against on_conn_closed (session.rs:2454), on_stream_close (stream.rs), and both teardown() paths — but a maintainer who owns this module should confirm the callback set that lsquic_engine_destroy can fire is fully covered by the two swapped slots plus the owner-null.
Other factors
My two prior inline nits (surface stderr in the assertion; comment the sleep) are resolved — the latter by rewriting the fixture to be deterministic (await an "up" message, then terminate) rather than sleeping. The comment-cop bot flagged the detach_for_finalize doc comment length; the author's response (it's the single place the cross-type invariant is stated, referenced by one-line SAFETY comments elsewhere) is reasonable and the thread is resolved. Fails-without/passes-with evidence under ASAN is included. No outstanding unaddressed reviewer comments.
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-25, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
…rker-finalizer-uaf # Conflicts: # src/runtime/node/quic/session.rs # src/runtime/node/quic/stream.rs
|
The walkthrough above raises no concerns and there are no open review threads. The PR is up to date with main at 971e764, and the same five files are in the diff. CI on the merge commit is running. Nothing to change on this side. |
There was a problem hiding this comment.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
src/runtime/node/quic/ffi.rs— nit: Maintainers auditing the one unsafe deref every node:quic callback goes through now read a liveness argument this PR itself shows is false at VM teardown. src/runtime/node/quic/ffi.rs:13-17 still says a non-null ctx is live because "the owning JS wrapper keeps the allocation alive until after the engine that could call us is destroyed", and that onlyteardownclears the ctx. After this change the load-bearing guarantee is different:QuicSession::finalize/QuicStream::finalizenull the lsquic ctx before the box drops, anddetach_for_finalizenullsvtable.owner. Fix: restate the contract so it names finalize (and the nulled owner) as the reason a non-null ctx/owner is live, since REVIEW.md requires SAFETY contracts to be accurate.Extended reasoning...
ffi.rs:3 calls this comment "the single audited deref for every node:quic callback"; every
lsquic_callback!body and the two new detached callbacks (session.rs:2365, stream.rs:597) cite it as their SAFETY justification. ffi.rs:13-14 says "teardown clears the conn/stream ctx" is the only way a NULL arrives; ffi.rs:15-17 says a non-null ctx is live because the JS wrapper outlives the engine. The PR's own description shows lastChanceToFinalize frees session/stream boxes before QuicEndpoint::finalize runs lsquic_engine_destroy, so the wrapper does not outlive the engine there. The new invariant is enforced at session.rs:2374-2382 and stream.rs:606-612 (finalize nulls the ctx) plus endpoint.rs:2636-2642 (owner nulled, so ctx_ref:: sees NULL for owner-keyed callbacks). None of that is mentioned in the contract; the new SAFETY comments point atQuicEndpoint::detach_for_finalizeinstead, so the audit point and the real reasoning have diverged. A future change that drops the finalize ctx-null (e.g. treating the empty-bodied finalize as dead code, as it was on the base) would…Verification: nit. Triggering condition: any maintainer reading the audited contract that every
lsquic_callback!body and both new detached callbacks rely on. The PR does not touchsrc/runtime/node/quic/ffi.rs; its contract comment still reads (ffi.rs:13-17, identical to base) "The only other value the shim can pass is NULL — teardown clears the conn/stream ctx ... A non-null ctx is a liveT: the… -
🟡
test/js/node/quic/quic-endpoint.test.ts:294— nit: CI maintainers get a new test that needs a 30000ms per-test timeout because the fixture takes ~9.5s under the ASAN debug build. test/CLAUDE.md says "Do not set a timeout on tests" and REVIEW.md says to shrink the workload rather than raise the timeout. Fix: bring the fixture under the default budget so the 30000 argument at quic-endpoint.test.ts:294 can be dropped, e.g. one round instead of two in quic-worker-terminate-fixture.ts:59 and the smallest session count that still leaves the IsoSubspace lower tier, while keeping the UAF repro failing on the unfixed ASAN build.Extended reasoning...
The PR's own evidence lists the new test at 9509.94ms on the ASAN debug run and 185.98ms on release. The root CLAUDE.md budget is roughly 1s per test with debug+ASAN 10-100x slower, and test/CLAUDE.md has a "No timeouts" section saying Bun already has timeouts. The fixture at test/js/node/quic/quic-worker-terminate-fixture.ts:59 runs two Worker rounds, each opening 12 client sessions (24 with the server side) with a 64KiB bidi stream, then terminate(). The test passes
30000as the third argument at test/js/node/quic/quic-endpoint.test.ts:294, which is what keeps the ASAN run from timing out at the default 5000ms. The file already has one such timeout at line 129, so this is precedent, not new practice, but it still adds a second slow serial test to the file. Consequence is a slower ASAN lane and a test whose pass depends on an explicit timeout rather than on fitting the budget; behaviour on the base branch is unaffected. A fixer should shrink rounds or sessions while confirming the unfixed ASAN build still reports heap-use-after-free.Verification: nit. Triggering condition: every CI run of this file on the debug/ASAN lane, where the new test only passes because of the raised per-test timeout. Verified in the diff:
test/js/node/quic/quic-endpoint.test.ts:294ends the new test with}, 30000);, and the file has nosetDefaultTimeout, so this is an explicit per-test timeout raise./home/claude/bun/test/CLAUDE.md:118-120states "### No…
There was a problem hiding this comment.
Code review found no new issues
No new issues were found in this update; 1 finding from earlier reviews is still open above.
Still open from earlier reviews (1):
- 🔴
src/runtime/node/quic/stream.rs:612—Under allocation failure, a user's process now gets a use-after-free write into a freed lsquic stream at GC time, where…
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
QuicStream::create stored the lsquic stream pointer before the ArrayBuffer allocations. If one failed, the wrapper kept a pointer to a stream lsquic still owned with a null ctx, and the finalizer would call set_ctx on it after lsquic freed it. bind_raw already sets raw once the allocations succeed.
|
The open finding on stream.rs was valid: |
|
b1a58e3 trims the comment above the |
There was a problem hiding this comment.
I reviewed the latest push and found no new bugs; the one item still open from the previous review (QuicStream raw set before the fallible allocations) is addressed by leaving raw null until bind_raw. Because the fix relies on GC-sweep-order reasoning through unsafe FFI ctx pointers, a human look would still be worthwhile.
What was reviewed:
- All three finalize orderings (endpoint first, session first, stream first): each path either nulls the lsquic ctx before the box drops or hits a detached callback that only touches the wrapper's own
raw/conn, never a back-pointer. - Owner-keyed vtable callbacks (
on_mini_conn_failed,packets_out, SSL_CTX lookups) go throughctx_ref, so the nulledvtable.ownershort-circuits them;push_eventfrom the non-detached conn callbacks only appends to the session's own vec. - Confirmed the inherent
finalizemethods take precedence over theJsFinalizedefault in the generated thunks, andbind_rawstill setsrawafter the ArrayBuffer allocations succeed. - The fixture awaits only
message/error; a worker that exits without posting would hit the 30s timeout rather than fail fast (minor, not blocking).
Extended reasoning...
Overview
The PR touches three unsafe-heavy files in src/runtime/node/quic/ (endpoint.rs, session.rs, stream.rs) plus a new subprocess test and fixture. It adds QuicEndpoint::detach_for_finalize (clears the raw *mut QuicSession registries, nulls vtable.owner, swaps on_stream_close/on_conn_closed to detached variants), and new QuicSession::finalize / QuicStream::finalize that null lsquic's ctx pointer before the Box drops. QuicStream::create now initializes raw to null and relies on the pre-existing bind_raw call at the end of create to set it after the fallible ArrayBuffer allocations, which closes the earlier finding about an orphaned wrapper pointing at a stream lsquic no longer associates with it.
Security risks
No user-input parsing, auth, or crypto surface is changed. The risk class is memory safety at the lsquic FFI boundary: a wrong liveness invariant here is a use-after-free reachable by any Worker that uses node:quic and terminates. I traced the three sweep orderings: (1) endpoint swept first: lsquic_engine_destroy drives the detached callbacks, which only null the still-live wrapper's own raw/conn, and the later session/stream finalizers see null and no-op; (2) session swept first: its finalizer nulls the conn ctx so the shim's null check skips every conn-keyed callback, and the stream's detached on_close does not follow session; (3) stream swept first: its finalizer nulls the stream ctx, and the shim's if (h) guard skips on_stream_close. Owner-keyed callbacks go through the lsquic_callback! macro's ctx_ref, which returns the default on a null owner. push_event only appends to the session's own events Vec, so a non-detached conn callback on a live session cannot reach a freed stream or endpoint synchronously. There are no Drop impls on these types that could follow back-pointers.
Level of scrutiny
High. This is unsafe Rust whose correctness depends on JSC's lastChanceToFinalize sweep order and on an invariant ("non-null raw/conn means the lsquic object is live") that is enforced only by convention across on_close, on_conn_closed, teardown, and now the two finalizers. The reasoning holds as far as I can trace it, but the codegen finalize thunk resolution (inherent method winning over the JsFinalize trait default) and the vendored lsquic destroy path (which I could not read in this shallow checkout, so I relied on the shim's null guards) are exactly the kind of thing a maintainer familiar with the QUIC subsystem should confirm. The automated approval bar for complex memory-safety changes is not met.
Other factors
The test is a proper spawned fixture that drains both pipes concurrently and asserts a combined {stdout, stderr, exitCode}, addressing the earlier inline comment. The 30s per-test timeout is on the high side for the suite's ~1s guidance, though the PR states a run takes roughly 10s under debug+ASAN and the workload (12 sessions x 2 rounds) is what makes the sweep order deterministic. The fixture's readiness promise only rejects on error, so a worker that dies without posting up would surface as a timeout rather than an immediate failure; this is a diagnostics nit, not a correctness problem. No CODEOWNERS entry covers the changed paths. All github-actions and coderabbit threads were resolved by the author or a non-author, and the author's commits after my prior review plausibly addressed the open finding.
|
c14090c takes the diagnostics note from the last review: the fixture now rejects on the worker The CI failures on b1a58e3 (build 117381) are all on lanes the diff does not touch: six tests that passed on retry and a Windows aarch64 step that failed outside the runner. The QUIC suite passed on every lane that ran it. |
There was a problem hiding this comment.
I reviewed the latest push and didn't find any new bugs; the raw-binding change from the earlier thread is now in place (QuicStream::create leaves raw null until bind_raw), but since this is unsafe lifetime handling at VM teardown a human look at the lsquic-side assumptions would still be worthwhile.
What was reviewed:
- Traced each
lastChanceToFinalizeordering (endpoint first, session first, stream first, session before its own streams) through the new finalizers and theon_close_detached/on_closed_detachedswaps — each path ends with the lsquic ctx nulled before the box it pointed at is freed. - Checked that
conn/raware assigned only on the live-object paths (after the fallible allocations increate, inbind_conn/bind_raw, atconnect) and cleared inteardown/on_close/on_conn_closed, so the finalizers'set_ctx(null)never targets a freed lsquic object. - Confirmed the registries cleared by
detach_for_finalizehold non-owning*mut QuicSession(ProvisionalSessionowns only adcidVec), and that every owner-keyed vtable callback goes throughlsquic_callback!/ctx_ref, so the nulledownerreturns the default;on_new_streamresolves its session via the conn ctx instead, which the session finalizer also nulls. - Not verified from source: lsquic is not vendored in this checkout, so which
stream_ifcallbackslsquic_engine_destroycan drive beyondon_close/on_conn_closed(e.g.on_hsk_donefromci_destroy) rests on the PR's description rather than on the destroy path itself.
Extended reasoning...
Overview
The PR touches three native files under src/runtime/node/quic/ (endpoint.rs, session.rs, stream.rs) plus one test and one fixture. The native change is small (~75 lines): QuicEndpoint::finalize now calls a new detach_for_finalize (clears the four *mut QuicSession registries, nulls vtable.owner, swaps on_stream_close/on_conn_closed to variants that touch only the wrapper's own lsquic handle) before release_native runs lsquic_engine_destroy; QuicSession and QuicStream gain inherent finalize(self: Box<Self>) methods that null lsquic's conn/stream ctx when the handle is still non-null, shadowing the blanket JsFinalize drop; and QuicStream::create now leaves raw null until bind_raw, which resolves the allocation-failure orphan raised in the previous review. The test spawns a fixture that twice builds a Worker with a listening endpoint, 12 client sessions and 12 64 KiB streams, then terminate()s it.
Security risks
No injection, auth, or data-exposure surface is touched. The risk class is memory safety: the change exists to remove a heap-use-after-free at Worker teardown, and a mistake in the invariants would be a different UAF (e.g. lsquic_conn_set_ctx on a conn lsquic already freed). I checked that conn is set only at QuicSession::create (after the fallible ArrayBuffer allocations), bind_conn, and the connect success path, and cleared in teardown (which also nulls the ctx) and on_conn_closed (where the shim has already nulled lsquic's copy); raw is set only in bind_raw and cleared in teardown, on_stream_close, and the new on_close_detached. That makes "non-null handle means lsquic still owns a live object" hold for the finalizers. The shim reads the vtable pointer out of the front of each ctx, so nulling the ctx (rather than the vtable slot) is the correct way to make a late callback a no-op. The registries are non-owning; ProvisionalSession owns only a dcid: Vec<u8> and pending_verneg holds a pointer plus a CID array, so Vec::clear frees nothing lsquic or JSC still references. There are no Drop impls on the three wrappers, so the box drop after finalize cannot re-enter lsquic.
Level of scrutiny
High. This is unsafe Rust across an FFI boundary in a finalizer that runs during VM::~VM, where JS-side roots (Strong, this_value) no longer order destruction. The four teardown orders I traced all end with the relevant ctx nulled before its box is freed, and the detached callbacks deliberately avoid the session/endpoint back-pointers that could be dangling. Two things keep this from being an approve: (1) lsquic itself is not in this checkout, so I could not confirm from ietf_full_conn_ci_destroy/lsquic_stream_destroy that on_close and on_conn_closed are the only stream_if callbacks the destroy path drives — the non-swapped callbacks (on_hsk_done, on_conncloseframe, on_stream_reset, ...) would still be safe against a freed wrapper because its ctx is nulled, but against a live wrapper they still follow the session/endpoint back-pointers, so if any of them can fire from lsquic_engine_destroy the same reasoning that motivated the two swaps applies; (2) I did not build and run the fixture in this session. A maintainer who knows the lsquic destroy path can settle (1) quickly.
Other factors
The earlier open finding (raw bound before the fallible allocations) is addressed in the current code, and the prior bot-thread resolutions by the author are consistent with the diff rather than contradicted by it. The PR's description claim that on_new_stream short-circuits via ctx_ref on the nulled owner is slightly inaccurate — on_new_stream ignores the owner and resolves the session from lsquic_conn_get_ctx — but that lookup is nulled by QuicSession::finalize/teardown and the callback cannot fire during lsquic_engine_destroy, so it is a doc nit rather than a bug. The test asserts `{stdout, s
|
On the two open points in the last review. The PR body said lsquic is vendored at vendor/lsquic. From |
This branch has not been deployed
2 participants
Problem
node:quiccrashes when it ends or is terminated. On a release build of main (fd8422c) the repro segfaults 3 of 3 runs:panic: Segmentation fault at address 0x1, exit 139. Under ASAN it isheap-use-after-freeREAD 8 on the Worker thread, top framenq_on_stream_close(packages/bun-usockets/src/node_quic_shim.c:127), reached throughlsquic_stream_destroy<-ietf_full_conn_ci_destroy<-force_close_conn<-lsquic_engine_destroy<-QuicEndpoint::release_native<-QuicEndpoint::finalize.WebWorker::shutdown->VM::~VM->Heap::lastChanceToFinalizesweeps the QUIC wrappers in unspecified order.QuicStreamandQuicSessiondropped their boxes without clearing the ctx pointer lsquic still held, so the endpoint finalizer'slsquic_engine_destroycalled back into freed memory. The same root also shows asnq_on_conn_closed(shim.c:57) and asQuicSession::push_eventviaon_conn_closedoron_mini_conn_failed.Fix
QuicStream::finalizeandQuicSession::finalizeclear lsquic's stream/conn ctx before the box drops. A non-nullraw/connalways means the lsquic object is still live:on_close/on_conn_closednull it before lsquic frees the struct, andteardown()clears it on the orderly path.QuicStream::createnow leavesrawnull untilbind_raw, after the fallible ArrayBuffer allocations, so a wrapper from a failedcreatenever points at a stream lsquic still owns.QuicEndpoint::detach_for_finalizeruns beforerelease_native. It clears the raw*mut QuicSessionregistries and nullsvtable.owner, so owner-keyed callbacks (on_mini_conn_failed,packets_out,on_new_conn, the SSL_CTX lookups) short-circuit inctx_ref.on_new_streamresolves its session through the conn ctx instead, whichQuicSession::finalizenulls.on_stream_closeandon_conn_closedfor variants that only clear the wrapper's own lsquic handle. A callback that reaches a wrapper the sweep has not freed yet then cannot follow that wrapper'ssession/endpointback-pointer into a box that is already gone.test/js/node/quic/quic-endpoint.test.ts(worker.terminate). Debug+ASAN fails 3 of 3 withsrc/at main and passes 3 of 3 with the fix.USE_SYSTEM_BUN=1(release 1.4.3) fails with the segfault. All oftest/js/node/quic/is 21/21.Background
lastChanceToFinalizeis the sweepVM::~VMruns so that every remaining cell's destructor runs before the heap goes away. It gives no ordering between cells, which is why an endpoint can outlive its own sessions here.void*per connection and per stream.node_quic_shim.creads the vtable pointer out of the front of that context on every callback, so a freed context is a wild read on the first instruction of the callback.lsquic_engine_destroyis not passive. It force-closes every live connection, which driveson_closefor each stream andon_conn_closedfor each connection.ctx_ref(src/runtime/node/quic/ffi.rs) is the one audited deref for these callbacks. A null context is its whole liveness test, so nulling a context is what makes a late callback a no-op.Notes
First opened 2026-07-25 and closed unmerged on 2026-09-13 by a stale-PR cleanup ("This is not a judgment on the fix itself"). This push merges current main and resolves two conflicts: #40516 deleted the empty
QuicStream::finalizeandQuicSession::finalizebodies in favour of theJsFinalizedefault, and this change fills those bodies in again. Theraw/conninvariants the fix relies on are unchanged on main, as is the shim's null check.What changed about the surface since the original post:
node:quicships in releases now, so this is no longer ASAN-lane only. 1.4.3 segfaults. 1.3.14 has nonode:quic.The fixture is deterministic rather than timing-based. JSC's
IsoSubspacelower tier puts the first 8 cells of each wrapper type inPreciseAllocations and the rest in aMarkedBlock, andlastChanceToFinalizesweepsMarkedBlocks first. Holding twelve live sessions plus their streams pushes those wrappers past the lower tier while the two endpoints stay in it, so the endpoint finalizer always runs after the session and stream boxes are freed. That is 8/8 fail-before where the earlier sleep-based version was about 4/8, and one run takes roughly 10s instead of 80s.Callbacks reachable from
lsquic_engine_destroy, from the vendored source:destroy_conn(lsquic_engine.c:994-1014) callson_mini_conn_failedfor a mini conn, thenci_destroy.ietf_full_conn_ci_destroy(lsquic_full_conn_ietf.c:3202) callslsquic_stream_destroyfor each stream, which callson_close(lsquic_stream.c:663), and thenon_conn_closed(line 3251).on_hsk_doneandon_hsk_confirmedare reached only from the crypto stream's handshake progress (lsquic_enc_sess_ietf.c:2044-2050viaci_hsk_done), never from the destroy path. So the three callbacks the detach covers are the full set.Four faces of the one root were observed while reducing it:
nq_on_stream_close(stream ctx),nq_on_conn_closed(conn ctx),QuicSession::push_eventfromon_conn_closed, andQuicSession::push_eventfromon_mini_conn_failedreaching a freed session through the endpoint'sprovisionallist.Open #40220 (
node:quic: remove unsafe from the endpoint, session, stream and TLS code) makes these contexts refcounted and lists a terminated-Worker hand test. It is a 26-file refactor, last pushed 2026-08-29, and currently conflicts with main, so it is not a near-term fix for this crash.[human-review] gate passed · iteration 0 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file
root cause · written by the author bot
When a Worker's VM is torn down, the finalize-order sweep can run QUIC endpoint, session, and stream finalizers while lsquic still holds raw context pointers to those objects, so native callbacks fired during native release could dereference freed memory. The fix detaches those pointers before any native resource is released: the endpoint clears its callback-visible registries and vtable callbacks first, detached callbacks null out the native pointer slots, and the session and stream finalizers clear the lsquic connection and stream contexts. A regression test terminates a worker with activ…