bundler: touch outstanding plugin requests through their pointer, not as &mut - #37746
bundler: touch outstanding plugin requests through their pointer, not as &mut#37746robobun wants to merge 4 commits into
Conversation
…ld by field While an onResolve/onLoad request is out with the plugins' thread it is still linked in the bundle thread's outstanding list, which that thread writes through (push of the next request, unlink of an answered one, Load::deferred from on_notify_defer_mini). Every place the plugins' thread touched a request held the whole struct as &mut for the duration (run_task arms, run_on_js_thread, the onResolveAsync/onLoadAsync/addError/ onDefer thunks, on_*_async across the post-back), and the list reborrowed the neighbours whole to write their links. Pass the request as *mut everywhere it may be outstanding and access only the owning side's fields through it; OutstandingNode projects the link as a raw pointer. Removes Load::was_file, which was never set. Adds a source lint over this population of functions.
WalkthroughBundle V2 plugin request handling now uses raw pointers for outstanding loads and resolves. Plugin inputs are copied into owned strings. Runtime callbacks and deferred notifications use pointer-based handoffs. A source-lint test audits these access patterns. ChangesOutstanding plugin request handling
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: reproduced as an aliasing-model violation, not a crash. Two reductions in the description (a thunk holding the request as Second revision (3cc7518), from self-review: the strings for the plugin call are copied out of the request before the call and Third revision (895a4fb) is comments and the test only: the contract is stated once on Related open PRs on the same lines: #37732 (posting side), #37709 / #37691 (how these functions reach the pass and the loop), #37731 (defer notify path). Each pair applies independently; whichever of a pair lands second resolves a small conflict, as described in the body. CI, for whoever picks this up: the source under test has not changed since 895a4fb (c7960c2 is an empty commit). Builds 92838 and 93269 of it are green on every lane that ran, including the ASAN lane and the source-lints job that runs the new lint; in both, the only red entry is the Update: the darwin-26 queue eventually served build 93544, and both of its |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it reworks the aliasing contract for cross-thread bundler plugin requests — converting eleven function signatures and the OutstandingList internals from &mut to raw-pointer field-by-field access, deleting the was_file field/branch and LoadJsExt, and adding a 626-line source lint — and it interacts with four other open PRs on the same lines, a human look is warranted.
What was reviewed:
- The
&mut T→*mut Tconversion at each of the eleven sites and the tworun_taskarms preserves the same field reads/writes in the same order. unlink's callers (on_load/on_resolve/pop) still type-check via&mut → *mutcoercion;Load::bake_graph()retains its remaining caller inon_load.was_filehas no remaining setters or readers inbundle_v2.rs; the deleted branch was unreachable.on_defernow readsbv2/parse_taskbefore the notify post so the post is the last touch of*load, matching the stated contract.
Extended reasoning...
Overview
This PR fixes an aliasing-model UB (verified under Miri Tree Borrows and Stacked Borrows) in the bundler plugin request path. api::JSBundler::Resolve/Load requests are shared between the bundle thread and the plugins' thread by field while outstanding: the plugins' thread writes value/called_defer/task while the bundle thread writes the intrusive outstanding link and deferred. The old code formed &mut Load/&mut Resolve on the plugins' thread (as function arguments, protected for the call duration), which under both borrow models is UB when the bundle thread concurrently writes the link. The fix converts every touch point on both sides to raw-pointer field-by-field access: run_on_js_thread(this: *mut Self), on_*_async(*mut), on_notify_defer_mini(*mut Load), OutstandingNode::link_raw(this: *mut Self) -> *mut OutstandingLink<Self>, and the four C++-facing thunks. It deletes the always-false was_file field and its dead branch, removes the LoadJsExt trait, and adds a source-lint test enforcing the shape.
Security risks
None. This is a mechanical &mut → *mut conversion with no change to what is read or written, no new inputs, and no user-facing surface.
Level of scrutiny
High. This is memory-safety-critical unsafe Rust at a cross-thread boundary. Per REVIEW.md's "know the thread affinity of every line you touch" and "never let a pointer or slice outlive the memory it points into," the field-ownership contract this PR establishes (documented on the Resolve/Load type docs) needs a maintainer to confirm it matches the actual concurrent execution model — particularly that the answer-side consumers (on_load/on_resolve) and fail_outstanding_plugin_requests really do run only after the plugins' thread is done with the request, which is what justifies them keeping &mut. The PR's Miri reduction proves the model violation but can't prove the field partition itself is complete.
Other factors
- The PR explicitly interacts with four other open PRs (#37732, #37709, #37691, #37731) on the same lines; whichever lands first affects the rebase of the others and the lint's
ENTRIEStable. A maintainer should decide the landing order. - The dead-code deletion (
was_file+ its branch that scheduledparse_task.taskdirectly and setvalue = Consumedwithout unlinking) is well-justified — I confirmed no setter exists in the tree — but deleting a branch that would have bypassedon_load_asyncwarrants human sign-off. - The 626-line source lint is a novel test shape (regex over stripped Rust source) with a self-test; it looks correct but is itself a maintenance surface.
- No CI results are visible on the timeline yet.
|
On the two points the review asked a human to confirm, here is the argument, now also in the description: Why
Completeness of the partition: grepping every write to a On |
|
Updated 7:07 PM PT - Aug 12th, 2026
❌ @robobun, your commit c7960c2 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 37746That installs a local version of the PR into your bun-37746 --bun |
…k the outstanding list raw A plugin that answers without suspending posts the answer from inside match_on_load / match_on_resolve, after which the pass side may consume the request and free its buffers, so the &[u8] arguments into the request those functions took were protected across exactly that window. The hops now build the BunStrings first and the match functions take them by value. OutstandingList gets a raw for_each so drain_deferred_tasks no longer walks the links itself; the type docs name both deferred writers. The lint now also covers drain_deferred_tasks and the two OutstandingNode impls, follows rebindings of the request pointer, rejects the as_mut / read / write / NonNull and untyped-parameter spellings, and requires the match_on_* call in the hops to take no borrows.
|
895a4fb (comments and the test only; no code change,
|
|
The |
|
Heads-up on overlap with #37709 (opened a bit earlier, same aliasing family, different bug): it rewrites the same two bodies, |
There was a problem hiding this comment.
I reviewed this PR and the bug hunting system found no issues. Because it reshapes the cross-thread aliasing contract for outstanding Resolve/Load requests across ~25 new unsafe blocks in bundler-critical code (and interacts with four sibling PRs — #37709, #37731, #37732, #37691 — on the same lines), a human look would still be worthwhile.
What was reviewed:
- The field-partition contract on
Resolveand every raw-pointer site against it — each side touches only its own fields, and the post is the last touch. match_on_load/match_on_resolvestring ownership: copies are made before the plugin call, so no borrow of the request is live across a synchronous answer; namespace defaulting moved toPlugin::load_namespace/resolve_namespacepreserves the empty/filemapping.on_defer:pluginandparse_taskare read out before the notify post;append_defer_promisetouches only the plugin, notload.was_filedeletion — the field was never set anywhere in the tree, so the removed branch was dead.
Extended reasoning...
Overview
This PR converts every access to an outstanding bundler plugin request (api::JSBundler::Resolve / Load) from &mut / &self to raw-pointer field-by-field access, on both the plugin side (run_on_js_thread, the JSBundlerPlugin__* C-ABI thunks, on_defer, on_*_async) and the pass side (OutstandingList / OutstandingNode, drain_deferred_tasks, on_notify_defer_mini). It also lifts the string arguments of match_on_load / match_on_resolve to owned BunStrings copied out of the request first, so no borrow of the request survives into the plugin call (which may answer synchronously and free those buffers before returning). The dead Load::was_file field and LoadJsExt trait are deleted. A new 814-line source lint (test/internal/source-lints/bundler-plugin-outstanding-request.test.ts) enforces the raw-pointer discipline over an explicit table of function names, with self-tests for both the conforming and previous shapes.
Security risks
None user-facing. This is internal memory-model correctness (Tree Borrows / Stacked Borrows aliasing UB under Miri, no known miscompile). No new inputs are parsed, no trust boundary changes. The FFI signature of JSBundlerPlugin__onLoadAsync changes from &mut Load to *mut Load, which is ABI-identical to the void* C++ already passes.
Level of scrutiny
High. Per the repo's own review guidance, native memory safety is the most-blocked category. The change introduces roughly two dozen new unsafe blocks/fns whose SAFETY comments hinge on a single field-ownership contract stated once on Resolve. The contract itself (which fields each side may touch while a request is outstanding, and that the post is the last touch) is the load-bearing invariant; a reviewer familiar with the bundler's threading should confirm it against dispatch, the mini-loop path, and the cancellation path. The description supplies two Miri reductions demonstrating both the before-UB and after-clean states, and the source lint pins the shape going forward, but the correctness of the partition is a design assertion that benefits from human sign-off.
Other factors
- The bug hunting system found nothing. My own read confirmed the string-copy hoisting preserves the namespace defaulting exactly,
for_eachreadsnextbefore invoking the callback,unlink(*mut T)still accepts&mut Tcallers by coercion, andon_deferreads everything it needs fromloadbefore the notify post. - The pre-existing
.expect()/panic!on conversion errors inonResolveAsync/onLoadAsync(flagged earlier and by CodeRabbit) are unchanged and acknowledged as a separate follow-up. - All comment-cop threads are resolved (comments were consolidated onto the
Resolvetype doc in 895a4fb; what remains are required# Safety/SAFETY:lines). - The author notes textual conflicts with #37709 / #37691 on
run_on_js_threadand semantic overlap with #37731 / #37732; whichever lands second needs a small rebase, and the lint's "good" sample callingbv2.plugins_mut()will need updating if #37709 lands first. That sequencing is a human call.
|
One clarification on the sequencing note above: the lint does not constrain how the hops reach the pass, so if #37709 lands first nothing in this test needs to change; the |
|
Overlap note: #37883 (under review by alii, third round just pushed) now converts |
Problem
deferredflag. UnderBun.buildthose are two threads; it works because neither side touches the other's fields.&mut(or&), which claims the other side's fields too. A link write landing during such a call is undefined behaviour; a reduction fails under Miri withUndefined Behavior: write access through <4570> at alloc2247[0x0] is forbidden.Undefined Behavior: deallocation through <9184> at alloc2084[0x0] is forbidden.Fix
Resolve/Loadtype docs.deferred, the plugin side writes only the answer, its defer flag and its task node, and no borrow of the request is live across the plugin call or after a post. Once answered or cancelled the pass owns the whole request again, so those paths keep taking&mut.Load::was_file,falsesince the port, is deleted. This is the consuming side only; the posting side is bundler: allocate, link and post a plugin request through one pointer instead of a &mut receiver #37732.Background
Resolvefor onResolve,Loadfor onLoad) is a struct the bundle pass allocates per hook call and hands to whatever runs the JS plugins. JS writes the answer into it and posts the same struct back; the pass then consumes it and frees its buffers. UnderBun.buildthe pass has its own thread and event loop; under bake both sides share one loop.defer()lets an onLoad plugin park its load until the rest of the scan is done. The pass sets and later clears adeferredflag on parked loads while they are still out with the plugin, so that is a second field the pass writes during the shared window.&mut Tor&Targument promises that nobody else writes to, or frees, any byte of theTuntil the call returns, even bytes the callee never reads. A raw pointer with a field projection (&raw mut (*p).field) promises that for the one field only. Miri checks this (Stacked Borrows and Tree Borrows are its two models); a violation is undefined behaviour whether or not the compiler exploits it today.[review] gate passed · iteration 1 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file
Original description
Problem
api::JSBundler::Resolve/Load(src/bundler/bundle_v2.rs) are the onResolve / onLoad requests a bundle pass hands to whatever runs the plugins: another thread underBun.build, the same loop under bake (the code is shared). Fromdispatchuntil the answer is consumed, one request is in use on both sides at once, by field. The plugin side reads the fields set before dispatch and writes the answer (value,called_defer, the intrusivetasknode). Meanwhile the request stays linked in the pass'sGraph::outstanding_resolves/outstanding_loads, and the pass side writes its link whenever another request is dispatched (OutstandingList::pushsets the current head'sprev) or answered (unlinkfixes up both neighbours), and writesLoad::deferred(Graph::drain_deferred_tasksover every linked load,on_notify_defer_minion the mini loop). UnderBun.buildthis is genuinely concurrent; it works because the two sides never touch the same field.Every place the plugin side touched a request nevertheless held the whole struct as
&mut, and the pass side did the same to requests that were still out:A
&mut Loadcovers theoutstandinganddeferredbytes too, and as a function argument it is protected for the duration of the call, so a link write landing during the call is a foreign write to a protected tag: UB under both aliasing models whether or not the reference is used again. Withon_load_asyncit is worse, because the pass side can unlink and consume the answer while the thunk's&mutis still on the plugin side's stack. Any multi-file build whose plugins answer asynchronously has several requests outstanding at once, so the link writes do land during the callbacks.There is a second, timing-shaped instance of the same thing that does not need two requests at all.
run_on_js_threadpassed&self.path/&self.import_record.*intoPlugin::match_on_load/match_on_resolve, whose&[u8]parameters stay protected for the whole call into the plugin. A callback that does not actually suspend (or no matching callback;runOnLoadPlugins/runOnResolvePluginsunwrap settled promises without awaiting, and the C++ side callsaddErrorsynchronously on a throw) answers from inside that call, the pass side may consume the answer at once, andon_load/on_resolvefreepath/import_record: a deallocation of memory a protected reference points into, while the plugin call is still unwinding.No crash is known from either: nothing in these bodies reads the fields the other side writes, and the freed buffers are not read again, so there is nothing for the
noalias/dereferenceableon the arguments to miscompile today. The contract is what is wrong. Two reductions, each sequenced over channels so that the only thing left for Miri to find is the aliasing violation, fail under Tree Borrows (the modelbun run rust:miriuses) and under Stacked Borrows, and pass in the shape this PR uses:(Stacked Borrows:
not granting access to tag <4728> because that would remove [Unique for <9295>] which is strongly protectedand.. would remove [SharedReadOnly for <6866>] which is strongly protected, at the same two sites.)Reduction 1: link write during a thunk (
cargo miri run -- beforefails,-- afterpasses; with and withoutMIRIFLAGS=-Zmiri-tree-borrows)Reduction 2: synchronous answer during the plugin call (same invocation)
This is the consuming side of the request; #37732 is the posting side (
dispatchtaking&mut selfand handing out two reborrows, which is also what stands between this PR and "the list, the cookie and the answer hold one pointer"; here the cookie and the answer are the task's pointer), and #37709 / #37691 are about how these same functions reach the pass (bv2) and the loop, which is left exactly as it was here. All four are independent; this one touches the samerun_on_js_threadbodies as #37709 and the same thunk bodies as #37691, so whichever lands second has a small conflict to resolve, with no change to what either does. #37731 (the scan counter whendefer()is not awaited) reshapes the defer notify path on these same lines and keeps the&mut Loadshape on the load it posts and in itson_notify_defer; if it lands first, those two get the same conversion in the rebase and join the lint's table, and if this lands first it needs the raw shape from the start. The lint's population is a table, so either order is a few lines.Fix
While a request is outstanding it is only ever touched through the raw pointer, field by field, on both sides, and nothing borrowed from it is live across the call into the plugin or after a post. The contract is written once, on the
Resolve/Loadtype docs; everything else points at it.Resolve/Load::run_on_js_thread(this: *mut Self); therun_taskarms pass the queued pointer instead of reborrowing it, andthisitself becomes the context cookie C++ hands back. The strings for the plugin call are copied out first (BunString::clone_utf8,Plugin::load_namespace/resolve_namespace, which is the namespace defaulting that used to live inside the match functions) andPlugin::match_on_load/match_on_resolvetake them by value, so no borrow of the request is an argument of the call that may answer it; those two functions have no other callers.JSBundlerPlugin__onLoadAsynctakes*mut Load(C++ declaresvoid*; the ABI is unchanged),onResolveAsyncandaddErrorstop forming&mut(addErrorcasts the cookie itself),on_deferbecomes a freeunsafe fn on_defer(load: *mut Load, ..)(theLoadJsExttrait existed only to give it method syntax) and readsbv2/parse_taskbefore posting the notify, so the post is the last thing done with the request.BundleV2::on_load_async/on_resolve_asynctake*mutand post it as is;from_callback,enqueue_task_concurrent_with_extra_ctxandTask::initalready took raw pointers.OutstandingNode::link(&mut self)becomesunsafe fn link_raw(this: *mut Self) -> *mut OutstandingLink<Self>(&raw mut (*this).outstanding);OutstandingList::push/unlink/popwrite through it and a newfor_eachwalks through it, so the list never forms a reference to a node anddrain_deferred_tasksshrinks to the onedeferredwrite it makes per load (OutstandingLink::nextbecomes private as a result).unlinktakes the node as*muttoo (its callers hold&mut, which coerces).on_notify_defer_minitakes*mut Loadand writesdeferredthrough it.BackReffield (parse_task) is copied out before use: rustc'sdangerous_implicit_autorefsrejects(*this).parse_task.x(it goes throughDeref), which is the same mistake in a form the compiler happens to catch.Load::was_fileand the branch inonLoadAsyncthat read it. It has beenfalsesince it was ported (it was never set in the Zig version either); had it run, it would have answered the load without posting it back and left it linked.The answer-side consumers (
on_load/on_resolve) and the cancellation path (fail_outstanding_plugin_requests) still take&mut, because by then the plugin side is done with the request and the pass side owns all of it again:on_*_asyncis now the last thing the plugin side does with the request (the thunks return straight after it, and C++ only ever round-trips the cookie without dereferencing it), and the post itself orders everything before it ahead of the pass side's pop. Thetasknode is written just before each post and read after the pop, so it is ordered the same way; the one case where it is enqueued a second time while still queued (defer()not awaited) is what bundler: keep the scan counter balanced when an onLoad plugin does not await defer() #37731 is fixing by giving the notify its own node.stop_for_vm_teardownruns on the plugins' own thread, tombstones the plugin (soonLoadAsync/onResolveAsync/addErrorare dropped in C++ from then on), then storescancelledwith Release;is_donereads it with Acquire, consumes the answers already in its queue, and only then fails what is left. Hop tasks still queued on that VM's loop are released unrun (__bun_release_task_unrun;release_unrunis a no-op for these types), so no access from that thread follows the store.The partition itself: while a request is out and not cancelled, the pass side writes only
outstanding(the list) andLoad::deferred(drain_deferred_tasks,on_notify_defer_mini), and the plugin side writes onlyvalue,called_deferandtaskand reads fields that were set ininit(theParseTaskbehindparse_taskis not scheduled untilon_load). Every write to a request field inbundle_v2.rs/Graph.rs/JSBundler.rsis one of those or is on the answer / cancellation side.No behavior change: the same strings reach the plugin (the copies were already being made, one frame lower), and the same fields are read and written in the same order on every path.
Verification
Nothing is observable at runtime, so the test is a source lint,
test/internal/source-lints/bundler-plugin-outstanding-request.test.ts, over exactly this population: the eleven function definitions that touch a request while it may be outstanding (includingdrain_deferred_tasks), the tworun_taskarms, theOutstandingNodetrait and its two impls, and theOutstandingListimpl. For each function it checks that the request arrives as a*mut Resolve/Load/Selfparameter (a receiver, a reference, or an untyped pointer with no recognized request parameter all fail;addErroranddrain_deferred_tasksname their handles explicitly), that neither that parameter nor any local rebound from it (let x = req;,let x = ctx.cast::<Load>();,ascasts) is reborrowed, borrowed whole, turned into a reference orNonNull(from_mut,as_mut(), ..), read or written as a whole (read()/write()/*req = ..), used as a method receiver through(*req), or reached throughcallback_ctx; that where the request is handed on (cookie, post-back, notify) the body passes the pointer it received; and that thematch_on_*call in the hops has no&argument at all. The structural audits check that the trait hands out no references, both impls are raw projections, and the list impl never reborrows a node or takes one as&mut. Its header states the boundary (per-function text checks; a helper that forms the reference internally is outside them), it has a guard that the population is still where it looks, and a self-test covering every accepted and rejected shape above, including the evasions. Against main'ssrc/it reports 35 findings at the sites listed above; here it passes.Debug (ASAN) build:
test/bundler/bundler_plugin.test.ts(53 pass; includes the sync and async throwing plugins, i.e.addErrorfor both request types, and plenty of synchronously answered requests),bundler_plugin_chain.test.ts(13),bundler_defer.test.ts(10;on_defer, the notify,on_notify_defer_mini,drain_deferred_tasksover the parked loads),test/bake/dev/plugins.test.ts(3; both sides on one loop).test/js/web/workers/worker-terminate-funnels.test.ts(10; builds cancelled with requests outstanding, i.e.pop/unlinkon the cancellation path) passed on the first revision of this branch; on the current revision its cases hit their 30s ceilings on a host with a load average around 170 (they take 20 to 26s here when the machine is quiet), and the cancellation path it covers is unchanged between the two revisions.cargo clippy -p bun_bundler -p bun_runtimeandcargo fmtare clean.