Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 16 additions & 6 deletions src/jsc/node_path.rs
Original file line number Diff line number Diff line change
Expand Up @@ -114,12 +114,19 @@ impl Clone for PathLike {
} else {
s.borrow()
}),
Self::Buffer(b) => Self::Buffer(MarkedArrayBuffer {
buffer: b.buffer,
// The clone borrows the JS-owned backing store; only the
// original (if any) owns the allocation.
owns_buffer: false,
pinned: false,
Self::Buffer(b) => Self::Buffer(if b.owns_buffer {
// An owned snapshot (resizable-backed async path) is freed by
// the original's `Drop`; dupe so the clone is independently
// droppable, mirroring the owned-`String` arm above.
bun_core::handle_oom(MarkedArrayBuffer::from_string(b.slice()))
} else {
MarkedArrayBuffer {
buffer: b.buffer,
// The clone borrows the JS-owned backing store; only the
// original (if any) owns the allocation.
owns_buffer: false,
pinned: false,
}
}),
Self::SliceWithUnderlyingString(s) => {
// `dupe_ref()` alone leaves `utf8` empty (lib.rs:1603) — a
Expand Down Expand Up @@ -155,6 +162,9 @@ impl Drop for PathLike {
b.pinned = false;
b.buffer.unpin();
}
// Frees the snapshot copy taken for resizable-backed async
// paths (`owns_buffer`); no-op for JS-owned backings.
b.destroy();
}
Self::SliceWithUnderlyingString(s) | Self::ThreadsafeString(s) => {
core::mem::take(s).deinit();
Expand Down
38 changes: 36 additions & 2 deletions src/runtime/node/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1010,6 +1010,26 @@ pub(crate) trait PathOrFdExt {
Self: Sized;
}

/// An async fs task snapshots the buffer's `ptr`/`len` on the JS thread and
/// reads them later on a work-pool thread. The pin taken by
/// [`Buffer::from_js_pinned`] blocks `transfer()`/detach but not
/// `ArrayBuffer.prototype.resize`: shrinking a resizable ArrayBuffer decommits
/// its tail pages (`JSC::ArrayBuffer::resize` → `OSAllocator::protect`), so the
/// pool thread's read of the stale snapshot faults. Copy the path bytes instead
/// (Node also copies buffer paths at call time). Growable SharedArrayBuffers
/// never shrink and keep a stable data pointer, so they stay zero-copy.
fn snapshot_resizable_path_buffer(buffer: &mut Buffer, will_be_async: bool) {
if !(will_be_async && buffer.buffer.resizable && !buffer.buffer.shared) {
return;
}
let copy = bun_core::handle_oom(Buffer::from_string(buffer.slice()));
if buffer.pinned {
buffer.pinned = false;
buffer.buffer.unpin();
}
*buffer = copy;
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}

impl PathLikeExt for PathLike {
// Const-generics can't change return mutability, so this always returns
// `&ZStr`. A future force=true caller that needs `&mut ZStr` will need a
Expand Down Expand Up @@ -1250,8 +1270,15 @@ impl PathLikeExt for PathLike {
}
return Err(err);
}
snapshot_resizable_path_buffer(&mut buffer, arguments.will_be_async);

arguments.protect_eat();
if buffer.owns_buffer {
// The owned snapshot no longer references the JS backing
// store, so the original value needs no GC root.
arguments.eat();
} else {
arguments.protect_eat();
}
Ok(Some(Self::Buffer(buffer)))
}

Expand All @@ -1271,8 +1298,15 @@ impl PathLikeExt for PathLike {
}
return Err(err);
}
snapshot_resizable_path_buffer(&mut buffer, arguments.will_be_async);

arguments.protect_eat();
if buffer.owns_buffer {
// The owned snapshot no longer references the JS backing
// store, so the original value needs no GC root.
arguments.eat();
} else {
arguments.protect_eat();
}
Ok(Some(Self::Buffer(buffer)))
}

Expand Down
84 changes: 84 additions & 0 deletions test/js/node/fs/fs.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4372,3 +4372,87 @@
expect(detachedDuringOptions).toBe(false);
expect(readFileSync(file, "utf8")).toBe("hello world");
});

it("async fs ops snapshot path buffers backed by resizable ArrayBuffers before a shrink can decommit them", async () => {
// Pinning the backing ArrayBuffer blocks transfer()/detach but not
// ArrayBuffer.prototype.resize: shrinking decommits the tail pages, so a
// work-pool thread reading the path bytes after the shrink would fault.
// The path bytes must instead be copied at call time (as Node does).
using dir = tempDir("fs-rab-path", {
"rab-path-fixture.js": String.raw`
import fs from "node:fs";
import path from "node:path";

const dir = process.cwd();

// A resizable-backed path arg is snapshotted at call time: shrinking
// right after the call must not affect the rename.
{
const src = path.join(dir, "real-src.txt");
fs.writeFileSync(src, "hi");
const srcBytes = Buffer.from(src);
const rab = new ArrayBuffer(64 * 1024, { maxByteLength: 64 * 1024 });
new Uint8Array(rab).set(srcBytes);
const view = new Uint8Array(rab, 0, srcBytes.length);
const renamed = fs.promises.rename(view, path.join(dir, "real-dest.txt"));
rab.resize(0);
await renamed;
if (!fs.existsSync(path.join(dir, "real-dest.txt"))) throw new Error("rename did not happen");
}

// Issues 256 renames of a missing source, then shrinks the backing
// store while they are still queued on the work pool. Every rename must
// reject with ENOENT; a resolution or any other code means the path
// bytes were not snapshotted correctly.
async function expectAllEnoent(pathArg, rab, dest) {
const all = [];
for (let i = 0; i < 256; i++) {
all.push(fs.promises.rename(pathArg, dest).then(() => "resolved", err => err.code));
}
rab.resize(0);
const codes = await Promise.all(all);
if (!codes.every(c => c === "ENOENT")) {
throw new Error("expected all ENOENT, got: " + JSON.stringify([...new Set(codes)]));
}
}

// Uint8Array view over a resizable ArrayBuffer.
{
const srcBytes = Buffer.from(path.join(dir, "missing-view"));
const rab = new ArrayBuffer(128 * 1024, { maxByteLength: 128 * 1024 });
new Uint8Array(rab).set(srcBytes);
const view = new Uint8Array(rab, 0, srcBytes.length);
await expectAllEnoent(view, rab, path.join(dir, "dest-view"));
}

// DataView over a resizable ArrayBuffer.
{
const srcBytes = Buffer.from(path.join(dir, "missing-dataview"));
const rab = new ArrayBuffer(128 * 1024, { maxByteLength: 128 * 1024 });
new Uint8Array(rab).set(srcBytes);
const view = new DataView(rab, 0, srcBytes.length);
await expectAllEnoent(view, rab, path.join(dir, "dest-dataview"));
}

// Resizable ArrayBuffer passed directly as the path.
{
const srcBytes = Buffer.from(path.join(dir, "missing-direct"));
const rab = new ArrayBuffer(srcBytes.length, { maxByteLength: srcBytes.length });
new Uint8Array(rab).set(srcBytes);
await expectAllEnoent(rab, rab, path.join(dir, "dest-direct"));
}

console.log("done");
`,
});

await using proc = Bun.spawn({
cmd: [bunExe(), "rab-path-fixture.js"],
env: bunEnv,
cwd: String(dir),
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect({ stdout, exitCode }).toEqual({ stdout: "done\n", exitCode: 0 });

Check warning on line 4457 in test/js/node/fs/fs.test.ts

View check run for this annotation

Claude / Claude Code Review

stderr is read but dropped from the failure assertion

nit: `stderr` is read on line 4456 but dropped from the assertion on line 4457, so when this crash-class test fails the diff shows only `{stdout: "", exitCode: 132}` with no crash report or fixture error message. Consider `expect(stderr).not.toMatch(/AddressSanitizer|Segmentation fault/)` before the combined assertion (or add `stderr: ""` to the `toEqual` object) so the diagnostic surfaces in the failure diff.
Comment thread
robobun marked this conversation as resolved.
Outdated
});
Loading