event_loop: post to a MiniEventLoop through raw pointers and publish last - #37883
event_loop: post to a MiniEventLoop through raw pointers and publish last#37883robobun wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (11)
WalkthroughChangesThe PR replaces mutable mini event-loop access with shared access and raw-pointer enqueue APIs. Queue publication and wake-up ordering remain explicit. Bundler, VM, shell, process, and deferred-load paths now use the updated APIs. Source-lint tests pin the required implementation ordering. Mini event-loop enqueueing
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
(Superseded; the current status is in the comment below.) |
|
Updated 10:42 PM PT - Aug 13th, 2026
❌ @robobun, your commit 0caaf9b has 2 failures in
🧪 To try this PR locally: bunx bun-pr 37883That installs a local version of the PR into your bun-37883 --bun |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The reordering in publish looks correct and the source-lint test pins it. Because this is a cross-thread memory-safety change in the event loop whose correctness rests on lifetime arguments (uws loop outlives the boxed MiniEventLoop; UnboundedQueue::push's last access to self is the publishing store) and it intersects with #37691/#37701 on the same lines, a human look would still be worthwhile.
What was reviewed:
publishreadsloop_beforeconcurrent_tasks.pushand wakes viabun_uws::us_wakeup_loop(loop_)— matches the existing pattern inMiniEventLoop::wakeupandjs_bundle_completion_task.rs:622.UnboundedQueue::pushtakes&selfand its final access is the AcqRel swap onback/ Release store onfront, so the consumer cannot observe the task before the poster's last read of the queue.- The new lint follows the conventions of the sibling
test/internal/source-lints/tests (usesglobAllSources, git-tracked filter, self-pinning fixtures) and its non-vacuous guard checks both public entry points are still recognized as publishers.
Extended reasoning...
Overview
The PR reorders two statements in MiniEventLoop::enqueue_task_concurrent and enqueue_task_concurrent_with_extra_ctx: instead of push(task) then (*self.loop_ptr()).wakeup(), both now go through a shared publish(&self, task) that copies self.loop_ into a local, pushes, and calls bun_uws::us_wakeup_loop on the copy. This closes a use-after-free window where a per-build boxed MiniEventLoop (created in JSBundleCompletionTask::init_and_run) can be dropped by the bundle thread between the poster's push and its subsequent read of self.loop_. A source-lint test enforces that no function under src/event_loop/ that pushes to concurrent_tasks (or calls one that does through self) touches self after the publishing call.
Security risks
None. This is a data-race / UAF fix in internal event-loop plumbing; no user-controlled input, no auth, no crypto.
Level of scrutiny
High. This is native cross-thread memory-safety code — the review guide's most-blocked category. The correctness of the fix rests on two lifetime claims: (1) the uws loop pointer is thread-lifetime and outlives any boxed MiniEventLoop, and (2) UnboundedQueue::push_batch's final access to self is the release store the consumer synchronizes on. I checked (2) against src/threading/unbounded_queue.rs:215-240 and it holds: the last touch of self is the AcqRel swap on back or the Release store on front, both of which are what the consumer's Acquire load observes. Claim (1) matches the field's documented invariant and the existing precedent in stop_for_vm_teardown. But these are exactly the kind of subtle arguments a maintainer who owns the bundle thread lifecycle should confirm, especially given the PR notes #35060's overflow-thread design would need additional care.
Other factors
The PR description explicitly flags that #37691 and #37701 touch the same lines with a different ordering, so whichever lands second needs a rebase and the lint here is what keeps the invariant. That coordination is a human call. There is no runtime regression test (the race is not deterministically reproducible, per the description); the source-lint substitutes for it and is well-constructed — it self-tests against the old and new bodies, a pre-read-but-still-wakes-through-self shape, a caller that touches self after, an unattributable push, and the owner's own drain — but a maintainer should confirm this style of test-as-lint is acceptable coverage for a UAF fix here. publish taking &self while the callers keep &mut self is fine (UnboundedQueue::push is &self); the residual &self being notionally live past the free is the same pre-existing concern as the &mut self receiver and is not worsened by this change.
There was a problem hiding this comment.
I reviewed this PR and the automated bug-hunting pass found no issues. The reordering in publish looks correct — loop_ptr() is copied before the push, UnboundedQueue::push takes &self, and us_wakeup_loop on the copied *mut Loop matches what MiniEventLoop::wakeup already does. Because this is a cross-thread memory-safety change in core event-loop code whose correctness rests on the uws-loop-outlives-the-struct argument, and because comment-cop has flagged the doc comment on publish (line ~400) which the author hasn't yet addressed or dismissed, a human look is still worthwhile.
What was reviewed:
- Confirmed
bun_uws::us_wakeup_loopis re-exported and takes*mut Loop, andUnboundedQueue::push(&self, ...)— sopublish(&self, ...)compiles and the wakeup path is behaviorally identical. - Checked the new source-lint follows the existing
test/internal/source-lints/pattern (sameglobAllSources().rust+git ls-treeguard asdead-code-escapes.test.ts); the self-check test pins the matcher on both the old and new bodies. - Noted the stated overlaps with #37691 / #37701 touch the same lines — landing order is a maintainer call.
Extended reasoning...
Overview
The PR reorders two statements in MiniEventLoop::enqueue_task_concurrent / enqueue_task_concurrent_with_extra_ctx so the uws loop pointer is read out of self before the task is published to the owning thread, closing a narrow cross-thread UAF window when the owning thread frees a per-build MiniEventLoop (the Bun.build case) between the poster's push and its wakeup. Both call sites are folded into a single publish(&self, ...) helper. A new source-lint test (test/internal/source-lints/mini-loop-publish-last.test.ts, ~290 lines) enforces that no function under src/event_loop/ touches self after a concurrent_tasks.push.
Security risks
None user-facing. This is an internal memory-safety hardening; no new inputs, no parsing, no auth surface.
Level of scrutiny
High. This is exactly the category REVIEW.md calls out as most-blocked: raw-pointer cross-thread lifetime reasoning in native runtime code. The fix's soundness depends on the invariant that the uws loop (a thread-local obtained via UwsLoop::get()) outlives the MiniEventLoop struct — the PR argues this holds for every mini loop today and stop_for_vm_teardown already relies on it, but a maintainer familiar with #35060's overflow-thread design and the two overlapping in-flight PRs (#37691, #37701) should confirm the argument and decide landing order.
Other factors
- Outstanding comment: the
comment-copbot flagged the 8-line doc comment onpublishat MiniEventLoop.rs:~400 ("paragraph-long comment to justify a workaround"). The author hasn't shortened it or replied. Whether that's a merge blocker or a false positive (the comment documents a real cross-thread invariant, not a workaround) is a maintainer judgment. - No runtime repro: the guard is a source-level lint plus a Miri reduction in the description; the PR is explicit that the race window is too small to reproduce deterministically. That's a reasonable trade-off for this bug class but again a maintainer call.
- Test conventions: the new lint mirrors existing files in
test/internal/source-lints/(same import shape, same tracked-file guard), and includes a self-test that pins the regex matcher on the exact old and new bodies plus adversarial variants, so it isn't vacuous. - I verified
bun_uws::us_wakeup_loop(*mut Loop)is the re-exported C binding already used byMiniEventLoop::wakeupandHTTPThread, andUnboundedQueue::pushtakes&self, so the refactor topublish(&self, ...)type-checks and is behavior-preserving apart from the read ordering.
alii
left a comment
There was a problem hiding this comment.
The two-line reorder is fine and this is the right layer to fix it at: a join at the owner cannot cover the plugin thread, and the WorkPool is process-wide, so publish-last in the poster is the only shape that works. Requesting changes because the PR's account of why it is correct does not hold up, and the lint that ships as the regression guard enforces that account rather than the real rule. Details inline; summary here.
1. The receiver, not the statement order, is what has to change
The description and the new SAFETY comment say the hand-off is sound because nothing on self is touched after the push. In the scenario the PR describes, the owner frees the Box while enqueue_task_concurrent(&mut self) and publish(&self) are still on the poster's stack. A reference argument is protected for the duration of the call, and freeing protected memory is UB under Tree Borrows (what bun run rust:miri runs) and Stacked Borrows, independent of whether self is read again. The repo already documents this rule in test/internal/source-lints/self-receiver-reclaim.test.ts:19-28, and this file already follows it for file_polls_raw(this: *mut Self).
I ran a reduction with this PR's exact statement order under Miri with tree borrows, freeing the struct between the queue's publishing store and the poster's return. &mut self at the entry point is rejected; &self at the entry point (the #37691 shape) is rejected, on the loop_ field this PR pre-reads; this: *const Self at the entry point, still calling UnboundedQueue::push(&self) on the field, is accepted. The Miri reduction in the description passes because it was written with this: *const Mini, which is not what the diff ships.
So: make enqueue_task_concurrent, enqueue_task_concurrent_with_extra_ctx and publish take this: *const Self, and pass the pointer the callers already hold (LinkerContext::any_loop_mut, the vtable at the bottom of this file) instead of a reborrow. The queue does not need to change. #37691 touches the same receivers; one PR should own the conversion and the other rebase.
2. The load-bearing invariant is in a file the PR does not touch
The fix also depends on UnboundedQueue::push_batch not touching the queue struct after the node is visible. That is true today (src/threading/unbounded_queue.rs: back.swap and front.store are each the last access to self in their branch) but is written down only in this PR's description. A debug_assert! after the swap, or a -> bool was_empty, reintroduces the UAF with the lint green. Please put a one-paragraph contract on push/push_batch naming the rule and this caller, and make the publish comment cite it.
3. The lint does not see the fix from (1)
mini-loop-publish-last.test.ts matches the token self. Running its check() unchanged against the description's own enqueue_buggy(this: *const Mini) reduction, or against both entry points converted to a raw receiver with the original buggy order, gives zero offenders while the second test still passes. After the receiver conversion the file reports coverage it does not have. Either drop it, pin the two bodies as literal snapshots the way #37691 does, or make it capture the receiver expression and add the description's reduction as a must-fail fixture. Any of those is much smaller than what is here.
4. The description puts the window on a path that is already joined (minor)
On the successful Bun.build path the parse workers are joined before init_and_run returns (wait group released after the callback in src/threading/ThreadPool.rs, and generate_chunks_in_parallel waits on the pool), so "the shared WorkPool is not waited on" is not true for the case narrated. The exposed cases are the error and cancellation returns after wait_for_parse, scan_module_graph_from_cli's early return, and the plugin JS thread on every path. Worth correcting because the ASAN evidence listed exercises the joined path, and because it steers a reader toward adding a join, which cannot work here.
Checked and not raising
Fixing at the owner instead (not viable, above); the #37701 rewrite to self.wakeup() (merge ordering only); the uws loop being freed at thread exit (pre-existing, this PR narrows it); push_batch front/back ordering, a late wakeup landing in the next build on the same uws loop, the uv loop living in the owner's TLS on Windows, the other per-scope mini loop owners under --watch and in build_command.rs. None of these change the verdict.
|
@robobun adopt |
|
Status: approved in the leaf shape (head 0caaf9b: mini loop entry points on raw pointers, |
|
@alii reworked along the lines of your review, in 5a87a34 (code) and 0d08ae5 (comment trims); title and description rewritten to match.
Not changed here, and stated in the description: the frames above these entry points ( CI: the build-bun steps on several lanes are failing to download vendored dependencies (c-ares, mimalloc, WebKit, lol-html); main's current build has the same failure, so those are not from this diff. |
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/bundler/bundle_v2.rs:4346-4359—on_load_async(&mut self, load: &mut Load)/on_resolve_async(&mut self, ...)still hold a protected&mut BundleV2(and&mut Load/&mut Resolve) across the Mini-arm publish, so the same protector UB alii demonstrated forenqueue_task_concurrent(&self)remains one frame up on theBox<BundleV2>allocation. The PR modified both bodies and its raw-receiver rework stops one frame short; per REVIEW.md ("fix the whole class") andself-receiver-reclaim.test.ts:19-28, these receivers (and their C-ABI callers inJSBundler.rs) need to become raw pointers as well.Extended reasoning...
What the bug is
The PR was reworked per alii's review so that
MiniEventLoop::enqueue_task_concurrent*andUnboundedQueue::push_batchtakethis: *const Selfinstead of&self/&mut self, because under Tree Borrows (and Stacked Borrows) a reference argument is protected for the whole call, and freeing the pointee while that protector is live is UB regardless of whether the body readsselfagain. The PR's own doc comment onenqueue_task_concurrentnow states this: "A reference argument would assert*thisfor the whole call."But one frame up,
BundleV2::on_load_async(&mut self, load: &mut jsc_api::JSBundler::Load)(bundle_v2.rs:4324) andon_resolve_async(&mut self, resolve: &mut ...)(bundle_v2.rs:4366) — both modified in this PR — still take&mut self. In theMiniarm (theBun.buildcross-thread case) they callMiniEventLoop::enqueue_task_concurrent_with_extra_ctx(&raw const **mini, ...). Once that call publishes the task, the bundle thread can drain it, satisfyis_done(), return frominit_and_run, and dropbv2: Box<BundleV2>(js_bundle_completion_task.rs:1236) — while the JS plugin thread'son_load_asyncframe, with its protected&mut BundleV2argument, is still live. That is exactly the shape alii's Miri reduction rejected ("deallocation ... is forbidden ... protected tag"), on theBundleV2heap allocation instead of theMiniEventLoopone.The specific code path
- JS plugin thread: C++ calls
JSBundlerPlugin__onLoadAsync→bv2_mut(this.bv2).on_load_async(this)(JSBundler.rs:1625), which reborrows the raw*mut BundleV2backref to&mut BundleV2and passes it as the&mut selfreceiver. Under Tree Borrows this creates a protector on theBundleV2allocation for the whole call. on_load_asyncmatchesself.any_loop()→Mini(mini)and callsenqueue_task_concurrent_with_extra_ctx(&raw const **mini, ptr::from_mut(load), ...)(bundle_v2.rs:4352-4360). The task is published to the bundle thread's queue.- Bundle thread:
tick_oncedrains the concurrent queue, runson_load_mini(which forms&mut *load— see below), the pass finishes,init_and_runreturns, and its localbv2: Box<BundleV2>drops, deallocating theBundleV2. - JS plugin thread:
on_load_asyncis still on the stack (in the match/fn epilogue after the enqueue returns). Its&mut selfprotector on the now-freedBundleV2allocation is still live → Tree Borrows rejects the deallocation.
The
&mut Load/&mut Resolveargument has the identical problem: it is enqueued as the task ctx, andon_load_mini/on_resolve_minion the bundle thread form&mut *loadwhile the JS thread's protected&mut Loadis still on the stack.Why existing code doesn't prevent it
alii's third review comment explicitly named the plugin thread (
on_load_async,on_resolve_async,on_defer) as "exposed on every path" — unlike the parse workers, nothing joins the plugin thread beforeBox<BundleV2>drops. The PR's original analysis of these two functions ("return straight out to C++, which does not touch the request again") uses the statement-order framing that alii's review corrected forenqueue_task_concurrent: the protector applies whether or notselfis read after the enqueue. The rework applied the raw-receiver conversion toMiniEventLoopandUnboundedQueuebut stopped at the frame above them.The repo already states this rule:
test/internal/source-lints/self-receiver-reclaim.test.ts:19-28— "a reference argument is protected for the duration of the call, and deallocating protected memory is rejected by both Stacked Borrows ... and Tree Borrows ... even ifselfis never touched again. The free has to go through the raw pointer the owner actually holds, which is why the tree's functions that end in a free takethis: *mut Self."The
ParseTask/ServerComponentParseTaskcompletions do not have this problem: they reachany_loop()throughworker.ctx.linker.any_loop()whereworker.ctxis aBackRef<BundleV2>— the&BundleV2there is a temporary formed by theDerefchain inside thematchscrutinee, not a protected fn argument spanning the enqueue.Impact
Same as the bug the PR fixes: Tree-Borrows-rejected UB on the
Bun.buildplugin path. The runtime window is one instruction wide (fn epilogue on the poster while the bundle thread finishes), so it has never been observed, but it is the same class REVIEW.md requires be closed together: "Fix the whole class in the same PR (same-class sites are ONE concern, not scope creep)." The PR modified both function bodies (theany_loop_mut→any_loopand raw-pointer-enqueue changes) and its post-review soundness argument ("a reference argument would assert*thisfor the whole call") is exactly what applies here.How to fix
Convert the receivers to raw pointers, mirroring what the PR did for
MiniEventLoop:pub unsafe fn on_load_async(this: *mut Self, load: *mut jsc_api::JSBundler::Load)and likewise foron_resolve_async. Readany_loop()via(*this)before the enqueue (or via the rawlinker.r#loopNonNull, asis_donenow does).- Callers in
JSBundler.rsalready hold the raw pointers:JSBundlerPlugin__onLoadAsynchasthis.bv2: *mut BundleV2andthis: &mut Loadfrom C++ — passthis.bv2andptr::from_mut(this)(or make the FFI signature take*mut Load) directly instead of routing throughbv2_mut(). Same forJSBundlerPlugin__onResolveAsyncand the twoJSBundlerPlugin__addErrorarms. - The
&mut Load/&mut Resolveargument becomes*mutfor the same reason (it is handed to the other thread as the task ctx while the reference protector on this thread is still live).
Step-by-step proof
Using alii's own reduction shape with
-Zmiri-tree-borrows:struct BundleV2 { mini: Mini } // Box'd; init_and_run drops it fn on_load_async(bv2: &mut BundleV2, freed: &Receiver<()>) { unsafe { Mini::enqueue(&raw const bv2.mini) }; // publish (raw, as in this PR) freed.recv().unwrap(); // bundle thread ran task, dropped Box<BundleV2> // fn epilogue: &mut BundleV2 protector still live → deallocation rejected }
Miri rejects this with "deallocation ... is forbidden ... protected tag (currently Unique)" on the
BundleV2allocation — the same output alii posted forenqueue_task_concurrent(&mut self). Changing the signature tofn on_load_async(bv2: *mut BundleV2, ...)and reading&raw const (*bv2).minibefore the publish is accepted.Concretely in this PR:
bundle_v2.rs:4324on_load_async(&mut self, ...)runs on the JS plugin thread viabv2_mut(this.bv2)(JSBundler.rs:1625). At bundle_v2.rs:4352-4360 it publishes to&raw const **mini. If that task is the last thing the pass was waiting for, the bundle thread'sinit_and_runreturns andbv2: Box<BundleV2>(js_bundle_completion_task.rs:1236) drops, freeing the allocation the JS thread's&mut selfprotector still covers.on_resolve_asyncat 4366/4391-4399 is identical. - JS plugin thread: C++ calls
There was a problem hiding this comment.
I reviewed the reworked revision and the bug-hunting pass found no issues. Given this changes the receiver shape and safety contract of UnboundedQueue::push_batch and the two MiniEventLoop::enqueue_task_concurrent* entry points — a rework alii specifically requested based on the Miri reduction — their sign-off on the final shape is the right next step.
Checked: every any_loop_mut caller is converted (no stragglers in-tree); push_batch is pub(crate) with push/push_raw as its only callers, so the signature change is contained; BackRef::as_const_ptr exists for the three EventLoopHandle::Mini call sites; the &raw const **mini at the four bundler sites takes the address of the boxed MiniEventLoop without forming a reference through the enum arm's &Box.
Also checked the source-lint: fnSource's brace-at-header-indent close matcher relies on rustfmt, which the header comment states, and the self-test covers the \b word boundary and the missing-fn error path.
Extended reasoning...
Overview
This PR converts MiniEventLoop::enqueue_task_concurrent / enqueue_task_concurrent_with_extra_ctx and UnboundedQueue::push_raw / push_batch from &mut self / &self receivers to this: *const Self, and reorders the body so loop_ is read before the publishing store and nothing on *this is touched after it. All eight call sites (four bundler posting paths via LinkerContext::any_loop — now shared — plus ConcurrentPoster::post_mini, the process waiter thread, and the shell yes builtin) are updated to pass raw pointers. A ~150-line source-lint test pins the four function bodies as inline snapshots.
Security risks
None user-facing. The change is a memory-safety soundness fix (a one-instruction-window UAF plus an aliasing-model protector violation) in the mini event loop's cross-thread posting path. It does not touch auth, crypto, network parsing, or user-input handling.
Level of scrutiny
High. This is core threading/event-loop code with a subtle aliasing-model argument (Tree Borrows / Stacked Borrows protectors on reference arguments, and interior-mutability exemptions defeated by QueuePadded's alignment padding). It changes a crate-visible primitive's signature to unsafe fn(*const Self, ..). The first revision was a statement-reorder that alii's review showed was insufficient under Miri; this is the full rework. The description also explicitly interacts with two other open PRs (#37691, #37701) and scopes out a same-class issue in the frames above these entry points to #37691.
Other factors
- alii left a detailed changes-requested review; the author reworked and replied point-by-point, and all of alii's threads are marked resolved. But alii has not yet re-reviewed the reworked commits (5a87a34 + 0d08ae5), and the rework departs from one of alii's suggestions ("the queue can stay as it is") with a stated Miri-backed reason (the
QueuePaddedpadding byte). That specific departure deserves a human eye. - Comment-cop bot flagged several long comments; the author shortened most and kept three as
# Safetysections that alii's review explicitly asked for — reasonable, but a maintainer should confirm those keeps. - CI is currently failing on unrelated vendored-dependency downloads (also failing on main per the author's note); the PR's own test lane has not run green on all platforms yet.
- The change is not simple or mechanical; it is exactly the kind of concurrency/lifetime reasoning that REVIEW.md flags as the most-blocked category. Auto-approval is not appropriate here.
|
@alii round three is in 06489e3; description updated, threads replied to.
One thing you should know for sequencing, since you asked for one PR per class: three other open PRs touch these same functions from adjacent angles. #37746 (field-wise access to an outstanding request while the pass writes its list link) also carries the string copies and the post-last ordering, which this PR now has, so it shrinks to the field-wise part on rebase; #37709 copies the plugin pointer into the requests and reallocates While reading |
alii
left a comment
There was a problem hiding this comment.
Round four does what it says.
dispatch.rshandsResolve/Loadon raw; bothrun_on_js_threadcopy the strings and the plugin handle out through(*this).bv2before the chain runs and hold only the raw request across it;match_on_*take the strings by value. The one&mutleft on that path is the JS-sidePlugin, which the bundle thread never touches.unget()moved ahead of the hand-off,post_resulttakes the pass pointer instead of&Worker, and the server-component task goes through the samepost_result.- Re-ran last round's probe: with
let _ = (*bv2).any_loop();injected after the publish inpost_result,mini-loop-post.test.tsnow fails that test (12/13); clean it is 13/13.task_callback_wrapand bothrun_on_js_threadare body-pinned.
CI red is the vendored-tarball download failure on the darwin aarch64 / freebsd build steps and their dependents, present on main's current build as well; every test lane is green.
Thanks for taking the optional point too.
alii
left a comment
There was a problem hiding this comment.
Lint removed as asked; the eleven src files are byte-for-byte what was approved in round four (the only other delta is main's merge). The Windows aarch64 test/bake/deinitialization.test.ts teardown segfault on this build has hit 26 unrelated PRs in the past week, so it is not this diff.
|
@robobun rebase |
382abdb to
97642f5
Compare
|
@alii rebased onto current main as a single commit (97642f5). The only overlap with main since the last merge was #37625's comment rewording in the |
|
Stepping back from the implementation, which is fine, to whether this should merge in this shape. I don't think it should, and I'd like to supersede it rather than land it. Why
What I'd like instead, as three small PRs:
So: please force-push this branch down to (1) and drop the bundler / JSBundler / dispatch / LinkerContext changes from it. I'm withdrawing the approval until then. If you think the 11-file shape is still the right call, say why here once and I'll take it to the other maintainers rather than going another round. |
alii
left a comment
There was a problem hiding this comment.
Superseding rather than merging in this shape; see the comment above for the reasoning and the three-PR plan. Withdrawn approval until the branch is cut down to the leaf change.
|
Agreed, no case to take further: the bundler half belongs in #37997's single |
…last Other threads hand work to a MiniEventLoop by pushing onto its queue and then waking it. Bun.build makes one loop per pass and frees it when the pass returns, and the task being posted can be the last thing the pass was waiting for, so the owner may run it and free the loop while the poster is still inside enqueue_task_concurrent. On main the body then read loop_ out of the freed struct; and the &mut self receiver (or a &self) asserts the struct for the whole call, which the aliasing models reject as soon as the owner drains the queue. enqueue_task_concurrent and enqueue_task_concurrent_with_extra_ctx take this: *const Self, read loop_ before the push and wake through the copy. The push goes through a new UnboundedQueue::push_raw, with push_batch on a raw pointer; push(&self) is unchanged for its other users (the queue's alignment padding is not interior mutable, so a &self on the queue would be rejected too once the owner frees it). Callers change spelling only. bun_threading joins the Miri set, and unbounded_queue.rs gets the two-thread reduction as a test: the consumer frees the queue as soon as the node is visible, so any access to the queue after the publishing store is reported as a race with the free. WaitGroup's own handoff test is ignored under Miri for now: Tree Borrows reports it as violated (finish(&self) is still on the finisher's stack when wait() returns), which is tracked separately.
97642f5 to
4a11cc1
Compare
|
@alii cut down as described; the branch is now 4a11cc1 (+ a two-line comment trim in 0caaf9b), 12 files, +186/-36, and the title and description are rewritten for this scope.
#37701 can rebase onto this once it lands; I will leave steps 2 and 3 of your plan to #37997 and a follow-up unless you want me to open the third one. |
alii
left a comment
There was a problem hiding this comment.
This is the shape I asked for, thanks for turning it round quickly.
- Bundler / JSBundler files are call-site spelling only (
&raw const **mini); every frame is as on main. - Ran the new test under Miri myself (
nightly-2026-07-20,-Zmiri-tree-borrows): passes as shipped in ~4 s; with one(*this).back.0.load(...)inserted after thefrontstore inpush_batchit fails on the first iteration withData race detected between (1) atomic load ... and (2) deallocation, pointing at the inserted line. So the contract is held by CI, not by the doc comment. - The
WaitGroupignore is honest about why and is the first catch of the crate joining Miri; fine as a separate report.
Steps 2 and 3 can stay with #37997 and a follow-up. #37701 rebases onto this.
|
Thanks for re-running it. Nothing further from my side; CI on 0caaf9b is at 177 lanes green with two still queued, so it is yours to merge when they finish. |
…t the release (#38330) ### Problem - `WaitGroup::wait()` returning is what lets the owner free the group, so once the last `finish()` has published the count and released the mutex, the finishing thread must not touch the group at all. #34458 moved the last real access in front of that release. What is left is that `finish(&self)`, `Mutex::unlock(&self)`, the per-OS unlock impls, and the contended-path `Futex::wake(&self.state)` all still hold references into the group through and after the release, and a reference argument asserts the memory for the whole call. - Miri rejects the waiter's free for exactly that reason. `bun run rust:miri -p bun_threading` (the crate is not in the Miri set yet) fails the crate's own test, `wait_group::tests::wait_returning_means_finish_is_done_with_self`, under Tree Borrows with `Undefined Behavior: deallocation through <tag> at alloc[0xc] is forbidden ... transitioned due to a protector release --> src/threading/Mutex.rs:193` (end of `DebugImpl::unlock`; offset `0xc` is padding inside the mutex, so this hits even though every field is an atomic), and under the default Stacked Borrows with `... would remove [SharedReadOnly ...] which is strongly protected`. Every seed I tried (12) fails within 2..200 iterations. - It is not only padding: when the waiter re-locked while the finisher still held the mutex, `FutexImpl::unlock` (src/threading/Mutex.rs:397 on main) forms `&self.state` for the wake after the releasing swap, i.e. possibly after the waiter has freed the group. In an instrumented Miri run that happened in 6 of 1500 iterations. On a real kernel the wake is harmless (a private `FUTEX_WAKE` only uses the address as a key), but the reference is still formed on freed memory. - In-tree callers with that lifetime: `LinkerContext`'s two source-map groups. `generate_chunks_in_parallel` frees the task slab on the line after `wait()` (src/bundler/linker_context/generateChunksInParallel.rs:78), and the `Bun.build` error path waits on both groups and tears down the whole `BundleV2` right after (src/runtime/api/js_bundle_completion_task.rs:1267). That is the path #34458's ASAN report came from. `ThreadPool`'s group is joined before the pool drops and the Windows install queue's group is a `static`, so those two are fine with `&self`. ### Fix - `WaitGroup::finish_raw(this: *const Self)` does the work through raw pointers, fast path included (a fast-path CAS can be the second-to-last decrement, with another finisher letting the waiter free the group while this frame is still live). Its last access to the group is the store that releases the mutex. - `Mutex::unlock_raw(this: *const Self)` and raw-pointer `unlock_raw` impls for the futex, Darwin and Windows backends, so no frame between `finish_raw` and the releasing store holds a reference. `unlock(&self)` delegates to it; `Bun__unlock` calls the impl directly; the `os_unfair_lock_unlock` extern takes the address (the Windows one already did). - `Futex::wake_raw(*const AtomicU32)` for the unlock tail; `wake(&AtomicU32)` delegates to it, so the other callers are unchanged. It is a safe fn because every backend's wake side only keys on the address and never reads the word (Linux `get_futex_key` for private futexes, `__ulock_wake`, `RtlWakeAddress*`, `_umtx_op` private wake, `memory.atomic.notify`); the worst a freed or reused address can produce is a spurious wakeup, which every wait loop already tolerates. - `finish(&self)` stays for groups kept alive by something other than `wait()` and delegates to `finish_raw`; its doc says when it is and is not allowed. The two `LinkerContext` finishers use `finish_raw` through `ParentRef::as_const_ptr`, so the finish is the last statement in the task that touches the context. - Why this is the right shape: the memory-level ordering from #34458 is unchanged; this only changes which pointers are live across the release, which is the thing both aliasing models (and LLVM's `dereferenceable` on reference arguments) reason about. It is the same shape as `UnboundedQueue::push_raw` in #37883 and the tree's `this: *mut Self` convention for functions that end in a free (test/internal/source-lints/self-receiver-reclaim.test.ts). - Linux `Futex` wake no longer panics on `EFAULT` (the FreeBSD backend already tolerated it). A real kernel only returns it for an address outside user space, which no caller can produce (every caller just did an atomic op on the same word), while Miri returns it for a word that has since been freed, which is now a documented-legal input; with the panic kept, the fixed test fails under Miri about once per 250 iterations (the 6/1500 above). The `futex_3arg` SAFETY comment in `bun_sys` is corrected to match (a WAKE only uses `uaddr` as a key). - Verified with: - The crate's own test (`wait_group::tests::wait_returning_means_finish_raw_is_done_with_the_group`), which now finishes through `finish_raw`. `bun_threading` is added to `MIRI_CRATES` and `src/threading/**` to the Miri workflow's paths, so the existing `cargo miri test` CI job is the automated check: it fails on main's `src/threading` (the diagnostic above) and passes here. Under `cfg(miri)` the test runs 500 iterations (about 15s; the unfixed shape fails within 200 on every seed tried), natively still 10,000. Passes on 12 seeds plus a 10,000-iteration run on the default seed, under both Tree Borrows and Stacked Borrows; the full `bun run rust:miri` set passes (`bun_threading` takes 15s, `bun_paths` takes 75s for comparison). - `cargo check -p bun_threading --tests` on linux-gnu, linux-musl, android, both darwin and both windows-msvc triples and freebsd, plus `--release` (the `ReleaseImpl`-direct path) on linux, darwin and windows; `cargo check --workspace` on the host; `cargo clippy --no-deps` on `bun_threading` and `bun_bundler`; the Windows-target clippy finding set is identical to main's. - `bun bd test test/bundler/bun-build-api.test.ts` (52 pass) and a 20-round loop of a 200-module `sourcemap: "external"` build plus a failing sourcemap build under the ASAN debug build, which exercises both `LinkerContext` call sites and the error-path teardown. - Overlap with open PRs: #37883 adds `bun_threading` to the Miri set with this test `#[ignore]`d under Miri; this PR makes it pass, so whichever lands second drops the ignore (the `MIRI_CRATES` and workflow lines are identical). #36481 introduces stack-scoped groups whose `BatchDone` drop calls `(*ptr).finish()` relying on this property; with this change that should be `WaitGroup::finish_raw(ptr)`, noted there. - Deliberately not in this PR: the same shape exists with other primitives in two places outside `bun_threading`, `SingleHTTPChannel::write_item(&self)` in src/http/AsyncHTTP.rs (`send_sync` frees the channel right after `read_item` returns) and `process_http_callback(&mut self)` in src/runtime/webcore/s3/download_stream.rs (`on_response` frees the task once that unlock lands). Both need a raw-unlock path for the guard types rather than `WaitGroup` changes, so they are tracked separately. `ResetEvent::set(&self)` has the same tail but its only user is a process-lifetime `BundleThread`, and the `Condvar` notifiers I looked at (`VmHandle`, `HTTPThread` shutdown) hold an `Arc` or a `static`. ### Background - Protectors: in Rust's aliasing models (Stacked Borrows, and Tree Borrows, which `rust:miri` uses) a reference passed as a function argument is "protected" for the duration of the call: the callee may assume the memory stays valid and unchanged by others until it returns, and freeing memory that a protected reference covers is undefined behavior even if the callee never touches it again. This is what lets rustc mark reference arguments `dereferenceable` for LLVM. A raw pointer argument carries no such assertion, which is why a function whose job ends by letting another thread free the object takes `*const Self`. - Padding: a `&Mutex` covers the struct's padding bytes too, and padding is not interior-mutable, so a struct made only of atomics still gets the strict treatment for those bytes. That is what the `0xc` in the diagnostic is. - Futex wake is address-keyed: `wait` sleeps on an address after comparing the word; `wake` looks the address up in the kernel's (or runtime's) waiter table without reading user memory. This is the property every futex-based mutex relies on so that the thread that acquires the lock next may free it; it is also why a wake on a freed address is harmless and why `wake_raw` needs no `unsafe`. - `WaitGroup::finish` publishes the final decrement under the group's mutex (since #34458), so `wait()`, which checks the count under the same mutex, cannot return before the finisher's unlock; the releasing store inside that unlock is therefore the exact point after which the group may be gone. <details> <summary>Miri diagnostic on main, and the iteration counts</summary> ``` $ bun run rust:miri -p bun_threading test wait_group::tests::wait_returning_means_finish_is_done_with_self ... error: Undefined Behavior: deallocation through <620185> at alloc217837[0xc] is forbidden --> library/alloc/src/boxed.rs:2002:17 = help: the accessed tag <620185> has state Reserved (conflicted) which forbids this deallocation (acting as a child write access) help: the accessed tag <620185> was created here, in the initial state Reserved --> src/threading/WaitGroup.rs:115:17 drop(Box::from_raw(wg)); help: the accessed tag <620185> later transitioned to Reserved (conflicted) due to a protector release (acting as a foreign read access) on every location previously accessed by this tag --> src/threading/Mutex.rs:193:6 (end of DebugImpl::unlock) ``` Depending on the interleaving the same test also fails as `deallocation ... is forbidden ... the accessed tag is foreign to the protected tag <..> (currently Frozen) ... protected tag was created here: Mutex.rs:66 pub fn unlock(&self)`, and under Stacked Borrows (`MIRIFLAGS=""`) as `not granting access to tag <..> because that would remove [SharedReadOnly for <..>] which is strongly protected`. Iteration at which the unfixed shape fails under Miri, by `-Zmiri-seed`: `0: 2, 1: 199, 2: 75, 3: 16, 4: 75, 5: 10, 6: 10, 7: 113, 8: 113, 9: 71, 10: 36, 11: 51`. Instrumented run of the fixed code, 1500 iterations, default seed: 2051 contended unlocks, 6 of whose wakes ran after the waiter had already freed the group (Miri returned `EFAULT`), 0 aliasing reports. </details>
Problem
MiniEventLoopwithenqueue_task_concurrent/enqueue_task_concurrent_with_extra_ctx(src/event_loop/MiniEventLoop.rs): push the task onto the loop's queue, then wake the loop.Bun.buildmakes one loop per pass and frees it when the pass returns, and the task being posted can be the last thing the pass was waiting for, so the owner can run it and free the loop while the poster is still inside the call.self.loop_after the push (MiniEventLoop.rs:389 / :416 before this change): a read of the freed struct. Never observed; the window is the gap between two adjacent instructions on the poster, so there is no runtime repro.&mut self(or&self) asserts the struct for the whole call, and Miri rejects both as soon as the owner drains the queue, free or no free (matrix below). The same holds one level down forUnboundedQueue::push(&self): the queue's alignment padding is not interior mutable, so a&selfon the queue is rejected once the owner frees it mid-push.Fix
this: *const Self, readloop_, push, and wake through the copy; nothing on*thisafter the push. The uws loop they wake belongs to the owning thread and outlives the struct (loop_ptrinvariant).UnboundedQueue::push_raw, andpush_batch(crate-private, one caller) carries the body on a raw pointer, documented as: thenext/frontstore publishes, nothing after it may touch the queue.push(&self)is unchanged for every other user.BackRef::as_const_ptr()inConcurrentPoster::post_mini, the process waiter thread and the shellyesbuiltin;&raw const **miniat the five bundler sites. No bundler frame is rewritten here; the frames above these entry points are bundler: route every cross-thread event to the bundle thread through one post() #37997'spost()(see scope).bun_threadingjoinsMIRI_CRATES(and the Miri workflow's path filter), andunbounded_queue.rsgets the two-thread reduction as a#[test]: the consumer frees the queue the moment the node is visible, for both publishing stores. Under Miri any access to the queue after the publishing store is reported as a race with that free on the first iteration (checked by adding one back:Data race detected between (1) atomic load ... and (2) deallocation); natively the test checks delivery. It cannot see a regression of the receiver shape itself (that would need the free to land inside the call, which Miri's scheduler does not produce in practice); that part is the signature.WaitGroup's own hand-off test fails under Tree Borrows:wait()can return whilefinish(&self)/Mutex::unlock(&self)are still on the finisher's stack, so the waiter's free is rejected. That is the same class, separate code; it is ignored under Miri with a note and reported separately so the crate can join now. The rest of the crate's tests pass, and the full default set still passes (bun run rust:miri, the crate adds about 5 s).bundler_plugin.test.ts(55 pass),bundler_defer.test.ts(10), 200Bun.buildpasses that fail after parsing (the exposed worker path),bun execwithcp -R/rm -rf(post_mini), the waiter-thread half ofbun-install-lifecycle-scripts.test.ts(54 pass; the one local failure is the pre-existingbun-on-PATH assumption, identical on the released binary);cargo checkfor windows-msvc and aarch64-darwin, clippy and rustfmt on the touched crates.Scope: this is step 1 of the plan in the review thread. The frames above these entry points on the
Bun.buildchains (on_load_asyncand friends,run_from_thread_pool) are replaced by #37997's singlepost(), which is where the same discipline goes next, once; hoisting the per-pass loop onto the bundle thread is the third step. #37701 rebases onto this. Earlier revisions of this PR carried the bundler rewrites; they are in the history below and superseded.Miri matrix (same result under Tree Borrows and Stacked Borrows)
One poster thread, one owner thread; the owner pops the node and frees the
Box<Mini>as soon as it has it, and the poster parks at the point the variant names until that has happened. The queue isunbounded_queue.rscopied verbatim,QueuePaddedincluded;Minihas the field shape ofMiniEventLoop(a plain fifo, the queue,loop_).&mut self(main)self.loop_&mut(reported before the free is even reached)&mut self->publish(&self)(this PR's first revision)loop_read first&selfloop_read first&self*const Selfloop_read first,push(&self)pushreturns*const Selfloop_read first,push(&self)push, after the publishing storepush's&selfprotects the padding byte at offset 8 ofQueuePadded; passes with therepr(align)removed*const Selfloop_read first,push_raw(shipped)*const Selfloop_read first,push_raw(shipped)*const Self(*this).loop_Background
MiniEventLoopis Bun's event loop for threads that run no JavaScript (bundler,bun install,bun exec): a task queue plus a pointer to the thread's uws loop. The uws loop is the OS poller, one per thread, alive for the whole thread; waking it makes a sleeping loop look at its queue. An awake loop drains its queue on its own, which is how the owner can be done with a task before the poster's wake runs.UnboundedQueueis the intrusive lock-free queue under the task queue. One store makes a pushed node visible (frontwhen the queue was empty, the previous tail'snextotherwise); from then on the consumer can pop and run it. That store is "the publish".bun run rust:miriuses Tree Borrows, which the Miri workflow runs in CI for the crates inMIRI_CRATES): a reference passed as an argument stays valid, and for&mutexclusive, until the function returns, even if the function never uses it again. Another thread freeing or writing that memory during the call is undefined behaviour. A raw pointer argument carries no such rule. Atomics are exempt; padding bytes are not, which is what catchespush(&self).