Skip to content

node:fs: snapshot resizable ArrayBuffer inputs for async write/writev - #34758

Open
robobun wants to merge 4 commits into
mainfrom
farm/8b8d790f/fs-write-writev-resizable-snapshot
Open

node:fs: snapshot resizable ArrayBuffer inputs for async write/writev#34758
robobun wants to merge 4 commits into
mainfrom
farm/8b8d790f/fs-write-writev-resizable-snapshot

Conversation

@robobun

@robobun robobun commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator

Repro

import fs from "node:fs";
import { open } from "node:fs/promises";
const fh = await open("/tmp/out", "w");
const ab = new ArrayBuffer(256 * 1024, { maxByteLength: 1 << 21 });
new Uint8Array(ab).fill(0x41);
const p = new Promise((res, rej) =>
  fs.writev(fh.fd, [new Uint8Array(ab)], 0, (e, n) => (e ? rej(e) : res(n))));
ab.resize(0);
await p;
EFAULT: bad address in system call argument, pwritev

fs.write fails the same way (syscall write). Node.js itself has the same race for fs.write/fs.writev (V8's BackingStore::ResizeInPlace also mprotects the shrunk tail to PROT_NONE, and node_file.cc snapshots a raw Buffer::Data()/Length() into the libuv request); Bun's threadpool just loses the race more often.

Cause

Async fs.write (args::Write::from_js) and fs.writev (VectorArrayBuffer::from_js via FdVectorIo) pin the backing ArrayBuffer and hand a (ptr, len) snapshot to the threadpool. pin() blocks transfer()/detach but ArrayBuffer.prototype.resize never consults the pin count: JSC's ArrayBuffer::resize shrink path calls OSAllocator::protect(.., readable=false, writable=false) on the tail, so the kernel's write(2)/pwritev(2) sees decommitted pages and returns EFAULT.

Fix

When an input buffer's backing is a resizable non-shared ArrayBuffer, copy the bytes into the job on the async path instead of borrowing:

  • args::Write: the async repin branch now returns an owned EncodedSlice for resizable non-shared inputs; the existing pinned borrow is kept for fixed-length and growable-shared backings.
  • VectorArrayBuffer gains an owned vector and snapshot_resizable_inputs(), which copies any resizable non-shared element into owned storage and repoints its iovec at the copy.
  • args::Writev is now a #[repr(transparent)] newtype over FdVectorIo whose from_js runs the snapshot on the async path. args::Readv stays the plain FdVectorIo alias, so output buffers are never copied (shrinking an output buffer mid-read is a caller error, and copying would discard the read bytes). All consumers keep accessing .fd/.buffers/.position through Deref.

Fixed-length ArrayBuffer (the overwhelmingly common case) stays zero-copy. Growable SharedArrayBuffer also stays zero-copy: grow() only commits additional tail pages in place, so the original extent remains valid.

This is the same approach #34751 takes for the crypto/zlib input funnels and #32189 takes for path buffers. Those PRs do not cover args::Write's repin branch or the writev collector; this one does, and it does not overlap their diffs. fs.writeFile/appendFile route their data argument through StringOrBuffer::from_js_maybe_async, which #34751 already patches.

Verification

test/js/node/fs/fs.test.ts gains, for each of write and writev:

  • a subprocess that saturates the threadpool with concurrent readFile, queues the operation on a resizable ArrayBuffer filled with 0x41, shrinks and regrows the backing (which zeroes it), and asserts the file contents are the call-time 0x41 bytes. On the unfixed build the subprocess reports ["zeroed", ...] (and occasionally "EFAULT"); with this change it prints ok.
  • a growable-SharedArrayBuffer case that exercises the resizable && !shared guard: grow() mid-flight must not copy and must still write the original bytes.

Existing writev/readv/write coverage in fs.test.ts and the Node test/parallel test-fs-write*/test-fs-writev*/test-fs-readv* suite pass unchanged; rust:check-all compiles on every target.


no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/fs/fs.test.ts

Async fs.write and fs.writev pin the caller's buffer against transfer()
but ArrayBuffer.prototype.resize never consults the pin count. When a
resizable backing store shrinks between the call and the threadpool
syscall, JSC decommits the tail pages (OSAllocator::protect with no
access), and the kernel write/pwritev sees EFAULT on the stale extent.

For an input buffer (the write side) the fix is to copy at call time
when the backing is resizable and non-shared:

- args::Write: on the async repin path, replace the pinned borrow with
  an owned EncodedSlice for resizable non-shared inputs.
- VectorArrayBuffer: add snapshot_resizable_inputs() which copies any
  such element into owned storage and repoints its iovec.
- args::Writev becomes a transparent newtype over FdVectorIo whose
  from_js runs the snapshot on the async path; args::Readv stays the
  plain alias so output buffers are never copied.

Fixed-length ArrayBuffer (the common case) and growable
SharedArrayBuffer stay on the existing zero-copy pinned path. readv and
read are intentionally unchanged: an output buffer the caller shrinks
mid-read is a caller error, and copying would discard the read bytes.
@coderabbitai

coderabbitai Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 38 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 133de6fa-99d5-4f31-92d2-69d0e4967c60

📥 Commits

Reviewing files that changed from the base of the PR and between 0a17ce6 and 325eb1e.

📒 Files selected for processing (3)
  • src/runtime/node/node_fs.rs
  • src/runtime/node/types.rs
  • test/js/node/fs/fs.test.ts

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 10:05 PM PT - Jul 19th, 2026

@autofix-ci[bot], your commit 325eb1e has 1 failures in Build #75981 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 34758

That installs a local version of the PR into your bun-34758 executable, so you can run:

bun-34758 --bun

@robobun

robobun commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator Author

Status: ready for review.

Gate proof (bun bd test test/js/node/fs/fs.test.ts -t "resizable ArrayBuffer input at call time"):

  • with src/ reverted to main: the write and writev "writes the call-time bytes" cases fail with ["zeroed", ...] from the subprocess
  • with the fix: all five tests pass

Self-review addressed in 8b8f40f: tightened the fs.write snapshot to copy only the [offset, offset+length) window (with a dedicated test), reworded the args::Writev doc comment, replaced the exact-empty stderr assertion with a combined object, and renamed the growable-SAB test since it cannot observe the !shared guard.

CI: test/js/node/fs/fs.test.ts passed on every lane in builds 75964 and 75981. Remaining reds are unrelated to this diff: build 75964's filesystem_router.test.ts resolver segfault is tracked separately; build 75981's reds are all marked [pre-existing] (test-net-connect-memleak.js) or [flaky] (passed on retry).

Comment thread test/js/node/fs/fs.test.ts Outdated
Comment thread test/js/node/fs/fs.test.ts Outdated
robobun and others added 2 commits July 20, 2026 04:24
…sertions

- args::Write: snapshot only the requested window instead of the whole
  view, and reset offset/length to match.
- Drop the misleading repr(transparent) justification on args::Writev.
- Replace exact-empty stderr assertion with a combined object per
  REVIEW.md; trim comment blocks to 3 lines.
- Rename the growable-SharedArrayBuffer test (it cannot observe the
  !shared guard) and add an offset/length window test for write.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both earlier nits are addressed and I didn't find any correctness issues this pass, but this touches memory-safety-critical async I/O plumbing (cross-thread buffer ownership, pin/unpin balance, a new Writev newtype that changes the dispatch type for every writev consumer) — worth a human sign-off on the approach.

What was reviewed:

  • Pin balance in the args::Write resizable branch — initial StringOrBuffer::from_js is non-async so unpinned; as_pinned_arraybuffer adds one pin, pinned.unpin() releases it; the replacing EncodedSlice is a no-op for both to_thread_safe and unprotect.
  • snapshot_resizable_inputs iovec repointing — Box<[u8]> heap addresses are stable across Vec::push, so iovecs stay valid as owned grows; freed by field Drop.
  • Writev newtype flows through impl_fs_argument!, the UV dispatch arm (line 875), and writev_inner/pwritev_inner via Deref — checked each .fd/.buffers/.position access still resolves.
  • Offset/length clamping in the snapshot mirrors write_inner exactly (default length = u64::MAX clamps to view.len() - off).
Extended reasoning...

Overview

The PR fixes an EFAULT / silent-corruption race when a resizable ArrayBuffer backing an async fs.write/fs.writev input is shrunk before the threadpool syscall runs. It does so by snapshotting resizable non-shared inputs into owned storage on the async path: args::Write::from_js copies the [offset, offset+length) window into an EncodedSlice, and VectorArrayBuffer gains an owned: Vec<Box<[u8]>> field plus snapshot_resizable_inputs() that repoints matching iovecs. args::Writev becomes a #[repr(transparent)] newtype over FdVectorIo so its from_js can run the snapshot while all downstream field access goes through Deref. Five new tests in fs.test.ts cover the race, the offset window, and the growable-SAB zero-copy path.

Security risks

None user-facing. The change is defensive — it copies attacker-controllable buffer bytes before handing them to the kernel, which is strictly safer than the prior borrow. No new untrusted-input parsing, no size arithmetic on external data beyond the existing clamped offset/length that already matches write_inner.

Level of scrutiny

High. This is squarely in REVIEW.md's most-blocked category: cross-thread buffer lifetime, GC pin/unpin balance, an ownership-changing enum-variant swap on the async path, and a type change (Writev newtype) that every writev consumer (posix writev_inner/pwritev_inner, the Windows UV dispatch arm at node_fs.rs:874, impl_fs_argument!) must resolve through Deref. Getting the pin count or the iovec pointer lifetime wrong here is a UAF or a leak that only shows under load. I traced each of these paths and they check out, but this is exactly the kind of change where a maintainer should confirm the approach (copy vs. e.g. teaching pin() to block resize()) is the one they want, and eyeball the newtype's interaction with the args_as! cast in the UV path.

Other factors

My two prior nits (comment length, exact-empty stderr assertion) were addressed in 8b8f40f and 325eb1e; both threads are resolved. The gate proof in the PR thread confirms the new tests fail on main and pass with the fix. CI build 75981 is in flight. The PR description references sibling PRs #34751 and #32189 taking the same approach for other funnels, which lends confidence to the pattern but also means a maintainer may want to confirm the three don't overlap or conflict.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant