bundler: allocate, link and post a plugin request through one pointer instead of a &mut receiver - #37732
bundler: allocate, link and post a plugin request through one pointer instead of a &mut receiver#37732robobun wants to merge 6 commits into
Conversation
Resolve::dispatch and Load::dispatch took &mut self, linked the request into Graph::outstanding_* through one reborrow of it and posted a second one (ptr::from_mut(self)) to the plugins' JS thread. The next dispatch writes the request's link through the list's pointer, which invalidates the posted one under the aliasing model before the answer is consumed through it; the &mut self argument also claims exclusive access for a call during which the JS thread may already be writing the request. Both functions now take the request as `this: *mut Self` and pass that same pointer to the list and to Task::init. The three callers bind the arena slot as a raw pointer; Resolve no longer needs a Default impl to pre-allocate the slot. The source lint checks the receiver shape and that the body links and posts the same pointer.
|
Warning Review limit reached
Next review available in: 7 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 |
|
Status: reproduced as an aliasing-model violation, not a crash. The reduction in the description (main's Current shape (after two self-review rounds): one by-value Landing order: after #37723 (its publish lint ratchets these two sites at exactly 2; this PR deletes that entry on its rebase, which that lint's own ratchet test enforces). The consuming side (the JS thread holding the whole request as CI: every bundler and plugin lane has been green on each push. The red entries on earlier builds were |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The transformation is mechanical and the aliasing argument is sound, but since it reworks the unsafe cross-thread hand-off in the bundler's plugin dispatch path, a human look at the pointer-provenance reasoning would still be worthwhile.
What was reviewed:
- Confirmed
arena_createreturns&mut T, immediately coerced to*mutat each of the three callers with no further use of the reference — the list and the task now share one provenance. - Verified no remaining
Resolve::default()or.dispatch()call sites insrc/; the removedDefaultimpl is dead. - Checked that ordering is preserved at all three callers (
increment_scan_counterbefore dispatch; cancelled path still links then early-returns). - The source-lint test guards against vacuous pass, self-tests its audit on both the old and several near-miss shapes, and pins the expected count of
fn dispatchinsrc/bundler/.
Extended reasoning...
Overview
This PR changes Resolve::dispatch and Load::dispatch in src/bundler/bundle_v2.rs from &mut self methods to unsafe fn dispatch(this: *mut Self) associated functions, so the outstanding-list pointer and the posted-task pointer are the same raw pointer rather than two independent reborrows of a &mut receiver. The three callers (enqueue_on_resolve_plugin_if_needed, the entry-point resolve path, and the load path) now build the request, place it in the arena, bind the slot as *mut, and call dispatch(ptr). The now-unused impl Default for Resolve is removed. A new source-lint test in test/internal/source-lints/ enforces the shape going forward.
Security risks
None. This is an aliasing-model contract fix with no behavioral change: same operations, same order, same data. No new inputs are parsed and no trust boundary is touched.
Level of scrutiny
High — this is unsafe Rust on a cross-thread raw-pointer hand-off path in the bundler, which REVIEW.md calls out as the most-blocked category. The change itself is mechanically narrow (a receiver-type change plus symmetric caller updates) and strictly weakens what the function asserts about aliasing, so it cannot introduce UB where there was none. The PR description includes a Miri reduction demonstrating the before/after under both Tree Borrows and Stacked Borrows. Still, the correctness of &mut *(*this).bv2 staying as-is (deferred to #37709) and the interaction with the consuming side (run_on_js_thread, noted as reported separately) rest on a threading-model argument a maintainer should confirm.
Other factors
- Part of an established series of identical-pattern fixes (#37685, #37723, #37709) already landing in this repo.
arena_createreturns&'r mut T(bundle_v2.rs:3005); the callers coerce it directly to*mut Tin the binding, so the&mutis never named and cannot be reused — the one raw pointer is what everything downstream holds.- Grep confirms no remaining
.dispatch()method-call sites orResolve::default()uses anywhere insrc/. - The new lint test follows the existing
test/internal/source-lints/conventions (git-tracked filter,globAllSources, self-test of the audit function, non-vacuous-scan guard) and asserts exactly twofn dispatchinsrc/bundler/so a moved or added request type trips it. - Not covered by CODEOWNERS.
|
Updated 1:32 PM PT - Aug 12th, 2026
❌ @robobun, your commit 3fd2e18 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 37732That installs a local version of the PR into your bun-37732 --bun |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it reworks the unsafe cross-thread pointer hand-off for bundler plugin requests — the memory-safety category REVIEW.md flags as most-blocked — a human look at the aliasing-model reasoning would still be worthwhile.
What was reviewed:
- Both
dispatchbodies:thisis passed verbatim tooutstanding_*.pushandTask::init, no reborrow formed; the cancelled path andincrement_scan_counterordering are unchanged. - All three call sites now bind
arena_create's result as*mut(ref-to-raw coercion at the binding, one root pointer); the removedDefault for Resolvehas no remaining users. - The source lint's
auditself-tests cover the pre-PR shape and raw-receiver bodies that reintroduce a second derivation;foundasserts exactly two sites so the check can't pass vacuously.
Extended reasoning...
Overview
Changes Resolve::dispatch and Load::dispatch in src/bundler/bundle_v2.rs from &mut self receivers to unsafe fn dispatch(this: *mut Self), so the outstanding-list entry and the posted task both hold copies of the same raw pointer instead of two independent reborrows of a &mut receiver. The three callers now build the request, place it in the arena, coerce the returned &mut T to *mut T at the binding site, and pass that pointer through. The now-unused impl Default for Resolve is deleted. A new source-lint test in test/internal/source-lints/ pins the shape.
Security risks
None user-facing. This is an internal aliasing-model contract fix; no input parsing, auth, or network surface is touched.
Level of scrutiny
High. This is unsafe Rust that shares raw pointers between the bundle thread and the JS thread, in a file that already carries several related in-flight PRs (#37685, #37709, #37723). The transformation is mechanical and the Miri reduction in the description is convincing, but per REVIEW.md this is exactly the category where a maintainer should confirm the reasoning — in particular that coercing arena_create's &'r mut T to *mut T at the caller (rather than having arena_create return raw) is the intended shape, and that leaving &mut *(*this).bv2 in place is consistent with the plan for #37709.
Other factors
Grep confirms exactly two fn dispatch in src/bundler/ (matching the lint's found assertion), no remaining .dispatch() method-call sites, and no remaining Resolve::default() uses. The comment-cop feedback on doc-comment length was addressed in dc8b5ba and all threads are resolved. The test is a regex source lint (not a runtime repro), which is the established pattern for this family of aliasing fixes; the PR description reports the bundler plugin suites pass on the debug/ASAN build.
BundleV2::dispatch_plugin_request takes the request by value, parks it in the arena and hands the one resulting pointer to both the outstanding list and the task, so no unsafe is needed anywhere on the dispatch path and the two request types' dispatch fns only forward to it. OutstandingNode gains the accessor for the list a request type lives in. The lint now checks the helper's shape and that every dispatch forwards to it, and fails if the tree-wide publish lint still ratchets bundle_v2.rs.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it reworks pointer provenance on the bundler's cross-thread plugin dispatch path and carries an explicit landing-order dependency on #37723, a human look would still be worthwhile.
What was reviewed:
dispatch_plugin_requestpreserves the original order (link → cancelled check → post) and bothOutstandingNode::outstandingimpls point at the right list.- All three call sites updated;
increment_scan_counterstill runs before dispatch; no remainingResolve::default()or parameterless.dispatch()calls in the crate. arena_create's&mut Treturn coerces to the single*mut Tused for bothpushandTask::init— matches the Miri-validated shape in the description.
Extended reasoning...
Overview
The PR converts Resolve::dispatch / Load::dispatch in src/bundler/bundle_v2.rs from &mut self methods that derived two pointers from the receiver (one into OutstandingList, one posted to the JS thread) into by-value forwarders to a new BundleV2::dispatch_plugin_request<T>. That helper allocates the request in the arena once and hands the same *mut T to both the intrusive list and Task::init, eliminating the aliasing-model UB Miri flags on main. OutstandingNode in src/bundler/Graph.rs gains an outstanding() associated fn so the helper can pick the right list generically. The now-unused Default for Resolve is removed, and a new source-lint test pins the shape.
Security risks
None identified. This is an internal aliasing/provenance contract fix; no user-controlled input reaches new code paths, no auth/crypto/permissions surface is touched, and the change is a net reduction of unsafe (two blocks removed, none added).
Level of scrutiny
High. This is memory-safety-adjacent code on the bundler's cross-thread plugin dispatch path — exactly the category the repo's review guidance names as most-blocked. The reasoning is subtle (Tree/Stacked Borrows provenance across an arena allocation, an intrusive list, and a task posted to another thread), and while the change looks correct and behavior-preserving, a maintainer familiar with the surrounding on_resolve/on_load consuming side and the related open PRs (#37723, #37709) should confirm the shape and coordinate the stated landing order.
Other factors
- I verified
arena_createreturns&'r mut T(bundle_v2.rs:2982), which coerces to the*mut Tbinding;Task::initandOutstandingList::pushboth already accept*mut T, so no new coercion sites were introduced. - Grep confirms no remaining callers of
Resolve::default()or the old zero-arg.dispatch()insrc/bundler/. - The behavioral test coverage is the existing
bundler_plugin*/bundler_defer/ bake plugins suites (per the description); the new file is a regex-based source lint rather than a runtime regression test, which is appropriate for a contract fix with no observable behavior change but means the runtime guarantee rests on those existing suites plus the Miri reduction. - The PR explicitly states it should land after #37723 and that its last lint test enforces the ratchet-entry cleanup on rebase — merge coordination is a human call.
|
Heads up: #37803 adds |
…d name Any push onto an outstanding list outside dispatch_plugin_request is now the offence, whatever the method is called, instead of auditing every fn named dispatch in the crate. The landing-order check against the publish lint's allowlist is dropped: that lint's own exact-count ratchet already fails in any tree that has both changes.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it reworks pointer ownership on the bundle-thread ↔ JS-thread plugin dispatch path and has a stated landing-order dependency on #37723 (ratchet entry to delete on rebase), a human look would still be worthwhile.
What was reviewed:
dispatch_plugin_requestbinds onearena_createresult and passes that same*mut Tto bothOutstandingList::pushandTask::init, matching the aliasing fix described;pushandTask::initalready take*mut T.- All three
.dispatch()call sites updated to pass the by-value request plus&mut BundleV2;increment_scan_counterstill runs before dispatch at bothResolvecallers; the cancelled-pass link-then-wake sequence is preserved verbatim. Resolve::defaultremoval — grepped, no remaining users.- The new source-lint's regexes checked against the actual helper body and against
OutstandingList::{push,unlink,pop}— non-link uses are correctly ignored.
Extended reasoning...
Overview
Refactors Resolve::dispatch and Load::dispatch in src/bundler/bundle_v2.rs from &mut self methods (which derived two separate raw pointers from the receiver — one for the outstanding list, one for the posted task) into thin by-value forwarders to a new generic BundleV2::dispatch_plugin_request<T>. That helper allocates the request in the arena once and hands the single resulting *mut T to both the intrusive list and the JS-loop task, resolving the Miri-reported aliasing violation (a neighbouring dispatch's link write invalidating the posted pointer). OutstandingNode in src/bundler/Graph.rs gains an outstanding() accessor so the helper can be generic over Resolve / Load. The now-unused impl Default for Resolve is deleted, and the three call sites are simplified (no more pre-allocate-then-assign). A new source-lint test pins the shape.
Security risks
None identified. This is an internal aliasing-model contract fix; no user-controlled input reaches new code, no auth/crypto/permission surface is touched. The pointer being unified is arena-owned and the arena outlives the bundle pass, same as before.
Level of scrutiny
High. This is memory-safety code on a cross-thread path (bundle thread posting to the JS/plugin thread), and it is one of a family of interdependent aliasing PRs (#37685, #37709, #37723, #37803) with cross-PR ratchet entries whose landing order is load-bearing. The description explicitly says this must land after #37723 and then delete that PR's ratchet entry for bundle_v2.rs — that step is not in this diff and needs to happen on rebase. A maintainer coordinating that family should confirm the sequencing.
Other factors
The order of operations in the consolidated helper matches both original bodies exactly (link → check cancelled → post), and the carried-over cancelled-path comment is verbatim. arena_create returns &'r mut T, which the helper immediately coerces to *mut T — the reference is never used again, so both consumers share one provenance. The stored bv2 back-pointer inside Resolve/Load (set by init) is still derived from a caller-side &mut BundleV2 before a fresh &mut self is taken for dispatch; that back-ref pattern is preexisting throughout this file and the PR notes the consuming-side aliasing is being addressed separately, so I did not treat it as a regression here. All comment-cop bot flags on doc length are resolved in the timeline (dc8b5ba, 0f43e95). No CODEOWNERS entry covers src/bundler/. Bundler plugin suites pass per the description; the source-lint self-tests cover main's shape and several near-miss regressions.
Problem
dispatch(&mut self)makes two pointers to the request from the same receiver: one kept in the pass's outstanding list, one posted to the JS thread as the task.Undefined Behavior: reborrow through <4058> at alloc1960[0x0] is forbidden), and passes once the list and the task hold one and the same pointer.Fix
dispatchbodies become one function on the pass that takes the request by value, allocates it in the arena, links the*mutthe arena hands back, and posts that same*mut. The old names stay as one-line forwarders; callers no longer preallocate a default slot.unsaferemains on this path.push(self)sites withsrc/at main and passes here. The Miri reduction fails before and passes after. The bundler plugin, bake plugin and worker-terminate suites pass on the ASAN build; none is claimed to fail without the fix.Background
ResolveorLoad) is a struct the bundle thread allocates in the pass's arena for one onResolve or onLoad call. The JS thread runs the plugin chain, writes the answer into the struct, and posts the same pointer back to the bundle thread.Graph, one per request type. Each request carries its own link, so pushing or unlinking a neighbour writes into this request's memory, and cancellation walks the list to fail every pending request.&mutgets its own tag, and a write through one tag invalidates the others. Copies of one raw pointer share a tag, which is why handing the same*mutto both places is fine.Task::initwraps a raw pointer to aTaskableso it can be posted to another thread's event loop.OutstandingNodeis the trait that maps a request type to its link field, and now also to its list.bun run rust:miriuses Tree Borrows.Original description
Lands after #37723: that PR's publish lint carries an exact-count ratchet entry of 2 for
bundle_v2.rs, for the two sites converted here. Once it is in, this PR gets rebased and deletes the entry; that lint's own ratchet test fails on the rebase until it is. If this one is merged first instead, #37723 has to drop the entry before it merges.Problem
Resolve::dispatchandLoad::dispatch(src/bundler/bundle_v2.rs) are how a bundle pass hands an onResolve / onLoad request to the thread that runs the plugins. Both took&mut selfand used it twice:From then on the request is reached through both pointers. The list's pointer is written through whenever a neighbouring request is dispatched or answered (
OutstandingList::pushsets the previous head'sprev;unlinkfixes up both neighbours) and is whatfail_outstanding_plugin_requestsuses on cancellation. The task's pointer is what the JS thread writesvaluethrough and what comes back toon_resolve/on_load, which reborrow it and unlink the request. Under the aliasing model the two are separate reborrows of the receiver, so the link write made by dispatching the next request invalidates the posted pointer, and consuming the answer through it is UB. This is deterministic as soon as two requests are outstanding at once, which is the normal state of a build whose plugin answers asynchronously; it does not depend on the JS thread's timing. Separately,&mut selfis a protected (noalias) argument for the whole call, while forBun.buildthe JS thread may already be writing the request beforedispatchreturns. Nothing misbehaves today (the reads that would be affected are not optimized across the post), so this is a contract fix, same family as #37723 (the completion hand-back, which frees) and #37685.A reduction with the real
OutstandingListshape fails under Miri with Tree Borrows (the modelbun run rust:miriuses) and with Stacked Borrows, in both cases at the answer's reborrow, naming the link write as what invalidated the pointer:The same program passes under both models when the list and the task are given one and the same pointer.
Reduction (
cargo miri run -- beforefails,-- afterpasses;MIRIFLAGS=-Zmiri-tree-borrowsor default)The threads are sequenced through the channels, so the only thing Miri can report is an aliasing violation. Stacked Borrows reports the same site:
trying to retag from <4198> for Unique permission ... <4198> was later invalidated at offsets [0x0..0x8] by a write accessat the sameprevstore.Fix
The two bodies were the same code apart from which list they push to, so they become one function on the pass:
It takes the request by value, so the
*mutthatarena_createhands back is the only pointer to it that ever exists, and that one goes both into the list and into the task;pushandTask::initalready took raw pointers.OutstandingNode(src/bundler/Graph.rs, which already maps a request type to its link) gainsoutstanding(), mapping it to its list.Resolve::dispatch/Load::dispatchbecome one-line forwarders takingselfby value plus the pass the callers already hold; they are kept under those names because the runtime's thunks in src/runtime/api/JSBundler.rs document their pointers as coming fromResolve::dispatch/Load::dispatch, in comments that #37691 and the consuming-side fix are editing, so rewording them here would only buy conflicts. The callers build the request and hand it over, which also means the twoResolvecallers no longer pre-allocate aResolve::default()slot to assign into, so thatDefaultimpl goes away. There is nounsafeleft on this path (main had two blocks), and nothing the callers can do with the request after dispatching it. Order of operations is unchanged: the scan counter is incremented before the dispatch, and a cancelled pass still links the request and leaves it foris_doneto fail.Adjacent work, not changed here: #37709 changes the
run_on_js_threadbodies next to these and leaves the dispatch side alone; the consuming side, where the JS thread holds the whole request as&mut Load/&mut Resolvewhile the bundle thread keeps writing its link through this same pointer, is a different shape (Miri flags it as a foreign write to a protected tag) and has been reported separately.Tests
test/internal/source-lints/bundler-plugin-dispatch-raw-request.test.tschecks two things across src/bundler/:dispatch_plugin_requesttakes the request by value, binds onearena_createresult and passes that same local to.push(..)andTask::init(..)without forming a reference to it; and no other code links a request into an outstanding list (outstanding_*.push(..)orT::outstanding(..).push(..)outside the helper's body). Linking is what makes something a dispatched request, so this is keyed on the shape rather than on a method name: a method of any name that links its receiver again is reported, and an unrelatedfn dispatchsomewhere in the crate is not. It self-tests the audit against main's shape and against near misses (request taken as*mut/&mut, a secondarena_create, the slot reborrowed, aredispatch(&mut self)re-adding a push, and non-link uses of the lists), and pins that there is exactly one helper and exactly one link site, so the stray-link check cannot go vacuous. Withsrc/at main it reports the twopush(self)sites (bundle_v2.rs:1133and:1267,links a request outside dispatch_plugin_request) and the missing helper; it passes here.On the debug (ASAN) build:
test/bundler/bundler_plugin.test.ts(53 pass),bundler_plugin_chain.test.ts(13),bundler_defer.test.ts(10), which dispatch both request types from theBun.buildbundle thread with several outstanding at once;test/bake/dev/plugins.test.ts(3) for the arm where the posting loop is the plugins' own loop; thepoolfamily oftest/js/web/workers/worker-terminate-funnels.test.ts, which terminates a worker with aBun.buildonLoad pending (the cancelled path popping the request back out of the list).bun test test/internal/source-lints/passes (19 files);cargo clippy -p bun_bundlerandcargo fmt --checkare clean.