diff --git a/src/jsc/node_path.rs b/src/jsc/node_path.rs index 3f2efd20713..6dec33d7ac9 100644 --- a/src/jsc/node_path.rs +++ b/src/jsc/node_path.rs @@ -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 @@ -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(); diff --git a/src/runtime/node/types.rs b/src/runtime/node/types.rs index a5979c4335b..b9b42cfa283 100644 --- a/src/runtime/node/types.rs +++ b/src/runtime/node/types.rs @@ -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; +} + 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 @@ -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))) } @@ -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))) } diff --git a/test/js/node/fs/fs.test.ts b/test/js/node/fs/fs.test.ts index 4e0fe760e70..ee000399e82 100644 --- a/test/js/node/fs/fs.test.ts +++ b/test/js/node/fs/fs.test.ts @@ -4372,3 +4372,87 @@ it("fs.promises.writeFile keeps a buffer path argument attached while options ar 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, stderr, exitCode }).toEqual({ stdout: "done\n", stderr: "", exitCode: 0 }); +});