fetch: release the FetchTasklet through its raw pointer, not a &mut receiver - #37703
fetch: release the FetchTasklet through its raw pointer, not a &mut receiver#37703robobun wants to merge 6 commits into
Conversation
WalkthroughFetchTasklet progress and request-stream callbacks now use raw-pointer entry points with ChangesFetchTasklet lifetime handling
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: the change is complete at 639f783 (939b42e on top of it is an empty commit used to re-run CI); waiting on a maintainer. Reproduced as a contract violation rather than a crash: the shape (a CI: build 93240 built and tested exactly these sources on every lane that got an agent (179 jobs passed; the two darwin aarch64 test jobs expired waiting for an agent, and the listed test entries all passed on retry). The re-run, build 93538, failed in |
|
Updated 1:02 PM PT - Aug 12th, 2026
❌ @robobun, your commit 939b42e has 3 failures in
🧪 To try this PR locally: bunx bun-pr 37703That installs a local version of the PR into your bun-37703 --bun |
There was a problem hiding this comment.
I reviewed this PR and the automated bug hunt found no issues. Because it restructures ref-count release ordering on the fetch() hot path and its correctness rests on lifecycle invariants argued in prose (which callers of write_end_request/cancel_request_body_sink provably hold another ref), a human familiar with the FetchTasklet ref-count model should confirm those claims before merge.
What was reviewed:
on_progress_updatesplit: verified every path in the old body derefed exactly once iffis_done, and the hoisted post-call deref preserves that (mutex unlock → cancel sink → poll_ref.unref → deref ordering unchanged).write_end_requestsplit: the four old exit-site derefs each returned immediately after; collapsing to one post-end_request_bodyderef is refcount-neutral.- Traced all
cancel_request_body_sinkcallers (on_body_received, cleanup closure, on_resolve/on_reject block, abort_listener) — each runs while the JS-side ref is held per the "un-ended sink ⇒ JS ref held" invariant now stated at the call site. from_raw_ref→from_raw_mutupgrade inon_progress_updatematches the file's existing pattern;sharedis dead before the&mutis formed.
Extended reasoning...
Overview
The PR converts two &mut self methods on FetchTasklet — on_progress_update and write_end_request — into associated functions taking *mut FetchTasklet, so their trailing intrusive-refcount release runs through the raw pointer after the &mut borrow has ended rather than through ptr::from_mut(self) while the receiver is live. It updates all call sites (dispatch.rs task arm, six exits in start_request_stream, both pump-promise handlers, end_from_stream, cancel_request_body_sink) and adds a source-lint test that regex-matches the banned shape across the tree with a documented ratchet allowlist.
Security risks
None. This is an aliasing-model soundness fix; no user-facing surface, parsing, auth, or I/O changes. The behavioural claim is explicitly "moves releases, does not add or remove any."
Level of scrutiny
High. Every fetch() completion runs through on_progress_update, and the correctness of the refactor depends on non-local ref-count reasoning: the assertion that start_request_stream's and cancel_request_body_sink's write_end_request(ptr::from_mut(self), ..) calls are never the last release relies on the JS-side ref outliving them, which in turn depends on every caller of cancel_request_body_sink (including abort_listener via the AbortSignal C callback) running before the final hop's deref. I traced those callers and the invariant holds as stated, but this is exactly the kind of lifecycle reasoning REVIEW.md flags as the most-blocked category, and a maintainer who owns this file should confirm the argument.
Other factors
- I verified the deref count is preserved on every path in both split functions: the two old
is_done-gated derefs inon_progress_update(script-forbidden exit +cleanupclosure) covered every return, andwrite_end_request's four exit derefs each immediately returned — so the post-call single deref is equivalent in both cases. - The source-lint test follows the existing
test/internal/source-lints/pattern (git-tracked-only scan, self-checking positive/negative examples, ratcheted allowlist with per-entry justification), and its allowlist names two more real instances of the same UB (FileSink::finalize,StaticPipeWriter::on_write) explicitly deferred to separate PRs. - The PR description includes a Miri reduction and an extensive verification run over the fetch test suite on the ASAN debug build.
- No prior human review or outstanding comments on the timeline.
|
For whoever checks the "another ref is held" claims, here is the ledger the in-frame Refs on a tasklet: the JS-side ref from
|
|
Heads-up on the allowlist: #37716 converts the JSSink finalize chain (thunk, js_finalize, JsSinkType::finalize) to *mut, which removes both FileSink.rs sites this lint carries at 2 (running this lint against that branch reports FileSink.rs at 0). Whichever of the two lands second should drop the src/runtime/webcore/FileSink.rs entry. The FetchRequestBodySink.rs hunks do not overlap (end_from_stream here, finalize there). |
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Can we use a better container type that avoids this issue entirely?
|
Yes. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/internal/source-lints/self-receiver-release.test.ts`:
- Around line 83-104: Extend SELF_POINTER_BINDING to recognize every
receiver-derived pointer form covered by POINTER_FROM_REFERENCE, including
stored NonNull::from(self), ptr::from_ref(self).cast_mut(), and &raw mut *self
assignments. Add a banned test case for each stored form that releases or
dereferences the binding, ensuring the cases exercise the production lint guards
and constants rather than only testing the regex directly.
- Around line 49-57: Update the tracked-file initialization around the git query
in self-receiver-release.test.ts to capture stderr and throw when r.success is
false instead of returning null. Include root and guidance to run the test from
a Git checkout in the error, while preserving the Set construction for
successful queries.
🪄 Autofix
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: 338fa611-4058-4625-81bb-1d3bff693b3b
📒 Files selected for processing (4)
src/runtime/dispatch.rssrc/runtime/webcore/fetch/FetchRequestBodySink.rssrc/runtime/webcore/fetch/FetchTasklet.rstest/internal/source-lints/self-receiver-release.test.ts
…eceiver FetchTasklet::on_progress_update(&mut self) ended the final progress hop with FetchTasklet::deref(ptr::from_mut(self)). That release is the tasklet's last ref whenever the HTTP thread has already dropped its own, which is the usual order, so deinit freed the allocation while the &mut self argument was still live. write_end_request(&mut self) had the same shape: its release is the last ref when the response finishes before a streamed request body does, and the promise handlers and the native sink reached it through a &mut receiver. Both now take *mut FetchTasklet, do their &mut work through a call-scoped reborrow (on_progress_update_locked / end_request_body), and release through the raw pointer once that borrow is over, the way callback and resume_request_data_stream already do. The task arm in dispatch.rs hands over task.ptr instead of forming a &mut. A source lint bans the shape tree-wide and ratchets the remaining instances.
…klet::deref Every JS-thread release of a FetchTasklet ref (the final progress hop, the drain hop, write_end_request, the sink's finalize fallback and the teardown release of a queued hop) now adopts the ref it owns into a bun_ptr::ScopedRef at the raw-pointer entry point, before any reference to the tasklet is formed; the guard releases when it drops, after the body's borrow is gone. With no raw JS-thread release left in the file, a &mut self method has nothing to release itself with. deref_from_thread stays: it is the HTTP thread's release and hops the destroy to the JS thread rather than freeing in place. The lint also bans ScopedRef::adopt on a pointer spelled from the receiver, which is the same bug with a guard around it.
…eceiver pointer The lint now also catches a receiver-derived pointer that is stored first (NonNull::from(self), ptr::from_ref(self).cast_mut(), &raw mut *self) and released later in the same function, including through .as_ptr(). That finds one more balanced release (PostgresSQLQuery::do_run), allowlisted.
…e sink through its pointer The two pointers start_request_stream stores for later (the sink's back-pointer and the promise ctx) were made from the hop's &mut, so the last-ref releases made through them later used a borrow that accesses through the allocation pointer had since invalidated (Miri rejects that under both models). The hop now passes its allocation pointer down and those two stashes are made from it; the releases that happen inside the frame keep going through self. FetchRequestBodySink::end_from_stream takes the sink pointer, like NetworkSink's, since its release can free the tasklet and the sink. Lint: also match a bare self argument (the &mut -> *mut coercion), not counting fn definitions; allowlist the sites that surfaces; state the spellings the lint does not see; drop the FileSink entry (#37716 landed).
f4cefa8 to
0b8e3e6
Compare
… of explaining why it never releases
|
Rebased on main (picks up #37716, which converted this sink's
Fetch suites and a native-body round-trip script (both sources, both orderings) pass on the debug ASAN build at this head; details in the description. |
There was a problem hiding this comment.
I reviewed this PR and the automated bug-hunt found no issues. Given that it reworks ref-count release ordering on every fetch()'s final hop and relies on non-local provenance/refcount-balance claims across several entry points, a human look is still warranted before merge.
What was reviewed:
- Guard/borrow ordering in
on_progress_update:_js_refdrops afteron_progress_update_locked's&mut selfprotection ends;shared's last use precedes both. - Refcount balance: each removed
FetchTasklet::derefmaps to exactly oneScopedRef::adopt(final hop,write_end_request,resume_request_data_stream,release_unrun, sinkfinalize);cancel_request_body_sink's in-frame release stays under the hop's held ref. end_from_streamsplit: the JSend()path now provably never releases (JS-pump sinks returnNonefromend_and_take_task), matching the debug_assert.- Lint allowlist entries were spot-checked against the cited files; the
fn deref(self)lookbehind and stored-binding/FN_ITEM boundary handle the listed positive/negative cases.
Extended reasoning...
Overview
This PR converts FetchTasklet's JS-thread refcount releases from explicit FetchTasklet::deref(ptr::from_mut(self)) calls inside &mut self methods to bun_ptr::ScopedRef guards adopted at raw-pointer entry points. It changes the signatures of on_progress_update, write_end_request, and FetchRequestBodySink::end_from_stream from &mut self to *mut Self, threads the allocation pointer through start_request_stream for the two stashed pointers (sink back-ref, promise ctx), deletes FetchTasklet::deref, and adds a source-lint (self-receiver-release.test.ts) that ratchets remaining instances of the pattern across the codebase with a documented 10-file allowlist.
Security risks
None identified. This is a Rust aliasing-model correctness fix (Stacked/Tree Borrows protector violation on deallocation while a &mut self parameter is live). No user-controlled input parsing, auth, or crypto is touched.
Level of scrutiny
High. Every fetch() goes through on_progress_update on its final hop, and the change reorders where the last ref is dropped relative to live borrows. The correctness depends on non-local claims: which ref is the last one on which path, that the hop's guard is held across every in-frame write_end_request(self_ptr, ..), and that guard drop happens strictly after the &mut body's protection ends. The PR description proves the shape under Miri with two reductions and the author's ledger comment on the PR walks each site, but verifying those claims against the actual callback/deref_from_thread/clear_sink interleavings is exactly the kind of review REVIEW.md flags as most-blocked.
Other factors
The change is behaviorally intended to be a no-op (moves releases, doesn't add or remove any) and the fetch/ASAN suites listed in the description reportedly pass. All bot threads (comment-cop, CodeRabbit) are resolved. The new lint's allowlist names five files as "real instances tracked separately" — those are pre-existing, not introduced here, and the ratchet keeps them from multiplying. Deferring because refcount-balance and provenance reasoning across this many entry points in a hot path warrants a maintainer's sign-off, not because a defect was found.
There was a problem hiding this comment.
I reviewed this PR and the automated bug hunt found no issues. Because it restructures refcount ownership across every JS-thread release path in FetchTasklet (a hot path every fetch() traverses) and relies on a subtle ledger of which releases can be the last ref vs. which run under a guard, a human look would still be worthwhile.
What was reviewed:
- Traced each converted release site (
on_progress_update,write_end_request,resume_request_data_stream,end_from_stream,finalize,release_unrun) to confirm theScopedRefguard drops at the same point the explicitderefused to run — no ref added or lost. - Checked the
thisvsself_ptrsplit instart_request_stream: the two stashed pointers (sink back-ref, promise ctx) now come from the allocation pointer; the six in-frame synchronous releases correctly go throughself_ptrwhile the hop's guard holds the JS-side ref. - Verified
FetchRequestBodySink::end()(JS entry) can no longer reach the releasing path, and thatSinkHandle::endnow matches theNetworkSinkraw-pointer dispatch shape. - Spot-checked the lint's allowlist claims for
node_zlib_binding.rsandhtml_rewriter.rsagainst source — the "another ref outlives the call" reasoning holds for those two.
Extended reasoning...
Overview
This PR fixes a Rust aliasing-model violation in FetchTasklet: several &mut self methods ended by calling FetchTasklet::deref(ptr::from_mut(self)), which on the final progress hop is the tasklet's last ref and frees the allocation while the &mut self receiver is still a live protected tag (rejected by both Stacked Borrows and Tree Borrows under Miri). The fix converts every JS-thread entry point that owns a ref to take *mut FetchTasklet, adopt the ref into a bun_ptr::ScopedRef guard before forming any borrow, and run the body through a call-scoped reborrow. FetchTasklet::deref is deleted so the shape cannot recur. A new source-lint test ratchets the pattern across src/ with a documented allowlist.
Files touched: dispatch.rs (1-line arm change), webcore.rs (SinkHandle::end dispatch), FetchRequestBodySink.rs (end/end_from_stream/end_and_take_task/finalize), FetchTasklet.rs (~180 lines: on_progress_update split into pointer-entry + locked body, write_end_request split into guard + end_request_body, start_request_stream threads the allocation pointer for deferred stashes, promise handlers, resume_request_data_stream, release_unrun), and a new 262-line lint test.
Security risks
None identified. This is an internal memory-safety refactor with no user-facing surface, no parsing of untrusted input, and no auth/crypto/permissions code. The change is behaviour-preserving by design (moves releases, does not add or remove any).
Level of scrutiny
High. This is native memory-safety code in the fetch hot path — the exact category the review guidelines call out as most-blocked. The correctness argument depends on a per-site ledger of which release can be the last ref (must go through the allocation pointer) vs. which runs while another guard holds a ref (may go through self). The PR itself required a self-review correction (the stashed-pointer provenance issue in the second Miri reduction), which is evidence the reasoning is non-obvious. The allowlist in the new lint also encodes claims about ten other files ("harmless" vs "real, tracked separately") that a maintainer familiar with those subsystems should confirm.
Other factors
- The PR description is unusually thorough, with two Miri reductions and a per-site ownership ledger; verification ran the fetch suite plus a targeted native-body script under ASAN.
- All bot threads (comment-cop, CodeRabbit) are resolved; comments were trimmed or the code restructured (e.g.
end()no longer reaches the releasing entry point). - A maintainer was explicitly pinged mid-thread for the ScopedRef redesign, suggesting this was already expected to get human eyes.
- No behavioural test asserts the fix directly (the aliasing violation has no known crash); coverage is the source lint plus existing fetch suites, which is appropriate for a Miri-only UB fix but means the ownership ledger is the load-bearing artifact to review.
… pointers, not &mut receivers (#37870) ### Problem - Under `bun run rust:miri`, tearing down an h2 session or a proxy tunnel is reported as undefined behaviour: "deallocation through <tag> is forbidden ... the strongly protected tag disallows deallocations", pointing at the method's own receiver. The reduction in #37703 is this shape, and the ASAN use-after-free trace in #31788 is the same shape observed at runtime, in `abort_by_http_id`. - Cause: four refcount releases in `bun_http` (h2 `on_close` and `maybe_release`, `ProxyTunnel::detach_and_deref`, h3 `detach`) go through the method's `&mut self`. For the h2 and tunnel sites that release is normally the last one, so the object is freed while a reference argument to it is still live, which is UB whether or not the reference is used again. - The keep-alive guards had the same defect one step removed: they were built from the receiver, so on failure paths the free still happened inside the method, at guard drop. - Two h2 entry points, `adopt` and `abort_by_http_id`, held no ref of their own while their body could tear the session down; `adopt` then read a field of the freed session. The `adopt` case was not triggered deterministically. ### Fix - Every h2 entry point that can end with the session released now takes the pointer its holder stores (socket slot, registry, pool, or what `create` returned) and runs through one wrapper: take a guard from that pointer, run the former `&mut self` body, then release the socket ref the body gave up (recorded in a flag) and the guard's ref, both after the borrow has ended. - Property to check: inside a body no release can be the last one, because the wrapper's guard holds a ref, and the two releases that can be last run with no reference to the session in existence. The points at which refs are released relative to other work are unchanged. - Proxy tunnel: `receive` and `on_writable` build their guard from the client's handle pointer; `detach_and_deref` is deleted (`start` now builds the TLS wrapper before allocating the tunnel, and the pool fallback releases through the handle it was given); `adopt` moves the pool's handle into the client instead of re-deriving one from the receiver. - h3 `detach`'s release is never the last one (the connection's own ref outlives every stream), so it keeps `&mut self`, says why, and releases through the stream's backref. - Verification: a new source lint fails on `main` at the five receiver-release sites and passes on this branch; the existing h2, h3 and proxy suites pass on an ASAN debug build; clippy and fmt are clean. The lint is the only test that fails without the change. ### Background - Intrusive refcount: `ClientSession` and `ProxyTunnel` carry their own `ref_count`; `deref(ptr)` decrements it and frees the allocation at zero. Each holder (socket ext slot, context registry, keep-alive pool, `HTTPClient.proxy_tunnel`) owns one count, and hand-offs between holders move a count rather than bump it. - Holders of an h2 session: the TLS socket's ext slot is tagged with the session, `HTTPContext.active_h2_sessions` lists it so later requests can multiplex onto it, and the keep-alive pool holds it while it is parked with no streams. - Protectors: under both of Miri's aliasing models (Stacked Borrows and Tree Borrows), a `&mut T` argument is protected for the whole call, so freeing the pointee during the call is UB even if the argument is never touched again. This is why the fix moves the free to after the body returns instead of avoiding later uses of `self`. - `bun_ptr::ThisPtr` / `ScopedRef`: a copyable raw handle to a refcounted object, and an RAII guard that bumps on construction and releases on drop. `SessionPtr` in this PR is `ThisPtr<ClientSession>`. - `RefPtr` (tunnels): the owning handle stored by the client or the pool; `RefPtr::deref()` releases the count that handle owns, so releasing through it is releasing through the holder's pointer. <details> <summary>Original description</summary> ### Problem Four intrusive-refcount releases in `bun_http` went through the method's own receiver, `&mut self` coerced to `*mut Self` at the call: ``` src/http/h2_client/ClientSession.rs:848 on_close(&mut self) unsafe { ClientSession::deref(self) } src/http/h2_client/ClientSession.rs:959 maybe_release(&mut self) unsafe { ClientSession::deref(self) } src/http/ProxyTunnel.rs:751 detach_and_deref(&mut self) unsafe { ProxyTunnel::deref(self) } src/http/h3_client/ClientSession.rs:192 detach(&mut self, stream) unsafe { ClientSession::deref(self) } ``` (`grep -rnP '\bderef\(\s*self\s*\)' src` at 9a543cc; the two `SubprocessPipeReader.rs` hits are being converted in their own PR.) For the h2 and tunnel sites the release is normally the last one. An h2 session is held by the socket ext slot, the context registry and (while parked) the pool; `on_close` leaves the registry and then releases the slot's ref, and `maybe_release` does the same on a connection it cannot pool, so the session is freed inside a method that still has a `&mut self` argument pointing at it. `detach_and_deref` was reached from `HTTPContext::release_socket` with the only remaining ref to the tunnel. Freeing an allocation while a reference argument to it is live is rejected by both aliasing models regardless of whether the reference is used again (Tree Borrows, the model `bun run rust:miri` uses: "deallocation through <tag> is forbidden ... the strongly protected tag disallows deallocations", pointing at the receiver; Stacked Borrows: "deallocating while item [Unique] is strongly protected"). The reduction in #37703's description is this exact shape. The keep-alive guards had the same problem one step removed: `ClientSession::ref_scope(&mut self)` built a `ScopedRef` from the receiver and every socket event took one, so on the `fail_all` paths the free happened at the guard's drop, still inside `on_data` / `on_writable` / `resume_receive_by_http_id`; `ProxyTunnel::receive` / `on_writable` built theirs from `NonNull::from(&mut *self)`, and when delivering the bytes completes or fails the request (which releases the client's ref, or hands it to a full pool) that guard's drop is the last release. Two entry points additionally held no ref at all while their body could reach one of these releases. `adopt`: both callers in `HTTPContext::connect` (registry match and pool resume) hold nothing of their own, so when the first flush of the new stream fails, `attach` -> `fail_all` -> `on_close` freed the session and `adopt` then read `self.encoder_poisoned` from the freed allocation (needs a TLS write to fail synchronously during the adopt; I could not trigger it deterministically). `abort_by_http_id`: the ASAN trace in #31788 is this shape observed, a custom TLS context dropping re-entrantly from the aborted request's result callback, `on_close` freeing the session at its guard drop while `abort_by_http_id`'s `&mut self` was live up the stack, and the tail of `abort_by_http_id` then running on freed memory. #31788 fixes that (and two unrelated defects) by giving `abort_by_http_id` a `ref_scope()` guard of its own, which moves the free to that guard's drop, still inside the method; here every entry point gets the guard from `enter`, taken from the holder's pointer, and the free happens after the body's borrow has ended. The two PRs overlap in that one line and are otherwise independent. The h3 site is different: the per-stream ref `detach` releases is provably never the last one (the connection's ref is released only by `on_conn_close` / `fail_session`, and both drain `pending` through `detach` first), so `&mut self` is a sound receiver there. The function now says so, and releases through the pending entry's own backref so that every release in the crate goes through the pointer of the holder whose ref it is. ### Fix **h2** (`h2_client/ClientSession.rs`, `HTTPContext.rs`, `HTTPThread.rs`, `lib.rs`): every entry point that can end with the session released takes a `SessionPtr` (`bun_ptr::ThisPtr<ClientSession>`, the pointer its holder has: socket ext tag, registry entry, pool entry, or what `create` returned) and goes through `ClientSession::enter`, which takes a guard from that pointer, runs the former `&mut self` body through a call-scoped reborrow, and after it returns releases the socket-ext ref if the body gave it up (a `socket_ref_owed` cell set by `fail_streams` and by `maybe_release`'s close branch, where the `deref(self)` calls were) and then the guard's own ref, both through the holder's pointer. Inside a body no release can be the last one (the guard holds one), and the two that can be last now run with no reference to the session in existence. Converted entries: `on_data`, `on_writable`, `on_close`, `adopt`, the leader's `attach_leader`, `enqueue`, `abort_by_http_id`, `stream_body_by_http_id`, `resume_receive_by_http_id`, `drain_response_body_by_http_id`; the bodies are private, so the type has no `&mut self` path to a release left. `ActiveSocketExt::session_mut` becomes `session() -> Option<SessionPtr>`, `HTTPThread` holds its own `ref_guard()` across the resume + drain pair (what its `ref_scope()` was for), `connect()` adopts after its registry scan has finished instead of from inside the loop that `maybe_release` swap-removes from, and the registry releases its ref through the entry it stored rather than whatever pointer the caller passed. Order of operations within each entry is unchanged; the releases happen at the same points relative to everything else, just after the body's borrow has ended. **proxy tunnel** (`ProxyTunnel.rs`, `HTTPContext.rs`, `lib.rs`): `receive` and `on_writable` take the client's handle pointer (`RefPtr::data`) and build their guard from it, the same contract the SSL callbacks in the file already use for `ref_scope`. `detach_and_deref` is deleted: `start()` now builds the SSL wrapper before allocating the tunnel, so its failure path has nothing to release, and the pool fallback in `release_socket` releases through the `RefPtr` it was handed, as `close_proxy_tunnel`, `AsyncHTTP` and the shutdown path already do. `adopt` takes the pool's `RefPtr` and moves it into the client instead of re-deriving a handle from its receiver with `RefPtr::from_raw(from_mut(&mut *self))`, so the ref the client eventually releases is the one the pool held. **h3** (`h3_client/ClientSession.rs`): `detach` documents why its release is never the last one and performs it through the stream's `session` backref. ### Test `test/internal/source-lints/self-receiver-deref.test.ts` bans `Type::deref(self)` (and the other raw-pointer release entry points of the refcount traits) and `ScopedRef` / `*RefGuard::new|adopt(self)`, checks its patterns against positive and negative spellings, and ratchets the two `SubprocessPipeReader.rs` sites at their exact count until their own conversion lands. Against `main` it reports exactly ``` src/http/h2_client/ClientSession.rs:206: SessionRefGuard::new(self) src/http/h2_client/ClientSession.rs:848: ::deref(self) src/http/h2_client/ClientSession.rs:959: ::deref(self) src/http/h3_client/ClientSession.rs:192: ::deref(self) src/http/ProxyTunnel.rs:751: ::deref(self) ``` and passes with this branch. It is deliberately limited to the bare receiver; the `ptr::from_mut(self)` family is what #37703's lint covers, and the two compose. ### Verification Debug (ASAN) build: `test/js/web/fetch/fetch-http2-client.test.ts`, `fetch-http2-adversarial.test.ts`, `fetch-http2-leak.test.ts`, `fetch-proxy-connect-tunnel-split-envelope.test.ts`, `fetch-proxy-tls-intern-race.test.ts`, `fetch-http3-client.test.ts`, `fetch-http3-adversarial.test.ts`, `fetch-http3-cold-post.test.ts`, and `test/js/bun/http/proxy.test.{ts,js}` plus the seven `proxy-stress-*` suites all pass (two `proxy.test.js` auth cases need `NO_PROXY` unset in my container because it lists `localhost`; they pass with it unset, and fail identically on the released binary with it set). `bun test test/internal/source-lints/` passes; `cargo clippy -p bun_http` and `cargo fmt --check` are clean. Not touched here, noted while reading: the pool's handle in `maybe_release` (`NonNull::from(&mut *self)`) and the h2 `Stream`/h3 `Stream` backrefs are still pointers derived from a receiver, but none of them is a release site, and the re-entrant `HTTPContext` borrows described on `unregister_h2_raw` are a separate problem. </details> --------- Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
Problem
fetch()ends with one final progress hop on the JS thread. That hop releases the tasklet's JS-side ref, normally its last, so the release frees the tasklet. The release was made from inside a&mut selfmethod, through a pointer spelled from the receiver.&mutparameter to it is still live is undefined behaviour under both of Rust's aliasing models, whether or not the reference is touched afterwards. A reduction of the shape fails under Miri withUndefined Behavior: deallocation through <1642> at alloc786[0x0] is forbidden(Tree Borrows) anddeallocating while item [Unique for <1663>] is strongly protected(Stacked Borrows).Fix
&mutbody through a borrow scoped to that call. The guard releases when it drops, after the borrow is gone, so no reference is live at the free.&mut selfmethod on this type has nothing left to misuse. The HTTP thread's release stays, since it posts the free to the JS thread instead of freeing in place.self; the hop's guard holds a ref throughout, so none of them can be the last.&mut selfmethods on this type safe to re-enter is out of scope (fetch: encode FetchTasklet's cross-thread ownership in the type system #31745). The same shape in other files is allowlisted by the new lint, per file at its exact count.Background
FetchTaskletis the per-fetch()object shared by the HTTP thread and the JS thread. It is a heap allocation held by raw pointer with an intrusive refcount; the release that takes the count to zero frees it.bun run rust:mirichecks), a reference passed as a function parameter is protected for the whole call: freeing its allocation during the call is undefined behaviour even if nothing reads it afterwards. A raw pointer parameter carries no such protection.&mutborrow is only valid while that borrow is. Once the allocation is used through another path, releasing through the stale pointer is also undefined behaviour, so anything stored past the current frame must come from the original allocation pointer.bun_ptr::ScopedRef::adoptwraps a ref the caller already holds in a guard that releases it, through its own pointer, on drop. One other task arm in the dispatcher already releases this way.[review] gate passed · iteration 1 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file
Original description
Problem
FetchTasklet::on_progress_update(&mut self)(src/runtime/webcore/fetch/FetchTasklet.rs) ended the final progress hop withThat deref releases the JS-side ref taken in
get().callbackon the HTTP thread derefs its own ref right after posting the final hop, so by the time the hop runs on the JS thread this is normally the tasklet's last ref:derefrunsdeinit, whichheap::takes the box, while the&mut selfargument (formed bycast!(FetchTasklet)in dispatch.rs) is still live. Everyfetch()goes through this on its final hop.Freeing an allocation while a reference argument to it is live is rejected by both aliasing models, independent of whether anything touches the reference afterwards. A standalone reduction of exactly this shape (a
&mut selfmethod whose trailing release drops the count to zero and frees the box) fails under Miri with Tree Borrows, the modelbun run rust:miriuses, pointing at the receiver:and under Stacked Borrows with
deallocating while item [Unique for <1663>] is strongly protected. The same reduction with the release performed through the allocation pointer after the method returns passes under both. No crash is known from this today; it is the contract that is wrong, and it is the reason the file's other releasing entry points (callback,resume_request_data_stream,deref_from_thread) already take*mutand say so in their comments.write_end_request(&mut self, err)had the same shape (let this_ptr = ptr::from_mut(self); ... FetchTasklet::deref(this_ptr)on four exits). Its release is the last ref when the response finishes before a streamed request body does: the final hop's cleanup cancels the sink and drops the JS-side ref, and the pump promise then settles intoon_resolve_request_stream/on_reject_request_stream, which called(*this).write_end_request(..)through a&mutreceiver.FetchRequestBodySink::end_from_streamreached it throughtask.get_mut()the same way (its own comment already noted the call may free the tasklet and the sink with it).Reduction run under Miri
MIRIFLAGS=-Zmiri-tree-borrows cargo miri run -- beforeand the default (Stacked Borrows) run both report the errors quoted above;-- afterexits 0 under both.Fix
The container that rules this out is
bun_ptr::ScopedRef(already used this way by theFileResponseStreamEofarm in dispatch.rs): the entry point that owns a ref receives the allocation pointer, adopts the ref into a guard before any reference to the tasklet exists, and runs the&mutbody through a call-scoped reborrow; the guard releases through its own pointer when it drops, after that borrow is gone. Every JS-thread release of a tasklet ref is now such a guard, andFetchTasklet::deref, the raw release a&mut selfmethod could reach for, is deleted, so the file cannot express the bug any more.deref_from_threadstays: it is the HTTP thread's release, and it hops the destroy to the JS thread instead of freeing in place.on_progress_update(this: *mut FetchTasklet): takes the mutex and readsis_donethroughfrom_raw_ref, adopts the JS-side ref whenis_done(Option<ScopedRef>), and runs the former body ason_progress_update_locked(&mut self, this, is_done). The two derefs inside the body (the script-forbidden exit and thecleanupclosure) are gone; the deref was already the last thing every path did, so the order of operations is unchanged. The dispatch.rs arm passescast_ptr!(FetchTasklet)instead of forming a&mut.write_end_request(this: *mut FetchTasklet, err): adopts thestart_request_streamref and runs the former body asend_request_body(&mut self, err)(the four exit-site derefs collapse into the guard).start_request_streamstores for the deferred releases, the sink's back-pointer and the promise ctx recovered byon_resolve_request_stream/on_reject_request_stream, are now made from the allocation pointer the hop passes down (BackRef::from_raw_mut(this),.then(.., this, ..)) instead of from its own&mut self. Self-review of the first version caught this: a pointer made from the hop's borrow is dead by the time those releases run (every later access through the allocation pointer, including the hop's own release, invalidates it), so the last-ref release through it was rejected by both models even after the receiver fix. The second reduction below models exactly that and passes only with the allocation pointer stashed. The releases that happen inside the frame (start_request_stream's six synchronous exits,cancel_request_body_sink) keep going throughself: they run while the hop's guard still holds the JS-side ref, so they never free, and a pointer derived from the live borrow is the right one to use inside it. The ledger for that claim is in the comments below.FetchRequestBodySink::end_from_streamtakes the sink pointer (as its siblingNetworkSink::end_from_streamalready did; theSinkHandle::endarm passesp.as_ptr()), because the release it makes can free the tasklet and, throughclear_sink, the sink itself. The JS-sideend()calls the non-releasing helper directly and debug-asserts that there was nothing to release, since only JS-pump sinks have a JS object.finalizewas converted the same way by JSSink: pass the sink to finalize as *mut instead of &mut #37716, which this branch is rebased on.resume_request_data_stream,release_unrunand the sink'sfinalizefallback adopt the ref they own the same way (the first loses the closure it used to reach a single trailing deref).Behaviour is unchanged: the diff moves releases, it does not add or remove any, and the guards drop at exactly the points the explicit derefs used to run.
What this does not do: make
&mut selfmethods of this type safe to re-enter (that is the&self/Cellconversion #31745 is pursuing for this type), or touch other types. The same shape exists elsewhere; see the lint's allowlist and scope note below.Second reduction: the stashed pointer
Stashing
from_mut(self)(the first version of this PR, and whatmaindoes): Tree Borrows reportsreborrow through <tag> ... is forbidden ... has state Disabled ... due to a foreign write access, Stacked Borrows reportstrying to retag from <tag> for Unique permission ... that tag does not exist in the borrow stack. Stashing the allocation pointer (-- root): both exit 0.Tests
test/internal/source-lints/self-receiver-release.test.tsbans a release (dereffamily orScopedRef::adopt) applied to the receiver: spelled as a pointer inline (deref(ptr::from_mut(x)),deref(ptr::from_ref(x).cast_mut()),deref_nn(NonNull::from(self)),self as *mut,&raw mut *self,ScopedRef::adopt(ptr::from_mut(self))), passed as a bareselfthat the*mutparameter coerces (deref(self), which the self-review found the first version missed, and which is how six live sites spell it), or stored in a local first and released later in the same function (including via.as_ptr()). Definitions such asfn deref(self)do not count. It checks its patterns against positive and negative examples and ratchets the remaining instances with their reason. Againstmainit reports exactlyand passes with this branch. Its header states what it does not see, most importantly
deref(self.as_ctx_ptr()): that is the spelling bun_ptr'sAsCtxPtrdoc currently recommends and the one most R-2 wrappers (Subprocess, sockets, websocket_client, the SQL connections) use, so it is a decision about the idiom rather than a list of sites, and it has been handed off as such. The allowlist entries were each read. Non-final releases (another ref provably outlives the call):html_rewriter.rs,node_zlib_binding.rs,js_valkey.rs,process.rs,PostgresSQLConnection.rs,PostgresSQLQuery.rs,h3_client/ClientSession.rs, three of the fourstatic_pipe_writer.rssites. Real instances of this bug, allowlisted at their exact counts so they cannot multiply, each with its own fix in flight or tracked:static_pipe_writer.rs(on_write, io: report to pipe writer parents through raw pointers, release StaticPipeWriter through its backref #37755),SubprocessPipeReader.rs(on_reader_done/on_reader_error),h2_client/ClientSession.rs(on_close, whoseref_scopeguard is the final release, andmaybe_release),ProxyTunnel.rs(detach_and_deref). The FileSink entry from the first version is gone because JSSink: pass the sink to finalize as *mut instead of &mut #37716 landed. The sibling lints for the unconditional-teardown half of the class (self-receiver-reclaimfrom blob: hand the read handler's pointer to on_read_bytes instead of &mut self #37681;destroy/deinit/finalizein bundler: make Worker::deinit_soon take the worker pointer instead of &mut self #37685, node:fs: let the fs completions own their task box instead of freeing it through &mut self #37693, blob(windows): free CopyFileWindows and ReadFileUV through the task pointer, not under &mut self #37705) state that refcount releases are out of their scope, which is what this one covers.Verification
On the debug (ASAN) build, at the current head: test/js/web/fetch/{fetch-abort-stream-body,body,client-fetch,fetch-keepalive,fetch-http2-client,fetch-stream-cancel-leak,fetch-response-finalizer-sweep,exiting}.test.ts, test/js/bun/http/fetch-file-upload.test.ts, test/js/node/http/node-fetch.test.js, test/regression/issue/13696.test.ts and the
used as a request bodytest in fetch.test.ts all pass (the one timeout seen was the ITER=100 ended-inline fixture finishing in 3.9 to 5.0 s in this container, where the debug binary's start-up alone is about 2.7 s of that). A script streaming aBun.file()stream and an upstream response body into fetch, both against a server that reads the whole body and against one that answers before the upload finishes (the ordering wherewrite_end_request's release is the last ref, through both the promise handlers andend_from_stream), ran 20 rounds with a full GC between rounds without an ASAN report. Earlier heads additionally ran the broader fetch suites listed in the history of this description (fetch, fetch.stream, fetch-leak, fetch-backpressure, abort-signal-leak, fetch.tls and others), with only environment-specific failures.bun test test/internal/source-lints/(19 files) passes;cargo clippy -p bun_runtimeandcargo fmt --checkare clean.