Skip to content

bundler: make Worker::deinit_soon take the worker pointer instead of &mut self - #37685

Open
robobun wants to merge 6 commits into
mainfrom
farm/cf508add/bundler-worker-deinit-soon-raw-receiver
Open

bundler: make Worker::deinit_soon take the worker pointer instead of &mut self#37685
robobun wants to merge 6 commits into
mainfrom
farm/cf508add/bundler-worker-deinit-soon-raw-receiver

Conversation

@robobun

@robobun robobun commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Problem

  • No crash is known from this. Worker::deinit_soon(&mut self) in the bundler thread pool frees the Worker it is called on, and every Bun.build() / bun build teardown goes through it.
  • A &mut self argument has to stay allocated until the call returns. Freeing it during the call is undefined behaviour under both of Rust's aliasing models, whichever thread does the freeing.
  • On the branch teardown normally takes, a pool thread may free the worker before deinit_soon returns; on the other branch the function frees it itself. A standalone reduction of this shape fails under Miri on both branches (the strongly protected tag disallows deallocations).
  • The task pointer handed to the pool thread was derived from &mut self, so under Stacked Borrows it covers one field, not the whole allocation that later gets freed.

Fix

  • deinit_soon takes this: *mut Worker. It reads thread and takes the task address through statement-scoped raw place expressions, and the one caller (bundler teardown) passes the pointer it already stores. Same code path and order as before; no behaviour change.
  • Correct because no reference to the Worker exists at either point where it can be freed, and the pointer that reaches the free is derived from the allocation itself. The reduction in the fixed shape passes under both models. This is the same contract the tree's other freeing teardown functions already use.
  • The branch that frees inline looks defensive: no path that creates a Worker off a pool thread was found. The pool-thread branch is the one that matters.
  • A new source lint bans handing self as a raw pointer to deinit / destroy / finalize, directly or through a scopeguard. On main it reports exactly this line; four other files keep their current counts in an allowlist that ratchets down as sibling PRs convert them (install: lifecycle exit returns Disposition; never free self via &mut #37551, 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).
  • Verification: the lint fails without the fix and passes with it. On a debug (ASAN) build the bundler API and plugin suites, which tear workers down through the fixed branch, pass. The UB itself was only demonstrated in the standalone Miri reduction, not in bun.

Background

  • Bundler workers: Bun.build() parses on a thread pool. Each pool thread gets a heap-allocated Worker (parser state, allocators), recorded in a workers_assignments map, and the bundle tears every entry down once at the end.
  • Idle tasks: deinit_task is a task struct embedded inside the Worker. push_idle_task hands its address to the pool thread, whose callback recovers the enclosing Worker from it (container_of) and frees it. Pool threads drain idle tasks between batches, so this can happen as soon as the task is pushed.
  • Protectors: under Stacked Borrows and Tree Borrows (the aliasing models Miri checks; bun run rust:miri uses the latter) a reference argument is protected for the whole call, so freeing it mid-call is UB even if it is never touched again. rustc's dereferenceable attribute on reference arguments encodes the same assumption.
  • Provenance: a raw pointer remembers what it was derived from. One taken from a field reference may only be valid for that field, so a callback that reclaims the whole Box needs a pointer derived from the allocation's own pointer.
  • Source lints: test/internal/source-lints/ holds tests that regex-scan the tree for banned code shapes. Files still carrying a shape are allowlisted with an exact count, so converting a file forces its entry to be deleted and nothing new can take its place.

[review] gate passed · iteration 0 · 3 files touched

fails on main (without fix)
ASAN without fix: 1 FAILED
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/internal/source-lints/self-receiver-teardown.test.ts
bun test v1.4.0 (5c9febbb0)

test/internal/source-lints/self-receiver-teardown.test.ts:
(pass) scans a non-empty set of tracked Rust sources [2.01ms]
(pass) the patterns recognize the spellings they claim to [21.31ms]
231 |   expect(banned.filter(s => !matches(s))).toEqual([]);
232 |   expect(allowed.filter(matches)).toEqual([]);
233 | });
234 | 
235 | test("no method hands its own receiver to destroy/deinit/finalize", () => {
236 |   expect(offenders).toEqual([]);
                          ^
error: expect(received).toEqual(expected)

- []
+ [
+   "src/bundler/ThreadPool.rs:590: Self::deinit(std::ptr::from_mut::<Self>(self))",
+ ]

- Expected  - 1
+ Received  + 3

      at <anonymous> (/workspace/bun/test/internal/source-lints/self-receiver-teardown.test.ts:236:21)
(fail) no method hands its own receiver to destroy/deinit/finalize [5.04ms]
(pass) allowlisted files still carry exactly their documented count [6.89ms]

 3 pass
 1 fail
 5 expect() calls
Ran 4 tests across 1 file. [2
... (truncated)

release without fix: 1 FAILED
bun test v1.4.0-canary.1 (9008ae7ab)

test/internal/source-lints/self-receiver-teardown.test.ts:
(pass) scans a non-empty set of tracked Rust sources [0.67ms]
(pass) the patterns recognize the spellings they claim to [0.50ms]
231 |   expect(banned.filter(s => !matches(s))).toEqual([]);
232 |   expect(allowed.filter(matches)).toEqual([]);
233 | });
234 | 
235 | test("no method hands its own receiver to destroy/deinit/finalize", () => {
236 |   expect(offenders).toEqual([]);
                          ^
error: expect(received).toEqual(expected)

- []
+ [
+   "src/bundler/ThreadPool.rs:590: Self::deinit(std::ptr::from_mut::<Self>(self))",
+ ]

- Expected  - 1
+ Received  + 3

      at <anonymous> (/workspace/bun/test/internal/source-lints/self-receiver-teardown.test.ts:236:21)
(fail) no method hands its own receiver to destroy/deinit/finalize [0.21ms]
(pass) allowlisted files still carry exactly their documented count [0.18ms]

 3 pass
 1 fail
 5 expect() calls
Ran 4 tests across 1 file. [776.00ms]
__F:1:S:0
passes on PR (with fix)
ASAN with fix: all passed
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/internal/source-lints/self-receiver-teardown.test.ts
bun test v1.4.0 (5c9febbb0)

test/internal/source-lints/self-receiver-teardown.test.ts:
(pass) scans a non-empty set of tracked Rust sources [3.92ms]
(pass) the patterns recognize the spellings they claim to [34.64ms]
(pass) no method hands its own receiver to destroy/deinit/finalize [2.35ms]
(pass) allowlisted files still carry exactly their documented count [10.40ms]

 4 pass
 0 fail
 5 expect() calls
Ran 4 tests across 1 file. [36.13s]
__F:0:S:0

release with fix: all passed
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped) in 1652ms (unchanged)
ninja: Entering directory `/workspace/bun/build/release'
[1/5] gen generated_host_exports.rs
generated_host_exports.rs: 93 exports (host=3, lazy=10, generic=80, rust=0); 239 extern-C blocks audited
[1/5] cargo bun_bin → libbun_rust.a (--target x86_64-unknown-linux-gnu)

  nightly-2026-07-20-x86_64-unknown-linux-gnu unchanged - rustc 1.99.0-nightly (9f36de775 2026-07-19)

^[[1m^[[92m   Compiling^[[0m bun_options_types v0.0.0 (/workspace/bun/src/options_types)
^[[1m^[[92m   Compiling^[[0m bun_crash_handler v0.0.0 (/workspace/bun/src/crash_handler)
^[[1m^[[92m   Compiling^[[0m bun_resolve_builtins v0.0.0 (/workspace/bun/src/resolve_builtins)
^[[1m^[[92m   Compiling^[[0m bun_api v0.0.0 (/workspace/bun/src/api)
^[[1m^[[92m   Compiling^[[0m bun_js_parser v0.0.0 (/workspace/bun/src/js_parser)
^[[1m^[[92m   Compiling^[[0m bun_js_printer v0.0.0 (/workspace/bun/src/js_printer)
^[[1m^[[92m   Compiling^[[0m bun_spawn v0.0.0 (/workspace/bun/src/spawn)
^[[1m^[[92m   Compiling^[[0m bun_patch v0.0.0 (/workspace/bun/src/patch)
^[[1m^[[92m   Compiling^[[0m bun_resolver v0.0.0 (/workspace/bun/src/resolve
... (truncated)
diff hotspot
src/bundler/ThreadPool.rs                          |  28 ++-
 src/bundler/bundle_v2.rs                           |   8 +-
 .../source-lints/self-receiver-teardown.test.ts    | 245 +++++++++++++++++++++
 3 files changed, 271 insertions(+), 10 deletions(-)

gate history · 1 passed · 0 rejected · iteration 0

evidence per changed file
file                                                      reads  edits  tests
src/bundler/ThreadPool.rs                                     5     12      0
src/bundler/bundle_v2.rs                                      2      1      0
…st/internal/source-lints/self-receiver-teardown.test.ts      4     13      0
Original description

Problem

Worker::deinit_soon(&mut self) in src/bundler/ThreadPool.rs frees the Worker it is called on:

pub(crate) fn deinit_soon(&mut self) {
    if let Some(thread) = self.thread {
        thread.push_idle_task(&raw mut self.deinit_task);   // pool thread frees *self from deinit_callback
    } else {
        unsafe { Self::deinit(std::ptr::from_mut::<Self>(self)) };   // heap::take(this) right here
    }
}

A &mut self argument is protected for the duration of the call, and deallocating memory that a protected reference points into is UB under both aliasing models, whoever does the deallocating:

  • else branch: deinit reclaims the Box while the deinit_soon frame still holds the protected &mut self.
  • if branch: push_idle_task publishes the intrusive deinit_task, and the pool thread may run deinit_callback immediately. Thread::drain_idle_events runs after every task batch and on every idle wake (src/threading/ThreadPool.rs), not only after the wake_for_idle_events() the caller issues afterwards, so a pool thread can free the Worker before deinit_soon returns. This is the branch every Bun.build() / bun build teardown takes. The task pointer was also derived from the &mut self reborrow rather than from the allocation's own pointer; under Tree Borrows that still covers the whole Worker, but under Stacked Borrows it only carries the field's range, which is the second Miri error below, and deriving it from the allocation pointer is the shape bun_core::container_of's docs describe.

A standalone reduction of exactly this shape (boxed struct, intrusive task field, push_idle_task modelled as the pool thread running the task immediately) fails under Miri on both branches. Tree Borrows, which bun run rust:miri uses:

error: Undefined Behavior: deallocation through <1834> at alloc841[0x0] is forbidden
     = help: the strongly protected tag <1823> disallows deallocations
help: the strongly protected tag <1823> was created here, in the initial state Reserved
  45 |     fn deinit_soon(&mut self) {

Stacked Borrows reports deallocating while item [Unique for <1870>] is strongly protected for the else branch and, for the if branch, rejects the Box reclaim in the callback because the field pointer derived from &mut self does not carry the whole allocation (trying to retag from <1902> for Unique permission at alloc845[0x8], but that tag does not exist in the borrow stack). The fixed shape below passes under both models on both branches.

No crash is known from this; it is the same contract fix as the tree's other teardown functions that end in a free and therefore take this: *mut Self (see the comment on deinit in src/sql_jsc/postgres/PostgresSQLConnection.rs, and schedule_with_options in this same file for the same reasoning applied to a publish).

Fix

deinit_soon becomes unsafe fn deinit_soon(this: *mut Self). It reads thread and takes the deinit_task address through statement-scoped raw place expressions, so no reference to the Worker exists when either free can happen, and the task pointer now carries the allocation's own provenance. The else branch passes this straight to deinit. The only caller, the worker teardown loop in BundleV2::deinit_without_freeing_arena (src/bundler/bundle_v2.rs), passes the pointer stored in workers_assignments. Same code path and order as before; no behaviour change.

I could not find a path that creates a Worker off a pool thread today (all nine Worker::get callers are pool-task callbacks, and pool threads always have Thread::current() set), so the else branch looks defensive; the if branch is the one that matters in practice.

Tests

test/internal/source-lints/self-receiver-teardown.test.ts bans deinit(..) / destroy(..) / finalize(..) calls whose argument is self spelled as a raw pointer (ptr::from_mut(self), self as *mut _, &raw mut *self, addr_of_mut!(*self), NonNull::from(self), with or without a trailing cast), and the deferred scopeguard::guard(ptr::from_mut(self), |p| Self::destroy(p)) form; it checks the patterns against positive and negative examples and ratchets an allowlist. On main it reports exactly

src/bundler/ThreadPool.rs:590: Self::deinit(std::ptr::from_mut::<Self>(self))

The other files that carry the shape are allowlisted at their current counts, so each conversion deletes its entry: src/install/lifecycle_script_runner.rs (5, #37551), src/runtime/node/node_fs.rs (3, #37693), src/runtime/webcore/blob/copy_file.rs (2) and read_file.rs (1, both #37705). The ratchet has already fired once: #37716 converting ArrayBufferSink landed while this was open, the entry went stale, and the Source lints job failed on the merge commit until the entry was deleted, which is the intended behaviour. #37693 and #37705 add this same file; the copies share the same body and differ in which entry each one deletes (this one additionally matches nested turbofish, from_mut(&mut *self) and a parenthesized cast, and names the file when the ratchet fails), so whichever lands first, the other two resolve the add/add conflict by taking the landed file and deleting their own entry. The header states the boundary honestly: the callee name list is what is enforced (src/runtime/dns_jsc/dns.rs's on_cares_complete sites have the same shape under another name and are being converted separately; the name can be added once they are), and refcount releases (deref(..)), a bare self coerced to *mut Self by the callee, and the heap::take / Box::from_raw layer (covered by self-receiver-reclaim.test.ts, now on main) are out of scope.

Verification

Debug (ASAN) build: test/bundler/bun-build-api.test.ts (52 pass) and test/bundler/bundler_plugin.test.ts (53 pass), which tear down workers through the if branch on every build including the failing ones; bun bd test test/internal/source-lints/ (18 files, 82 tests) passes, and the new lint fails with the two src files at their main versions, reporting exactly the line shown above. cargo clippy and cargo fmt --check on bun_bundler are clean.

@coderabbitai

coderabbitai Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 21 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: fada2e28-f284-4278-9308-8a5752afbb3e

📥 Commits

Reviewing files that changed from the base of the PR and between 626034f and 5c9febb.

📒 Files selected for processing (3)
  • src/bundler/ThreadPool.rs
  • src/bundler/bundle_v2.rs
  • test/internal/source-lints/self-receiver-teardown.test.ts

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author

Status: ready for review. Rebased onto main after #37716 landed (it converted an allowlisted site, so the ratchet test failed on the merge commit and that entry is now deleted); the src change is unchanged in substance since the first push, which was green in builds #92668 and #92805.

Reproduced how: the lint run with the two src files at their main versions reports exactly src/bundler/ThreadPool.rs:590: Self::deinit(std::ptr::from_mut::<Self>(self)) and passes with this branch; a standalone Miri reduction of the shape fails on both branches under Tree Borrows (the strongly protected tag disallows deallocations, pointing at fn deinit_soon(&mut self)) and passes with the raw-pointer receiver. Details in the description.

The lint file is shared with #37693 and #37705 (same body, each copy deletes its own allowlist entry); whichever lands first, the others drop their entry on rebase. Two same-shape findings from reviewing this (dns.rs on_cares_complete callers, and Thread::pop(&mut self) in the threading pool) are being handled separately.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this and found no issues. The receiver-shape change is mechanical and mirrors schedule_with_options in the same file, and the new source-lint follows the sibling lints' conventions exactly. Since it's an aliasing-model soundness fix in unsafe cross-thread teardown code and introduces a new tree-wide ratchet lint, a maintainer glance would still be worthwhile.

What was reviewed:

  • deinit_soon body: (*this).thread read is Copy (Option<ParentRef<_>>), &raw mut (*this).deinit_task matches push_idle_task(&self, *mut Task), and nothing touches *this after publish/free.
  • Sole caller in bundle_v2.rsfor &worker in assignments.values() correctly destructures the map's *mut Worker; no other deinit_soon callers exist.
  • Lint allowlist counts (copy_file.rs:2, lifecycle_script_runner.rs:5, node_fs.rs:2) match the tree; globAllSources().rust, tracked-set guard, and ratchet shape copied from unsound-erased-box.test.ts / dead-code-escapes.test.ts.
Extended reasoning...

Overview

Three files: src/bundler/ThreadPool.rs converts Worker::deinit_soon(&mut self) to unsafe fn deinit_soon(this: *mut Self) (~30 lines including doc comment); src/bundler/bundle_v2.rs updates the single caller to pass the raw *mut Worker it already held in workers_assignments; test/internal/source-lints/self-receiver-teardown.test.ts is a new regex-based ratchet lint banning deinit(..)/destroy(..) calls whose argument is self spelled as a raw pointer.

Security risks

None. No user-facing input handling, no auth/crypto/permissions. This is an internal soundness fix under the Rust aliasing models (Stacked/Tree Borrows) — deallocating memory while a protected &mut self is live is UB. No observable behavior change.

Level of scrutiny

High — this is unsafe Rust in the bundler worker-pool teardown path, executed on every Bun.build() / bun build. REVIEW.md flags native memory safety as the most-blocked category. That said, the transformation itself is mechanical: the body performs the same two operations (read thread, then publish deinit_task or call deinit) via raw place expressions instead of through a &mut self reborrow. It follows the exact precedent of schedule_with_options in the same file and PostgresSQLConnection::deinit. I verified ParentRef is Copy (parent_ref.rs:169) so Option<ParentRef<Thread>> is Copy and the (*this).thread read is a value copy that ends before any free; push_idle_task takes (&self, *mut Task) so no reference to *this is formed there either.

Other factors

  • The new lint test copies its scaffolding (globAllSources().rust, git-tracked filter, realpath dedup, comment stripping, ratchet allowlist) from existing sibling lints in test/internal/source-lints/. The regex has a positive/negative example test guarding it. I ran the pattern against src/ with PCRE2 and confirmed the allowlist counts (2/5/2) match exactly and ThreadPool.rs no longer matches.
  • Only one deinit_soon caller exists in the tree; grep confirmed.
  • The PR description includes Miri output for both branches under both aliasing models and verification via debug+ASAN bundler tests.
  • Deferring rather than approving because (a) unsafe cross-thread teardown reasoning benefits from a second pair of eyes even when mechanical, and (b) the new tree-wide lint introduces an allowlist that a maintainer should acknowledge as something they want to maintain.

@robobun

robobun commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author

Heads-up on the lint: #37693 (the node_fs.rs conversion this allowlist points at) adds test/internal/source-lints/self-receiver-teardown.test.ts at the same path. Same design, plus a second pattern for the scopeguard::guard(ptr::from_mut(self), |p| Self::destroy(p)) form (which is what UVFSRequest::run_from_js_thread had, so node_fs.rs has three sites on main, not two), and it allowlists src/bundler/ThreadPool.rs at 1 pointing here.

The identical path is deliberate: whichever of the two PRs lands second gets a conflict on the file rather than landing a ratchet entry that is already stale, and resolves it by keeping one copy and deleting the entry for the conversion that has landed. If this one lands first, I will rebase #37693 onto it that way.

@robobun

robobun commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 7:47 AM PT - Aug 12th, 2026

@robobun, your commit 5c9febbb0f4908480a2351318bbe9afbde10a8be passed in Build #93038! 🎉


🧪   To try this PR locally:

bunx bun-pr 37685

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

bun-37685 --bun

Comment thread src/bundler/ThreadPool.rs Outdated
Comment thread src/bundler/ThreadPool.rs Outdated
Comment thread src/bundler/ThreadPool.rs

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this PR and didn't find any bugs. The &mut self*mut Self conversion is mechanical and matches the tree's established pattern (schedule_with_options in the same file, PostgresSQLConnection::deinit), and the sole caller is updated correctly. Because it touches unsafe Rust in the bundler's concurrent worker-teardown path and adds a new tree-wide source lint with an allowlist coupled to three other in-flight PRs, a human look would still be worthwhile.

What was reviewed:

  • deinit_soon: verified Option<ParentRef<Thread>> is Copy (parent_ref.rs:169), so the (*this).thread read is a value copy that ends before the publish; the &raw mut (*this).deinit_task projection carries the allocation's provenance for from_field_ptr!.
  • bundle_v2.rs: confirmed this is the only deinit_soon caller; for &worker correctly destructures the map's *mut Worker and assignments.clear_and_free() still runs after the loop.
  • Lint file: pattern self-tests, non-vacuous scanned > 0 guard, ratchet direction, and the src/cli symlink / tracked-file filters match sibling lints in test/internal/source-lints/.
Extended reasoning...

Overview

This PR converts Worker::deinit_soon(&mut self) in src/bundler/ThreadPool.rs to unsafe fn deinit_soon(this: *mut Self), updates its single caller in src/bundler/bundle_v2.rs, and adds a new source-lint test (test/internal/source-lints/self-receiver-teardown.test.ts) that bans the Self::destroy(ptr::from_mut(self)) shape tree-wide with a ratcheted allowlist.

The motivation is a Stacked/Tree Borrows soundness issue: a &mut self argument is protected for the whole call, and both branches of the old body could deallocate the Worker while that protector is live (the else branch synchronously via heap::take, the if branch via a pool thread that can drain idle tasks between batches before deinit_soon returns). The PR description includes a Miri reproduction under both aliasing models.

Security risks

None. This is an internal aliasing-model soundness fix in bundler teardown; no user input handling, auth, crypto, or network surface is touched.

Level of scrutiny

High. Per REVIEW.md, native memory safety is the most-blocked review category, and this is unsafe Rust in a concurrent teardown path where a pool thread can free the object mid-call. The Rust change itself is small and mechanical — it mirrors the same conversion already applied to schedule_with_options in this file and to PostgresSQLConnection::deinit — but the reasoning about provenance, protectors, and idle-task drain timing is exactly the kind of thing that benefits from a maintainer's eyes.

The new 232-line source lint is a separate axis: it introduces tree-wide regex enforcement with an allowlist keyed to exact per-file counts, cross-referenced against four other PRs (#37551, #37693, #37705, and the ArrayBufferSink trait case). The design (scope boundaries, what's deliberately excluded, how the three copies of this file across sibling PRs will merge) is a maintainer-level decision.

Other factors

  • I verified ParentRef is Copy (src/ptr/parent_ref.rs:169), so the SAFETY claim on the (*this).thread read is accurate.
  • Confirmed bundle_v2.rs:5022 is the only deinit_soon call site in the tree; the for &worker in assignments.values() destructure yields the raw *mut Worker directly, and the map is still cleared afterwards.
  • The lint file follows the conventions of the 17 existing sibling files in test/internal/source-lints/ (globAllSources, git-tracked filter, realpath symlink dedup, positive/negative pattern self-test, ratchet test).
  • The comment-cop bot fired three times on the doc comment; the author trimmed it and the final form (one-sentence receiver note + # Safety contract) matches deinit directly below it and schedule_with_options above. All three inline threads are resolved.
  • No prior claude[bot] reviews on this PR; first CI build (#92668) was reported green with subsequent pushes touching only comments and the lint file.

Jarred-Sumner added a commit that referenced this pull request Aug 12, 2026
### Problem

The JSSink finalize chain frees the sink while reference arguments to it
are still live. At `3fc747a7da`:

* generated thunk `extern "C" fn ${name}__finalize(this: &mut ${name})`
(src/codegen/generate-jssink.ts), called from `~JS${name}`,
`~JSReadable${name}Controller` and `${name}__doClose`
* `JSSink::js_finalize(this: &mut T)` (src/runtime/webcore/Sink.rs)
* `JsSinkType::finalize(&mut self)` (src/runtime/webcore/Sink.rs), whose
impls do the actual release:
* `ArrayBufferSink` (src/runtime/webcore/ArrayBufferSink.rs):
`Self::finalize(ptr::from_mut(self))` -> `destroy` -> `heap::take`,
unconditionally. The comment on the impl said the C export owned the
free; this call is the free.
* `FileSink` (src/runtime/webcore/FileSink.rs): the inherent
`finalize(&mut self)` ends in `FileSink::deref(ptr::from_mut(self))`,
which runs `deinit` -> `heap::take` whenever the wrapper's +1 was the
last ref, i.e. on an ordinary GC sweep of a sink nothing else holds. The
header comment argued this was fine because the `&mut` carries write
provenance, which is true but is not the problem.
* `FetchRequestBodySink`
(src/runtime/webcore/fetch/FetchRequestBodySink.rs): drops the tasklet
ref taken in `start_request_stream`. The tasklet owns the sink
allocation, so if that ref is the last one, `FetchTasklet::deinit` ->
`clear_data` -> `clear_sink` -> `heap::take(sink)` frees `*self` inside
the call. That is the fallback path for a pump that never settled; it is
reachable at least on worker teardown: phase B of `VirtualMachine`
teardown releases the aborted fetch's other refs on the tasklet, and
phase C then destroys the heap, sweeping the controller with `m_sinkPtr`
still set because `JSSinkController__onClose` does not run the detaching
JS callback once termination is pending.
* `HTTPServerWritable`, `NetworkSink` and `RewriterPipe` do not free
anything here (their allocations are owned by the `RequestContext`, the
S3 wrapper and the pipe's own refcount respectively).

A reference passed as an argument has to stay dereferenceable until the
call returns. Freeing it from inside the call is undefined behaviour
under both aliasing models whether or not the reference is used again
(Stacked Borrows: `deallocating while item is strongly protected`; Tree
Borrows, which `bun run rust:miri` uses, rejects it the same way), and
that protector is the model behind the `dereferenceable` attribute rustc
puts on every `&`/`&mut` argument, so the optimizer may legitimately
move a load through any of the three frames past the free. No crash is
known from this; ASAN only has something to catch if the optimizer
actually takes that liberty, which the unoptimized debug build never
does, so it is not observable as a runtime test. Same family as #37672,
#37681, #37685, #37693, #37705 and #37551; #37705's description leaves
this chain out explicitly because it needs a change to the generated
thunk.

### Fix

The whole chain takes the raw pointer, which is what the C++ side has
anyway (`void* m_sinkPtr`):

* generate-jssink.ts emits `pub unsafe extern "C" fn
${name}__finalize(this: *mut ${name})` forwarding to `js_finalize`; the
ABI is unchanged, so JSSink.cpp is untouched.
* `JSSink::js_finalize(this: *mut T)` forwards to the trait.
* `JsSinkType::finalize` becomes `unsafe fn finalize(this: *mut Self)`,
documented as "the cell is giving up its claim; this may free the sink",
the same shape as `HTTPServerWritable::abort(this: *mut Self)` and the
FileSink PipeWriter callbacks.
* The three freeing impls release through the pointer without forming a
reference to the allocation: `ArrayBufferSink` calls `destroy` directly
(the inherent `finalize` wrapper, whose only caller was the trait impl,
is deleted); `FileSink::finalize(this: *mut FileSink)` keeps the same
body with per-statement `(*this).field` access, like `on_close` in the
same file (the file header no longer claims the `&mut` version was
sound; the rationale lives once, on the trait method);
`FetchRequestBodySink::finalize(this: *mut Self)` takes `task` out
through the pointer and does not touch it after the deref.
* `HTTPServerWritable` and `NetworkSink` reborrow inside their own impl
to call the unchanged inherent `finalize(&mut self)`; that borrow ends
before the impl returns and nothing under it frees, which the SAFETY
comments state. `RewriterPipe`'s impl stays empty.

Every impl performs the same operations in the same order as before; the
only thing that moves is the type the pointer travels as.
`js_controller_detached`, `js_close` and `js_end_with_sink` still take
`&mut`: nothing frees under them (the `controller_detached` contract on
the trait already requires deferring a last-owner free for that reason).
`FileSink::assign_to_stream`'s `FileSinkRef` guard also derefs from a
`&mut self` frame, but its ref is balanced against one it took itself
and every caller (subprocess stdin setup) holds its own ref across the
call, so it can never be the one that frees; left alone. Sites with the
same shape outside this chain
(`S3UploadStreamWrapper::handle_{resolve,reject}_stream`,
`FetchTasklet::write_end_request`) are not sink frames and are reported
separately.

### Tests

test/internal/source-lints/jssink-finalize-raw-ptr.test.ts scans every
`impl ... JsSinkType for ...` block for a `finalize` item and requires
`unsafe fn finalize(<ident>: *mut Self)`, checks the other frames by
signature (trait declaration, `js_finalize`, the codegen template, and
the three inherent methods that perform the free, which `pub` tells
apart from the trait impls in the same files), and checks its own
patterns against positive and negative spellings. With src/ restored to
`main` it reports:

```
src/runtime/api/html_rewriter.rs:1650: impl JsSinkType for RewriterPipe: fn finalize(&mut self) (line 1661)
src/runtime/webcore/ArrayBufferSink.rs:213: impl JsSinkType for ArrayBufferSink: fn finalize(&mut self) (line 221)
src/runtime/webcore/fetch/FetchRequestBodySink.rs:274: impl JsSinkType for FetchRequestBodySink: fn finalize(&mut self) (line 281)
src/runtime/webcore/FileSink.rs:1283: impl JsSinkType for FileSink: fn finalize(&mut self) (line 1294)
src/runtime/webcore/streams.rs:2104: impl JsSinkType for HTTPServerWritable: fn finalize(&mut self) (line 2119)
src/runtime/webcore/streams.rs:2523: impl JsSinkType for NetworkSink: fn finalize(&mut self) (line 2530)
src/runtime/webcore/Sink.rs: JsSinkType::finalize declaration does not take the sink as `*mut`
src/runtime/webcore/Sink.rs: JSSink::js_finalize does not take the sink as `*mut`
src/codegen/generate-jssink.ts: generated `${name}__finalize` thunk does not take the sink as `*mut`
src/runtime/webcore/FileSink.rs: FileSink::finalize does not take the sink as `*mut`
src/runtime/webcore/fetch/FetchRequestBodySink.rs: FetchRequestBodySink::finalize does not take the sink as `*mut`
```

(`ArrayBufferSink::destroy` already took `*mut` on `main`; its entry is
a ratchet.)

The behaviour itself is the existing coverage of each finalize path; see
below.

### Verification

Debug (ASAN) build on Linux: `cargo clippy -p bun_runtime` and `rustfmt
--check` on the touched files are clean; the generated thunks have the
new signature. Passing: test/internal/source-lints/ (all 18 files),
test/js/bun/util/arraybuffersink.test.ts and filesink.test.ts (wrapper
sweep and prototype `.close()` for the two Box/refcount sinks),
test/js/bun/spawn/spawn.test.ts (stdin `FileSink` via
`assign_to_stream`), test/js/web/fetch/body-stream.test.ts,
fetch-abort-stream-body.test.ts and fetch-stream-cancel-leak.test.ts
(`FetchRequestBodySink`),
test/js/bun/http/serve-response-stream-sink-leak,
serve-direct-readable-stream, serve-stream-reject-flush-leak and
serve-async-stream-client-abort (`HTTPServerWritable` controller
teardown), test/js/web/fetch/server-response-stream-leak.test.ts,
test/js/web/streams/streams.test.js,
test/js/workerd/html-rewriter.test.js and html-rewriter-leak.test.ts
(`RewriterPipe`), test/js/bun/s3/s3-stream-error-gc.test.ts and
s3-argument-validation.test.ts. The S3 upload tests that would drive
`NetworkSink` (s3.test.ts, s3-storage-class.test.ts) cannot connect from
this environment and fail identically on the released binary, so that
impl (a one-line forward to the unchanged inherent method) is left to
CI.

Overlap with the sibling lints, each of which documents these sites as
tracked separately: #37685 / #37693 / #37705 add
`self-receiver-teardown.test.ts` with
`src/runtime/webcore/ArrayBufferSink.rs: 1` allowlisted for the
`Self::finalize(ptr::from_mut(self))` line this PR removes, and #37703
adds `self-receiver-release.test.ts` with
`src/runtime/webcore/FileSink.rs: 2` allowlisted for the two derefs
inside the old `FileSink::finalize(&mut self)` (running that lint
against this branch reports FileSink.rs at 0). Whichever side lands
second deletes the entry; nothing else conflicts (#37703's
FetchRequestBodySink.rs hunk is `end_from_stream`, a different
function). #34999 and #35528 edit the body of `FileSink::finalize`
textually but keep the receiver.

---------

Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this and didn't find any bugs. The &mut self*mut Self receiver conversion is sound and follows the same pattern as schedule_with_options in the same file, and the sole caller is updated correctly. A human look would still be worthwhile for the new tree-wide source lint being added — it establishes an allowlist ratchet coordinated across three other in-flight PRs (#37551, #37693, #37705), which is a process decision worth a maintainer's sign-off.

What was reviewed:

  • deinit_soon rewrite: verified Option<ParentRef<_>> is Copy (src/ptr/parent_ref.rs:169), so the (*this).thread read is a plain copy; the &raw mut (*this).deinit_task projection carries whole-allocation provenance for from_field_ptr!.
  • Confirmed bundle_v2.rs is the only deinit_soon caller and that assignments.clear_and_free() follows the loop, matching the new SAFETY contract.
  • The lint file mirrors the sibling fn-long-mut-reborrow.test.ts structure (same globAllSources/tracked-set/realpath filters), and the pattern self-test guards against vacuous matches.
Extended reasoning...

Overview

The PR converts Worker::deinit_soon(&mut self) in src/bundler/ThreadPool.rs to unsafe fn deinit_soon(this: *mut Self), updates its sole caller in src/bundler/bundle_v2.rs to pass the raw *mut Worker stored in workers_assignments, and adds a new source lint (test/internal/source-lints/self-receiver-teardown.test.ts) that bans the Self::deinit(ptr::from_mut(self)) shape tree-wide with a documented allowlist ratchet.

The Rust change is small and mechanical: it removes a Stacked/Tree Borrows violation where the &mut self argument's protector tag outlives the deallocation of *self (either synchronously via Self::deinit in the else branch, or on another thread once push_idle_task publishes the intrusive task in the if branch). The fix reads thread and projects deinit_task through statement-scoped raw place expressions off the caller-supplied allocation pointer, so no reference exists when the free happens and from_field_ptr! in deinit_callback receives whole-allocation provenance. I verified ParentRef is Copy, so the (*this).thread read is sound; and confirmed via grep that bundle_v2.rs:5022 is the only call site.

Security risks

None. This is an internal aliasing-model correctness fix in bundler worker teardown; no user-facing surface, no input parsing, no auth/crypto.

Level of scrutiny

High — this is memory-safety code on a cross-thread teardown path in the bundler, exactly the category REVIEW.md flags as most-blocked. That said, the change is a strict narrowing: runtime behavior (code path, ordering, which thread frees what) is identical; only the Rust-level receiver type changes. The pattern is already established in the same file (schedule_with_options takes *mut ParseTask for the same publish-then-free reason) and elsewhere (PostgresSQLConnection::deinit), and the PR description includes a Miri reduction demonstrating the before/after under both borrow models.

Other factors

The reason I'm deferring rather than approving is the new source-lint file. It is well-constructed (mirrors sibling lints, has a regex self-test with positive/negative fixtures, a non-vacuous scan guard, and a ratchet assertion), but it introduces a tree-wide enforcement mechanism with an allowlist referencing three other in-flight PRs that intentionally add the same file at the same path so whichever lands second conflicts and rebases. That coordination plan and the choice to add this class of lint are process/design decisions a maintainer should acknowledge, even though I found nothing wrong with the implementation.

The comment-cop bot fired three times on earlier revisions about paragraph-length comments; the author trimmed them and all threads are resolved. The current doc comment is a # Safety contract plus one sentence on the receiver choice, consistent with deinit directly below it.

…&mut self

Worker::deinit_soon frees the Worker it is called on, either directly (the
off-pool branch calls Worker::deinit, which reclaims the Box) or by pushing
the intrusive deinit_task to the pool thread, which may run deinit_callback
and free the Worker before deinit_soon returns. With a &mut self receiver
that reference is a protected argument for the whole call, and freeing the
allocation it points into while it is protected is UB under both Stacked
and Tree Borrows. The field pointer handed to the pool was also derived from
the &mut self reborrow rather than from the allocation's own pointer, which
is what deinit_callback's container_of recovery requires.

deinit_soon is now `unsafe fn deinit_soon(this: *mut Self)`: it reads
`thread` and takes the deinit_task address through statement-scoped raw
place expressions and passes `this` itself to deinit. The only caller, the
worker teardown loop in BundleV2::deinit_without_freeing_arena, passes the
pointer stored in workers_assignments.

Add a source lint banning deinit(..)/destroy(..) calls whose argument is
`self` spelled as a raw pointer, with a ratcheted allowlist for the three
files that still carry the shape.
…37705

Same lint body as the other two PRs converting sites of this shape (adds
the scopeguard deferred form and finalize as a callee), so each PR's copy
differs only in which allowlist entry it deletes. This copy drops the
ThreadPool.rs entry and keeps node_fs.rs, copy_file.rs and read_file.rs
at their current counts.
Match nested turbofish, a reborrow of the receiver inside the pointer
constructor, and a parenthesized cast; name the file when the ratchet
fails; state the name-list boundary and the bare-self coercion gap in the
header instead of claiming the list is exhaustive.
@robobun
robobun force-pushed the farm/cf508add/bundler-worker-deinit-soon-raw-receiver branch from 0b34ae6 to 5c9febb Compare August 12, 2026 09:16

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this PR and didn't find any bugs. The &mut self*mut Self receiver conversion is mechanical and follows the same pattern as schedule_with_options in the same file, and the single caller in bundle_v2.rs is correctly updated. Because it's unsafe Rust in the bundler's cross-thread worker-teardown path and adds a new source lint whose allowlist coordinates with three other in-flight PRs, a human look would still be worthwhile.

Checked: ParentRef is Copy (src/ptr/parent_ref.rs:169) so the (*this).thread read is sound; push_idle_task takes *mut Task so &raw mut (*this).deinit_task matches; grep confirms bundle_v2.rs:5022 is the only deinit_soon caller; the lint mirrors self-receiver-reclaim.test.ts's tracked-file/realpath/non-empty-scan guards.

Extended reasoning...

Overview

Three files: src/bundler/ThreadPool.rs converts Worker::deinit_soon(&mut self) to unsafe fn deinit_soon(this: *mut Self) with statement-scoped raw place expressions; src/bundler/bundle_v2.rs updates the sole caller in the workers_assignments teardown loop to pass the stored *mut Worker directly; test/internal/source-lints/self-receiver-teardown.test.ts is a new 245-line regex-based lint banning destroy/deinit/finalize calls whose argument is self spelled as a raw pointer, with pattern self-tests and a ratcheted allowlist.

Security risks

None. No user-facing surface, no parsing of untrusted input, no auth/crypto/permissions. This is an internal aliasing-model soundness fix in bundler teardown.

Level of scrutiny

High. This is unsafe Rust in the bundler thread-pool worker teardown path, which runs on every Bun.build() / bun build. The if branch publishes an intrusive task to a pool thread that may free the Worker before deinit_soon returns — exactly the class REVIEW.md calls out under "Never let a pointer or slice outlive the memory it points into" and "Know the thread affinity of every line you touch." The change itself is strictly UB-reducing (a protected &mut self becomes a raw pointer with no protector; the caller no longer forms &mut **worker), and it mirrors schedule_with_options(this: *mut ParseTask) in the same file for the same publish-then-free reason. But the aliasing-model reasoning (Stacked vs Tree Borrows provenance for the from_field_ptr! reclaim) is subtle enough that a maintainer familiar with the tree's Miri conventions should confirm.

Other factors

  • Verified ParentRef<T, P> is Copy (src/ptr/parent_ref.rs:169), so let thread = unsafe { (*this).thread }; is a valid by-value read of Option<ParentRef<Thread>>.
  • push_idle_task(&self, task: *mut Task) (src/threading/ThreadPool.rs:1123) — the projected &raw mut (*this).deinit_task matches.
  • Grep confirms bundle_v2.rs:5022 is the only deinit_soon call site; workers_assignments is ArrayHashMap<ThreadId, *mut Worker> so for &worker in assignments.values() yields the raw pointer directly.
  • The lint follows sibling conventions exactly (same git ls-tree tracked-file guard, realpathSync dedup, and non-empty-scan test as self-receiver-reclaim.test.ts). Its allowlist references four files across three in-flight PRs (#37551, #37693, #37705) with an explicit add/add-conflict merge plan; a human should confirm that coordination is acceptable.
  • The comment-cop bot fired three times on earlier revisions; the author trimmed the doc comment down to a # Safety contract plus a one-line receiver note (matching deinit directly below), and those threads are resolved.
  • PR description reports ASAN debug tests (bun-build-api.test.ts, bundler_plugin.test.ts) and the full test/internal/source-lints/ suite passing, plus clippy/fmt clean; builds #92668 and #92805 were green per the status comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant