node:fs: post the stat watcher's JS-thread hops through the watcher's own pointer - #37857
node:fs: post the stat watcher's JS-thread hops through the watcher's own pointer#37857robobun wants to merge 1 commit into
Conversation
… own pointer StatWatcher::post_to_js_thread(&self) queued self.as_ctx_ptr(), a pointer spelled from the shared reference, and the JS thread releases the hop's ref through that pointer. On the initial-stat path that ref can be the watcher's last one, so the hop frees the watcher through a pointer with shared provenance, possibly while post_to_js_thread's &self and the &self of the loop_handle field it posted through are still live arguments. The periodic path posted the same kind of pointer, and the initial hop's pointer is also what append() put in the scheduler's queue, so the queue's own releases went through it as well. post_to_js_thread and restat now take this: *mut Self, which every caller already holds (the initial-stat task's field, the queue entry); the posting function finishes its field accesses, clones the loop handle out and posts this itself. The as_ctx_ptr helper goes away with its last two users. test/internal/source-lints/ctx-ptr-publish.test.ts bans posting an as_ctx_ptr() pointer as a task, directly or through a local; it fails on the previous version of this file and has no other hits in the tree.
|
Warning Review limit reached
Next review available in: 1 minute 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 (2)
Comment |
|
Updated 1:05 PM PT - Aug 12th, 2026
❌ @robobun, your commit bb73d87 has some failures in 🧪 To try this PR locally: bunx bun-pr 37857That installs a local version of the PR into your bun-37857 --bun |
|
Status: structural soundness fix, no runtime repro by construction (the poster reads nothing after the push today). Reproduced as:
Behavioural coverage: |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it reworks unsafe cross-thread refcount hand-off and pointer provenance in the stat watcher, a human look would still be worthwhile.
What was reviewed:
- Refcount take/release points in
post_to_js_thread,restat, andrun_ownedare unchanged; only the pointer that carries them changed from&self-derived to the allocation's own. ParentRefis a bareNonNullwrapper with noDrop, sothis_refstaying in scope past the post inrun_owned(where the hop may hold the last ref) does not touch freed memory; last deref of it is inside the match, before the post.- The cloned
loop_handleinpost_to_js_threadand the pre-existing clonedhandlefor_finishedinrun_ownedmean nothing borrowing into the watcher is live acrosspost_task. - The new source lint follows the sibling lints' structure (
self-receiver-reclaim.test.ts); its.from(arm is method-position only soString::fromand similar path calls don't match.
Extended reasoning...
Overview
This PR fixes a latent pointer-provenance soundness bug in src/runtime/node/node_fs_stat_watcher.rs. post_to_js_thread(&self) and restat(&self) posted self.as_ctx_ptr() (a *mut Self derived from &self, so carrying shared/Frozen provenance) as a concurrent task. The JS-thread consumer (run_hop / release_hop) releases the hop's ref through that pointer, and when it is the last ref, frees the watcher through it — which both Tree Borrows and Stacked Borrows reject. On the initial-stat path the free can also occur while &self and &self.loop_handle are still live protected arguments inside post_task. The fix converts both to unsafe fn(this: *mut Self), clones loop_handle out before posting, and threads the allocation's original pointer (from heap::into_raw / the intrusive queue entry) all the way through. as_ctx_ptr is deleted. A new source lint bans posting an as_ctx_ptr() result as a task.
Security risks
None user-facing. This is an aliasing-model soundness fix; the observable behavior (which threads take/release which refs, and when) is unchanged. No untrusted-input parsing, auth, or crypto is touched.
Level of scrutiny
High. This is unsafe Rust in the memory-safety category REVIEW.md calls out as the most-blocked: cross-thread intrusive refcounting where the last ref can drop on either the pool thread or the JS thread, with pointer-provenance subtleties that Miri catches but ASAN cannot. The reasoning is well-documented (Miri reductions in the PR body, per-site SAFETY comments, and the fix mirrors the established post_job pattern in src/jsc/VmHandle.rs), but the correctness argument depends on non-local invariants ("the caller's queue ref keeps the watcher alive past the post" in restat; "nothing below touches it" in `run_owned") that a maintainer familiar with the sibling PRs (#37723, #37703, #37768, #37591) should confirm.
Other factors
- I verified
ParentRefis#[repr(transparent)]overNonNull<T>with noDrop, sothis_refremaining in scope afterpost_to_js_threadinrun_ownedis inert once its last deref (inside the match) completes. - The refcount ledger is unchanged:
create_and_schedule's ref still transfers to the hop viarun_owned;restatstill bumps one ref for the hop and the queue ref held bywork_pool_callbackstill keeps the watcher alive across the post and the subsequent re-push. - The new lint follows the exact structure of
self-receiver-reclaim.test.ts(sametrackedfilter, comment-strip, banned/allowed self-tests,scanned > 0guard against vacuous pass). The.from(constructor arm is scoped to method-call position, so path calls likeString::from(...)are excluded (covered by an allowed case). - The PR body reports the existing
fs.watchFilebehavioral suite and Node parallel tests pass, andtest/internal/source-lints/passes with the fix and fails without it.
Given the unsafe cross-thread lifetime reasoning involved, deferring to a human reviewer rather than auto-approving.
|
The automated review above raised no findings, so there is nothing to change from it; the PR is ready for a maintainer to look at the unsafe hand-off (the two |
Problem
fs.watchFile, same class as bundler: hand a finished Bun.build back through its pointer, not a &mut receiver #37723 and fetch: release the FetchTasklet through its raw pointer, not a &mut receiver #37703, reduced with Miri rather than observed.&self; the scheduler's queue entries were copies of it. When the ref released through it is the last one (JS closed and collected the watcher during the initial stat), the watcher is freed through a shared-reference-derived pointer.deallocation through <tag> ... is forbidden(Tree Borrows),trying to retag ... for Unique permission(Stacked Borrows).&self, and the&loop_handleit posted through, are still live call arguments. That is UB on its own.Fix
&self. The callers already hold it (the work-pool task's field, the scheduler's queue entry), and that pointer is what gets posted. The&self-to-pointer helper is deleted with its last users.Background
fs.watchFilein bun is aStatWatcher: a work-pool thread stats the path on an interval and hands the JS thread a "hop" (a task carrying a pointer to the watcher) to run the listener. The watcher is intrusively refcounted; each hop carries one ref, released on the JS thread, and the last release frees it.&Tmay only be used to read. Freeing through it is UB under both of Rust's aliasing models even if nothing ever races with it, and Miri reports it. See "Pointer provenance at FFI boundaries" in src/CLAUDE.md.dereferenceable), so another thread freeing the pointee before the call returns is UB even if the callee never touches it again. Cloning the loop handle out and posting through the clone is the existing pattern for this (post_jobin VmHandle.rs).as_ctx_ptr()is bun's helper for the read-only case (from_ref(self).cast_mut()for ctx slots), so posting its result as a task is wrong by definition. That is the rule the new lint encodes.test/internal/source-lints/holds tests that grep the tree for banned spellings; they stand in for a regression test where no runtime test can observe the bug.Original description
Problem
StatWatcher::post_to_js_thread(&self)insrc/runtime/node/node_fs_stat_watcher.rsqueued the pool-to-JS hop asThe JS thread runs the hop (
run_hop, orrelease_unrunwhen the VM is torn down first) and releases the ref the hop carries through that pointer:WatcherRefGuard/release_hopend inThreadSafeRefCount::deref(this), whose zero path isdeinit(this)->heap::take(this). Two things are wrong with the pointer it was given:&selfit was spelled from. When the hop's ref is the last one, the watcher is freed through a shared-reference-derived pointer, which both aliasing models reject regardless of when the free happens (src/CLAUDE.md, "Pointer provenance at FFI boundaries"). The hop's ref is the last one whenever JS closes and collects the watcher while the initial stat is in flight:InitialStatTask::run_ownedhands thecreate_and_scheduleref to the hop, andfinalizemay already have dropped the wrapper's ref.post_to_js_thread's&self, and the&self.loop_handlethatpost_taskwas called on, are still live arguments: the JS thread can run the task as soon as it is pushed, beforepost_taskreturns. Reference arguments are protected for the duration of their call (and annotateddereferenceablefor it), so deallocating what they point at during the call is UB even though nothing reads them afterwards.The periodic path had the first problem too:
restat(&self)took the hop's ref onself.as_ctx_ptr()and posted through the same function. And becauseinitial_stat_*_on_main_threadpass the posted pointer on toStatWatcherScheduler::append, the scheduler's queue entries carried the same provenance, so the queue's own releases inwork_pool_callbackandshutdown_for_exit(which can also be the last ref) went through it as well.Nothing in the poster reads the watcher after the push today, so this is latent: a soundness bug of the same class as #37723 and #37703, reduced below with Miri. Same area as #37768 (the scheduler's
WorkPool::schedule(&raw mut self.task)sites, different functions, no overlapping hunks) and #37591 (carries these refs asOwnedRefvalues; note that its post still goes through a&loop_handleinside the watcher, the second point above).Miri reductions of the two shapes (Tree Borrows, which
bun run rust:miriuses, and Stacked Borrows)Initial-stat shape,
&selfmethod postsself.as_ctx_ptr(), hop releases the last ref (the consumer is invoked from insidepost_task, one of the legal interleavings of the JS thread):Periodic shape,
restat(&self)takes the hop's ref onself.as_ctx_ptr(), the queue ref is released on a later pass, then the hop's ref (no protector involved; the provenance alone is enough):The fixed shape (
post_to_js_thread(this: *mut Self)cloning the handle out,restat(this: *mut Self), both paths) runs clean under both models.Fix
The callers already hold the allocation's pointer:
run_ownedhas it in the task'swatcherfield (it came fromheap::into_rawininit), andwork_pool_callbackhas the queue entry. So the two functions take it instead of forming&self:post_to_js_thread(this: *mut Self, hop)setspending_hopand clonesloop_handleout through(*this)accesses that end before the post, then poststhisthrough the cloned handle. Nothing referring into the watcher is live once the task is queued, and the pointer the hop releases through is the allocation's own. This is the shapepost_jobinsrc/jsc/VmHandle.rsalready uses ("clone the handle out first").restat(this: *mut Self)does its stat bookkeeping through the file's usualParentRefview and takes the hop's ref on, and posts,this.work_pool_callbackpasseswatcher.as_ptr().run_ownedpicks the hop in the match and posts once afterwards; theembedded_work_finishedguard already ran on a cloned handle for the same reason.as_ctx_ptris deleted with its last two users.Since the initial hop now carries the real pointer,
appendand the queue get it too, so every release of a watcher ref in the file goes through the allocation's pointer. Refs are taken and released at the same points and on the same threads as before.Test
test/internal/source-lints/ctx-ptr-publish.test.tsbans posting anas_ctx_ptr()pointer as a task, directly or via a local in the same function:as_ctx_ptr()is documented (bun_ptr::AsCtxPtr) as a shared-provenance pointer for read-only ctx slots, and a task post hands the object to a consumer that writes to, releases or frees it, so the two never go together. It fails on the previous version of this file (src/runtime/node/node_fs_stat_watcher.rs:681) and has no other hits in the tree, so there is no allowlist. A behavioural test cannot tell the two pointers apart (ASAN included), which is why the proof is structural, as in the sibling PRs.Verification
bun bd test test/js/node/watch/fs.watchFile.test.ts: 10 pass, 2 skip (covers watch/unwatch, 1000-watcher stress with unwatch during initial stats, worker termination with queued hops, ENOENT initial callback, change callbacks after unwatch).test-fs-watchfile.js,test-fs-watchfile-bigint.js,test-fs-watchfile-ref-unref.js,test-worker-fs-stat-watcher.js,test-fs-watch-file-enoent-after-deletion.jsfrom the Node suite exit 0 under the debug build.bun test test/internal/source-lints/: 85 pass; the new lint fails withsrc/reverted and passes with it.cargo clippy -p bun_runtime --no-depsand rustfmt are clean.