node:fs: snapshot path buffers backed by resizable ArrayBuffers for async ops#32189
node:fs: snapshot path buffers backed by resizable ArrayBuffers for async ops#32189robobun wants to merge 4 commits into
Conversation
…sync ops Async fs operations capture a raw ptr/len into a buffer-typed path argument on the JS thread and read it later on a work-pool thread. The pin taken at parse time blocks transfer()/detach, but ArrayBuffer.prototype.resize does not consult the pin count: shrinking a resizable ArrayBuffer decommits its tail pages, so the pool thread's read of the stale snapshot faults. Copy the path bytes at parse time when the backing store is a non-shared resizable ArrayBuffer (paths are bounded by MAX_PATH_BYTES, and Node also copies buffer paths at call time). Growable SharedArrayBuffers never shrink and keep a stable data pointer, so they stay on the zero-copy pinned path. PathLike::Drop now frees the owned snapshot, and PathLike::Clone dupes an owned payload instead of borrowing it, mirroring the owned-String arm.
|
Updated 7:44 PM PT - Jun 12th, 2026
❌ @robobun, your commit 912b092 has 4 failures in
🧪 To try this PR locally: bunx bun-pr 32189That installs a local version of the PR into your bun-32189 --bun |
|
Status: reproduced the crash on Linux with a 20-line script against current canary (1.4.0-canary.1, several work-pool threads fault reading the decommitted pages). Fix verified locally: the new test in test/js/node/fs/fs.test.ts fails on an unfixed debug build (child exits 132 with no output) and passes with the fix across repeated runs. CI (build 62158, sha 912b092): 285 of 286 jobs green, including every build lane and all of Windows/Linux. The one red lane is darwin-14-aarch64 test-bun, which timed out: the docker-backed service tests (sql-mysql, valkey connection-failures, websocket autobahn) all failed to start their containers ("Failed to start service ...", docker-daemon errors), and init.test.ts picked up the agent checkout's own CLAUDE.md as a scaffolded agent-rule file (agent-workspace artifact, not reproducible off that runner). None of these touch the changed files (node_path.rs, types.rs, fs.test.ts); recent main builds 62125/62089/62048 are all green. The diff is ready; the remaining red is unrelated macOS agent/docker flake. |
|
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 (1)
WalkthroughThis PR prevents stale pointer reads in async filesystem operations when path arguments are backed by resizable ArrayBuffers. It snapshots path bytes at call time, updates PathLike ownership and cleanup for owned snapshots, and validates the behavior with integration tests. ChangesAsync path buffer snapshot safety for resizable ArrayBuffers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/node/types.rs`:
- Around line 1021-1030: snapshot_resizable_path_buffer() can replace the
JS-backed path with an owned snapshot (setting buffer.owns_buffer), but callers
still call arguments.protect_eat() unconditionally which roots the original JS
value and later PathLike::unprotect() can't release that root; fix by making the
protect_eat() call conditional so it only runs when the PathLike::Buffer still
references the original JS-backed value (e.g., !buffer.owns_buffer or
buffer.buffer.value.is_some()), ensuring the matching unprotect() will run for
the same acquisition; apply this same conditional change in both buffer arms
referenced around the existing protect_eat() calls so every protect has a
corresponding unprotect.
In `@test/js/node/fs/fs.test.ts`:
- Around line 4411-4416: The test collects results from fs.promises.rename but
treats a successful promise (which resolves to undefined) as indistinguishable
from the catch result, so find(c => c !== "ENOENT") can miss unexpected
successes. Change the promise collection so successful renames resolve to a
distinct sentinel (e.g., "OK" or "SUCCESS") while failures return err.code; then
search codes for anything !== "ENOENT" (or explicitly !== "ENOENT" && !== "OK")
and throw if found. Update the blocks that build all/codes/bad (the promises
array variable all, the Promise.all result codes, and the bad check) accordingly
so an unexpected success is detected.
- Around line 4403-4431: Add a third test block mirroring the two existing cases
that uses a DataView to cover the remaining supported path-buffer form: create
srcBytes from path.join(dir, "missing-dataview"), allocate a resizable
ArrayBuffer rab sized to srcBytes.length, copy srcBytes into the buffer,
construct a DataView (e.g., const view = new DataView(rab, 0, srcBytes.length)),
set dest to path.join(dir, "dest-dataview"), spawn 256 concurrent
fs.promises.rename(view, dest).catch(err => err.code) into an all array, resize
rab to 0, await Promise.all(all) into codes and assert every code is "ENOENT"
(find bad and throw if found) using the same variable names (srcBytes, rab,
view, dest, all, codes, bad) as the other blocks to keep consistency.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d54e024f-e2c3-4005-b8e7-ab773773601d
📒 Files selected for processing (3)
src/jsc/node_path.rssrc/runtime/node/types.rstest/js/node/fs/fs.test.ts
The owned snapshot no longer references the JS backing store, so rooting the original argument pairs with no release (the success path never drops the ArgumentsSlice). Plain eat() keeps the snapshot path balanced. Test: assert via every() so an unexpected resolution cannot slip through find()'s undefined result, and cover DataView as the third buffer form.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this changes Drop/Clone semantics and GC-root accounting for PathLike::Buffer on the hot path of every async fs op — worth a human pass on the ownership reasoning.
Extended reasoning...
Overview
Fixes a user-reachable segfault: async node:fs ops snapshot a raw ptr/len into a path buffer's backing store on the JS thread, then read it on a work-pool thread; if the backing is a resizable ArrayBuffer that gets shrunk in between, the pool thread reads decommitted pages. The fix copies the path bytes at parse time when the backing is a non-shared resizable ArrayBuffer, and updates PathLike::Clone/Drop so the owned snapshot is duped on clone and freed on drop. Touches src/jsc/node_path.rs, src/runtime/node/types.rs, and adds a regression test in test/js/node/fs/fs.test.ts.
Security risks
None new. This is a defensive memory-safety hardening — it removes a crash, doesn't add attack surface. The path bytes are still bounded by MAX_PATH_BYTES (validated before the snapshot via Valid::path_buffer).
Level of scrutiny
High. This is memory-lifetime code on the critical path for every async fs operation that takes a buffer path. It modifies Drop for PathLike to add a b.destroy() call and Clone for PathLike to add an owned-dupe branch. I verified MarkedArrayBuffer::destroy() is gated on owns_buffer (no-op for JS-owned backings, so the existing non-snapshot path is unaffected), and that from_string()/from_typed_array/from_array_buffer/from_js_pinned set owns_buffer consistently. The eat() vs protect_eat() split after snapshotting looks correct. But the first revision already had a GC-root balance bug (caught by CodeRabbit, fixed in 2386df1), and the author surfaced a related pre-existing leak (#32191) during the work — the protect/unprotect accounting here is subtle enough that a maintainer familiar with the run_async/ManuallyDrop<ArgumentsSlice> lifecycle should sign off.
Other factors
- All CodeRabbit findings are addressed and resolved (DataView coverage added, ENOENT assertion strengthened,
protect_eatskipped for owned snapshots). - The musl CI failures are an LTO toolchain linking issue unrelated to this diff.
- No CODEOWNERS cover the touched files.
- Test coverage is solid: spawned-subprocess crash repro covering Uint8Array view, DataView, and direct ArrayBuffer, plus a positive call-time-snapshot semantics check.
Surfaces the child's crash report or fixture error in the failure diff instead of only the exit code.
1 participant
Crash
Async
node:fsoperations that take a buffer-typed path (Uint8Array,DataView, orArrayBuffer) capture a rawptr/leninto the backing store on the JS thread, then read it later on a work-pool thread. The pin taken at parse time (#31221) blockstransfer()/detach, butArrayBuffer.prototype.resizenever consults the pin count: shrinking a resizable ArrayBuffer decommits its tail pages (JSC::ArrayBuffer::resize->OSAllocator::protect(start, len, false, false)), so the pool thread's read of the stale snapshot hits no-access pages.Repro (crashes current canary, verified on Linux; the decommit and stale read are platform-independent; several pool threads fault at once):
Found while auditing the async fs path-buffer lifetime for the Windows rename crash fleet (Sentry BUN-2V6E). Shipped 1.3.x builds predate the pin, so they are additionally exposed to plain
transfer()detach; on current main the resize hole was the remaining gap. Whether BUN-2V6E itself is this exact mechanism is not proven (its fault address pattern differs), but this is the one memory-unsafe path the audit found, and it is user-reachable.Fix
Copy the path bytes at parse time when the backing store is a non-shared resizable ArrayBuffer (
snapshot_resizable_path_bufferinsrc/runtime/node/types.rs, applied to both the typed-array and ArrayBuffer arms ofPathLike::from_js_with_allocator). Paths are bounded byMAX_PATH_BYTES, and Node also copies buffer paths at call time, so the copy both matches Node semantics and is cheap. Fixed-length buffers stay on the existing zero-copy pinned path, and growable SharedArrayBuffers (which can never shrink and keep a stable data pointer) stay zero-copy as well.Supporting changes in
src/jsc/node_path.rs, sincePathLike::Buffercan now own its allocation:PathLike::Dropfrees the owned snapshot (MarkedArrayBuffer::destroy, a no-op for JS-owned backings)PathLike::Clonedupes an owned payload instead of borrowing it, mirroring the owned-StringarmScope: this covers path arguments (
PathLike), which all async fs ops parse through the same two arms. Data arguments (StringOrBuffer) have the same bug class and are being fixed separately in #31645;fs.read/fs.writedata buffers are zero-copy by API contract and are not changed.Verification
test/js/node/fs/fs.test.tsgains a test that exercises both parse arms plus call-time snapshot semantics (a rename issued beforeresize(0)must still act on the original path). It fails on the unfixed build (child segfaults, exit 132) and passes with the fix; the existing pin tests ("keeps a buffer path argument attached") still pass.