bundler: hand a finished Bun.build back through its pointer, not a &mut receiver - #37723
bundler: hand a finished Bun.build back through its pointer, not a &mut receiver#37723robobun wants to merge 4 commits into
Conversation
…ut receiver complete_on_bundle_thread posts the completion task's only ref to the JS thread, which frees the task as soon as the post lands. It took &mut self, and BundleThread held the task as &mut C across the post as well, so the allocation could be freed while reference arguments to it were still live. The trait method now takes *mut Self and BundleThread keeps the dequeued pointer raw, reborrowing it per call; nothing forms a reference to the task across either hand-back. Adds a source lint banning posting a pointer spelled from self as a task, with the remaining same-shaped sites ratcheted.
|
Warning Review limit reached
Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
|
Updated 12:05 PM PT - Aug 12th, 2026
❌ @robobun, your commit 2ba0888 has some failures in 🧪 To try this PR locally: bunx bun-pr 37723That installs a local version of the PR into your bun-37723 --bun |
|
Status: ready for review; nothing further planned from this side. Reproduced as a standalone Miri reduction of the hand-back shape (bundle thread posts from Verified on the debug (ASAN) build with test/bundler/bun-build-api.test.ts, the plugin/defer tests, metafile.test.ts, bundler_html_server.test.ts and bun-serve-html.test.ts; details in the description. Since the first push: the lint's header also states the same-thread (provenance) hazard and lists the sites its regex cannot see, each allowlist entry names the PR converting it (#37732, #37709, #37741, #37762), and the contract comments were shortened. Merge order with those PRs and with #37740 is in the description: whichever lands second rebases, and the ratchet test points at any entry that has to go. CI: build 93236 (final push, 2ba0888) has no failing tests: 192 jobs passed and the two darwin 26 aarch64 test jobs expired unrun waiting for an agent; those lanes ran and passed on build 92775 for the same source change (the pushes in between touch only comments and the lint file, which the Buildkite lanes do not run). 92775's one red, test-tonic on darwin 14, fails and passes on retry on main as well. Not retriggering for the expired jobs. |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it restructures cross-thread unsafe ownership hand-back in the Bun.build() completion path (aliasing-model protectors, statement-scoped reborrows, raw-pointer receivers) and encodes an allowlist of five other sites in a new source-lint, a human look would still be worthwhile.
What was reviewed:
- Order of operations in
generate_in_new_threadis unchanged (post-then-teardown on success,set_result-then-post on error); each(*completion)reborrow ends at its;before the hand-back. - Confirmed
Transpiler.logis*mut Log(transpiler.rs:124) —drop_in_place(transpiler_ptr)after the post has no drop glue that dereferences into the possibly-freed task. - The new lint follows
self-receiver-reclaim.test.tsconventions exactly (tracked-file filter, realpath dedup, ratcheted allowlist, positive/negative self-checks, non-vacuous scan guard).
Extended reasoning...
Overview
This PR converts CompletionStruct::complete_on_bundle_thread from &mut self to unsafe fn(this: *mut Self), and threads the raw pointer through BundleThread::thread_main → generate_in_new_thread so no &mut C reference to the completion task is live across the point where it is posted to the JS thread (which may free it). The impl in js_bundle_completion_task.rs reads bundle_loop/loop_handle through statement-scoped raw accesses before posting this. A new source-lint (self-receiver-publish.test.ts) bans posting a self-derived pointer via Task::init / create_from / from_callback, with a ratcheted allowlist for five documented sites (bundle_v2.rs ×2, DeferredBatchTask.rs, napi_body.rs ×2).
Security risks
None user-facing. The change is a Rust aliasing-model soundness fix (protected reference freed by another thread mid-call). No input parsing, auth, or crypto is touched.
Level of scrutiny
High. This is unsafe Rust in the cross-thread hand-back that runs on every Bun.build(). The transformation is mechanical (reference → raw pointer with statement-scoped reborrows) and follows in-tree precedent (async_job_run, post_job; same class as #37703/#37685), but the correctness relies on subtle claims: that no reborrow of *completion outlives its statement, that drop_in_place(transpiler_ptr) after the post cannot reach the task's memory (verified: log is a raw *mut), and that the allowlisted sites in the lint are correctly characterised as not-freeing / same-thread / blocked-on-callers.
Other factors
- Bug hunter found nothing. I checked the two paths independently and the order of operations matches the pre-PR code.
- The lint test mirrors
self-receiver-reclaim.test.tsstructure closely and self-checks its regexes against 18 banned and 15 allowed examples plus a non-empty-scan guard. - The PR description is thorough (Miri reduction under both aliasing models, extensive test list, explicit scoping of what is left for follow-ups).
- No CODEOWNERS on these paths; no prior human reviewer comments to address.
Deferring because memory-safety-critical unsafe Rust across threads warrants human eyes even when the automated pass is clean.
|
The two |
|
Follow-up on the |
…-receiver-publish cannot see Pin create_from/from_callback to ConcurrentTask paths, add the intrusive .from(..) spelling, state the same-thread (provenance) hazard that makes same-loop posts part of the shape, list the same hand-over spelled outside the regex, and correct the napi allowlist note: the finalizer post's consumer does free the receiver.
There was a problem hiding this comment.
I reviewed this PR and the automated bug hunt found no issues. Because it rewires the cross-thread ownership hand-back for every Bun.build() — turning &mut self into raw-pointer receivers with new statement-scoped reborrows and a post-then-teardown ordering that depends on Transpiler's drop glue not reaching the completion's log — a human look at the unsafe reasoning would still be worthwhile.
What was reviewed
generate_in_new_threadordering: confirmed the arena teardown aftercomplete_on_bundle_threadonly touches bump-allocated memory (transpiler_ptr,ast_memory_store), not*completion; thetranspiler.lograw pointer into(*completion).logdangles after the post but has no drop glue per the PR's claim.- Error path in
thread_main: the(*completion).set_result(..)reborrow ends beforeC::complete_on_bundle_thread(completion), matching the trait contract. - The source-lint's regex patterns against its own positive/negative examples and the ratchet entries; the
bundle_v2.rs: 2entry's comment does not yet name #37732 as the 12:24 follow-up asked.
Extended reasoning...
Overview
This PR fixes a Rust aliasing-model violation in the Bun.build() completion hand-back: JSBundleCompletionTask::complete_on_bundle_thread(&mut self) posted a pointer derived from self to the JS thread, which adopts the task's only ref and may free it while the &mut self (and the caller's &mut C in generate_in_new_thread / thread_main) is still a live protected argument. The fix converts complete_on_bundle_thread and generate_in_new_thread to take *mut Self / *mut C, with each trait call reborrowing through a statement-scoped (*completion).method() so no reference is live at the post. A new source-lint test bans the Task::init(from_mut(self)) pattern tree-wide with a documented ratchet for the four remaining sites.
Security risks
None in the traditional sense; this is a memory-safety soundness fix under Tree/Stacked Borrows (demonstrated via Miri reduction), not a new attack surface. The change strictly narrows what is UB.
Level of scrutiny
High. This is unsafe Rust in the cross-thread ownership hand-back for every Bun.build() — REVIEW.md's most-blocked category. The reasoning is subtle: after the post, *completion may be freed on another thread, and the function continues to run ast_memory_store.pop() and drop_in_place(transpiler_ptr). That is only sound because Transpiler holds log (which points into (*completion).log) as a raw pointer with no drop glue, an invariant the PR asserts but which a reviewer should verify against Transpiler's actual Drop impl. The success-path ordering (post before arena teardown) is preserved from before, so this is not a new hazard, but the raw-pointer conversion makes the dependency explicit.
Other factors
The PR is thoroughly argued (Miri reduction, ASAN test runs across the bundler suite including the thousands-of-builds stress test, clippy/fmt clean, CI green modulo an unrelated darwin flake). The comment-cop bot flagged the SAFETY comments and the author shortened them in c265bf9; those threads are resolved. There's a self-noted follow-up (12:24Z) asking the bundle_v2.rs ratchet entry to name #37732 — the current diff's entry does not, which is a minor doc nit. Given the depth of unsafe reasoning required and the hot-path nature of the code, I'm deferring rather than approving.
…ption The embedded-task .from() spelling is covered by the sibling lint in #37750, so this one goes back to the heap-task constructors only.
|
Done in 2ba0888: the |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it restructures the cross-thread ownership hand-off that every Bun.build() goes through — turning &mut self receivers into raw *mut with statement-scoped reborrows across generate_in_new_thread, thread_main, and the trait impl — a human look at the aliasing/lifetime reasoning would still be worthwhile.
What was reviewed:
- Confirmed no access to
*completionsurvives pastcomplete_on_bundle_threadon either the success or error arm; the post-hand-back teardown (drop_in_place(transpiler_ptr),ast_memory_store.pop()) only touches arena/bundle-thread memory, andTranspiler.logis a raw*mutwith no drop glue so the completion'slogfield is not reached. - Checked the new impl reads
bundle_loop/loop_handleout before postingthis, and only the localhandleclone is used afterwards. - Verified the source-lint test's ratchet entries name their converting PRs (the 12:24 follow-up ask re: #37732 is addressed), the pattern self-test covers the spellings, and
scripts/glob-sources.tsexportsglobAllSources.
Extended reasoning...
Overview
This PR converts CompletionStruct::complete_on_bundle_thread from &mut self to unsafe fn(this: *mut Self) and threads that change up through BundleThread::generate_in_new_thread (now takes *mut C) and thread_main (no longer materializes &mut C). The motivation is a Miri-reproduced aliasing-model violation: the bundle thread posts the task's only ref to the JS thread, which may free it while a &mut self argument is still protected on the bundle thread's stack. A new source-lint (self-receiver-publish.test.ts) bans the pattern tree-wide with a ratcheted allowlist for four sites being converted in sibling PRs.
Security risks
None. This is an internal ownership/aliasing correctness change with no user-facing surface, no parsing of untrusted input, and no security-sensitive paths.
Level of scrutiny
High. This is unsafe Rust on a hot cross-thread path (every Bun.build()) whose correctness rests on non-local invariants: that each (*completion).method() reborrow ends with its call, that the returned &'a mut Transpiler borrows the arena rather than the task, that Transpiler::drop (run via drop_in_place after the post) does not reach into the completion (its log field is a raw *mut, confirmed at src/bundler/transpiler.rs:124), and that nothing below the hand-back touches *completion. I traced each of these and they hold, and the change follows the same shape as async_job_run / post_job cited in the description — but this is exactly the category REVIEW.md flags for careful maintainer sign-off ("Never let a pointer or slice outlive the memory it points into", "Reference counts provably balanced on every terminal path").
Other factors
The PR is thoroughly verified (Miri reduction under both aliasing models, ASAN debug build across the bundler suite including the thousands-of-builds test, the lint fails on main at exactly the fixed site). The comment-cop bot threads are all resolved (comments were shortened in c265bf9 to the required # Safety / SAFETY: contracts). The 12:24 robobun follow-up asked for the bundle_v2.rs allowlist entry to name #37732, which it now does. There is a coordination note that #37732 will delete that entry when it lands second. Nothing here blocks; the deferral is purely because the unsafe reasoning is subtle enough to merit a maintainer's eyes.
Problem
Bun.build()ends with the bundle thread posting the finished task to the JS thread from insidecomplete_on_bundle_thread(&mut self). The post carries the task's only ref, so the JS thread can free the task while that&mut self, and the&mutits caller holds, are still live arguments.the strongly protected tag disallows deallocations(Tree Borrows) anddeallocating while item [Unique] is strongly protected(Stacked Borrows).Fix
complete_on_bundle_threadtakesthis: *mut Selfinstead of&mut self. It reads the two fields it needs through raw accesses that end before the post, then poststhis, the same shape as the existing zlib and VmHandle posts.&mut. Each trait call gets a reborrow that ends with the call, so on both the success and the error path no reference to the task exists on this thread when the post happens. Order of operations is unchanged.selfas a heap task. Onmainit reports exactly this one site; five other sites it can see are allowlisted by count, each naming the PR that converts it.throw: falseand throwing arms 50 times each pass on the ASAN build.Background
Bun.build()runs on a dedicated bundle thread. Each build is a heap-allocated completion task: the JS thread enqueues it, the bundle thread dequeues it as a raw pointer, runs the build, stores the result and log on it, and hands it back.ConcurrentTaskposted to the JS thread's event loop. The task has one ref from creation, the post carries it, and the JS-side handler adopts and releases it, which frees the task. The post is therefore the bundle thread's last legal touch of the object.bun run rust:miriuses Tree Borrows.test/internal/source-lints/holds bun tests that regex-scan the Rust sources for banned spellings, with a per-file allowlist of exact counts. The count must match exactly, so converting a site forces its entry down and a new site fails outright.[review] gate passed · iteration 0 · 3 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
Original description
Problem
JSBundleCompletionTask::complete_on_bundle_thread(&mut self)(src/runtime/api/js_bundle_completion_task.rs) is how the bundle thread hands a finishedBun.build()back:The task is created with
ref_count == 1(create_and_schedule_completion_task) and nothing takes another ref, so the post carries the task's only ref. On the JS threadon_complete_anytaskadopts it (ScopedRef::adopt) and releases it on return, which runsdeinitandheap::takes the allocation. The JS thread picks the task up as soon aspost_taskwakes its loop, so on the normal path of every build the allocation can be freed while the bundle thread is still insidecomplete_on_bundle_thread(&mut self), and insideBundleThread::generate_in_new_thread(completion: &mut C, ..)(src/bundler/BundleThread.rs), whose caller inthread_mainalso held the task as&mut Cand calledcompletion.complete_on_bundle_thread()itself on the error arm.A reference argument is protected for the duration of the call it was passed to, and freeing protected memory is UB under both aliasing models regardless of whether the reference is used again (codegen relies on the same thing: the argument is annotated dereferenceable for the whole call). A standalone reduction of exactly this shape, with the consumer on another thread, fails under Miri with Tree Borrows (the model
bun run rust:miriuses):and under Stacked Borrows with
deallocating while item [Unique for <4874>] is strongly protected. The same reduction posting through the raw pointer passes under both. No crash is known from this today (nothing reads the task after the post); the contract is what is wrong. Same class as #37703 (FetchTasklet release) and #37685 (Worker::deinit_soon, which had the same publish-then-freed-elsewhere shape), here across threads.Reduction run under Miri
MIRIFLAGS=-Zmiri-tree-borrows cargo miri run -- beforeand the default Stacked Borrows run both exit 1 with the errors quoted above;-- afterexits 0 under both.Fix
CompletionStruct::complete_on_bundle_threadbecomesunsafe fn complete_on_bundle_thread(this: *mut Self), with the contract (last touch of*thisby the bundle thread; the caller must not hold a reference to it across the call) on the trait. The impl readsbundle_loop/loop_handlethrough statement-scoped raw accesses and poststhisitself, likeasync_job_runin node_zlib_binding.rs andpost_jobin VmHandle.rs already do.BundleThread::thread_mainno longer materializes&mut Cfor the dequeued task;generate_in_new_threadtakes*mut Cand reborrows it for each trait call (create_and_configure_transpiler,init_and_run,set_log; the returned&'a mut Transpilerborrows the arena, not the task), so no reference to the task is live at either hand-back. The order of operations is unchanged: the success path still posts before the arena teardown (which only drops bundle-thread memory;Transpiler/Resolverholdlogas raw pointers with no drop glue), and the error path still posts fromthread_mainafterset_result.The rest of the build's lifetime (the build-long
&mut selftrait calls, and the JS-side field writes that happen after the enqueue) is the same family and is #37740, which overlaps this PR oncomplete_on_bundle_threadand the pointer plumbing; whichever lands second has a small rebase. #35312, #35060 and #35158 touch neighbouring lines ofBundleThread.rsbut none of them changes this receiver.Other sites of this shape were found while writing the lint below and are each their own change rather than part of this one:
Resolve::dispatch/Load::dispatch(#37732),DeferredBatchTask::schedule(#37709 reshapes it), the twoThreadSafeFunctionposts (#37741 addon-thread side, #37762 JS-thread side),napi_async_work::run/post_to_js_thread(#37750, which also adds the lint for the embedded-task.from(..)spelling),TranspilerJob::dispatch_to_main_thread(#37778), andFetchTasklet::deref_from_threadandStatWatcher::post_to_js_thread, which post through&self/ a helper's pointer and are reported for their own fixes. The lint's header lists the ones its regex cannot see; its allowlist names the PR converting each one it can, and the entry for whichever of those lands before this PR gets dropped here on rebase (the ratchet test fails otherwise, so it cannot be missed in either order).Tests
test/internal/source-lints/self-receiver-publish.test.tsbans posting a pointer spelled fromselfas a heap task:Task::init/ConcurrentTask*::create_from/ConcurrentTask*::from_callbackapplied tofrom_mut(self),from_ref(self).cast_mut(),self as *mut,&raw mut *self, bareself(which coerces), or a local bound to one of those (includinglet p: *mut Self = self;) further down the same function. Same-loopenqueue_taskposts are in scope on purpose: there the hazard is provenance rather than the protector (the queued pointer is a child of the receiver reborrow, and the first access through the owner's pointer before the queue drains kills it; themaybe_queue_finalizercase in napi_body.rs is exactly that, and the finalizer it posts is what frees the object), which the header states alongside the cross-thread argument, both checked with Miri reductions. It checks its patterns against positive and negative examples and ratchets the five regex-visible instances with what is wrong at each and the PR converting it (above). Againstmainit reports exactlyand passes with this branch.
Verification
On the debug (ASAN) build: test/bundler/bun-build-api.test.ts (52 pass, including the "thousands of times in one process" test, which runs this hand-back a few thousand times), bundler_plugin.test.ts, bundler_plugin_chain.test.ts, bundler_defer.test.ts, metafile.test.ts, bundler_html_server.test.ts and test/js/bun/http/bun-serve-html.test.ts (the HTMLBundle route path through the same completion), plus a script exercising the success arm, the
throw: falseerror arm and the throwing arm 50 times each.bun test test/internal/source-lints/(19 files) passes;cargo clippy -p bun_bundler -p bun_runtimeandcargo fmt --checkare clean.Two things seen while running those are unrelated to this change and were reported separately: a DevServer debug assertion when a dev-mode HTML route is started after
process.chdir()(shows up when bundler_html_server.test.ts and bun-serve-html.test.ts run in one process; the backtrace does not involve this code), and bun-serve-html-entry.test.ts failing to connect tolocalhostin this container with the release binary as well.