From e1cb5b45e4d15cc42c71ed9f8996175b02e9e393 Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Tue, 26 May 2026 02:30:06 +0000 Subject: [PATCH] node:fs: align recursive readdir symlink and cycle handling with Node --- src/js/node/fs.promises.ts | 4 +- src/runtime/node/node_fs.rs | 33 +++-- test/js/node/fs/fs.test.ts | 118 +++++++++++++++--- ...eaddirSync-recursive-error-leak-fixture.js | 33 +++-- .../readdirSync-recursive-error-leak.test.ts | 4 +- 5 files changed, 154 insertions(+), 38 deletions(-) diff --git a/src/js/node/fs.promises.ts b/src/js/node/fs.promises.ts index 6f0a0e2dec8a..25587809d1ff 100644 --- a/src/js/node/fs.promises.ts +++ b/src/js/node/fs.promises.ts @@ -167,7 +167,9 @@ const exports = { }, read: asyncWrap(fs.read, "read"), write: asyncWrap(fs.write, "write"), - readdir: asyncWrap(fs.readdir, "readdir"), + readdir: async function readdir(path, options) { + return fs.readdir(path, options, true); + }, readFile: async function (fileHandleOrFdOrPath, ...args) { fileHandleOrFdOrPath = fileHandleOrFdOrPath?.[kFd] ?? fileHandleOrFdOrPath; return _readFile(fileHandleOrFdOrPath, ...args); diff --git a/src/runtime/node/node_fs.rs b/src/runtime/node/node_fs.rs index 3b6ee31facdc..fe6b9bd8210c 100644 --- a/src/runtime/node/node_fs.rs +++ b/src/runtime/node/node_fs.rs @@ -3609,6 +3609,7 @@ pub mod args { pub encoding: Encoding, pub with_file_types: bool, pub recursive: bool, + pub no_symlink_descent: bool, } fs_args_path_forwarders!(Readdir; path); impl Readdir { @@ -3629,6 +3630,7 @@ pub mod args { let mut encoding = Encoding::Utf8; let mut with_file_types = false; let mut recursive = false; + let mut no_symlink_descent = false; if let Some(val) = arguments.next() { arguments.eat(); match val.js_type() { @@ -3650,11 +3652,18 @@ pub mod args { } } } + if let Some(val) = arguments.next() { + if val.is_boolean() { + arguments.eat(); + no_symlink_descent = val.to_boolean() && with_file_types; + } + } Ok(Readdir { path, encoding, with_file_types, recursive, + no_symlink_descent, }) } } @@ -6655,9 +6664,10 @@ impl NodeFS { match err.get_errno() { // These things can happen and there's nothing we can do about it. // - // This is different than what Node does, at the time of writing. - // Node doesn't gracefully handle errors like these. It fails the entire operation. - E::ENOENT | E::ENOTDIR | E::EPERM => return Ok(()), + // Node lists the entry but does not descend into it when the + // child cannot be opened for reasons like these (including + // symlink cycles), so skip it instead of failing the whole walk. + E::ENOENT | E::ENOTDIR | E::EPERM | E::ELOOP => return Ok(()), _ => {} } let joined = paths::resolve_path::join_z_buf::( @@ -6730,6 +6740,7 @@ impl NodeFS { sys::FileKind::SymLink | // we know for sure it's a directory sys::FileKind::Directory => { + if current.kind == sys::FileKind::SymLink && args.no_symlink_descent { break 'enqueue; } // if the name is too long, we can't enqueue it regardless // the operating system would just return ENAMETOOLONG // @@ -6748,7 +6759,9 @@ impl NodeFS { Ok(st) => { let real_kind = sys::kind_from_mode(st.st_mode as Mode); effective_kind = real_kind; - if matches!(real_kind, sys::FileKind::Directory | sys::FileKind::SymLink) { + if real_kind == sys::FileKind::Directory + || (real_kind == sys::FileKind::SymLink && !args.no_symlink_descent) + { async_task.enqueue(name_to_copy_z); } } @@ -6857,9 +6870,10 @@ impl NodeFS { match err.get_errno() { // These things can happen and there's nothing we can do about it. // - // This is different than what Node does, at the time of writing. - // Node doesn't gracefully handle errors like these. It fails the entire operation. - E::ENOENT | E::ENOTDIR | E::EPERM => continue, + // Node lists the entry but does not descend into it when the + // child cannot be opened for reasons like these (including + // symlink cycles), so skip it instead of failing the whole walk. + E::ENOENT | E::ENOTDIR | E::EPERM | E::ELOOP => continue, _ => { // TODO: propagate file path (removed previously because it leaked the path) return Err(err); @@ -6912,6 +6926,7 @@ impl NodeFS { sys::FileKind::SymLink | // we know for sure it's a directory sys::FileKind::Directory => { + if current.kind == sys::FileKind::SymLink && args.no_symlink_descent { break 'enqueue; } if utf8_name.len() + 1 + name_to_copy.len() > paths::MAX_PATH_BYTES { break 'enqueue; } // PORT NOTE: Zig `basename_allocator.dupeZ` — store with trailing NUL // so the next iteration can hand it to `openat` as a `&ZStr`. @@ -6928,7 +6943,9 @@ impl NodeFS { Ok(st) => { let real_kind = sys::kind_from_mode(st.st_mode as Mode); effective_kind = real_kind; - if matches!(real_kind, sys::FileKind::Directory | sys::FileKind::SymLink) { + if real_kind == sys::FileKind::Directory + || (real_kind == sys::FileKind::SymLink && !args.no_symlink_descent) + { let mut owned = Vec::with_capacity(name_to_copy.len() + 1); owned.extend_from_slice(name_to_copy); owned.push(0); diff --git a/test/js/node/fs/fs.test.ts b/test/js/node/fs/fs.test.ts index 1e839710b638..66544d614d10 100644 --- a/test/js/node/fs/fs.test.ts +++ b/test/js/node/fs/fs.test.ts @@ -1178,14 +1178,10 @@ it("readdirSync throws when given a file path with trailing slash", () => { } }); -// The error cleanup path previously called MarkedArrayBuffer.destroy() on -// structs stored by-value inside the entries ArrayList, which passed interior -// ArrayList pointers to the allocator (freeing entries.items.ptr for index 0 and -// then freeing it again in entries.deinit()). A self-referential symlink makes -// the recursive walk fail with ELOOP after entries have been collected, exercising -// that cleanup path. +// Like Node, a self-referential symlink is listed but not descended into; the +// recursive walk completes instead of failing with ELOOP. it.skipIf(isWindows)( - "readdirSync({encoding: 'buffer', recursive: true}) frees entries safely when a subdir fails to open", + "readdirSync({encoding: 'buffer', recursive: true}) lists a symlink loop without throwing", async () => { using dir = tempDir("readdir-buffer-error", { "a.txt": "a", @@ -1200,16 +1196,13 @@ it.skipIf(isWindows)( "-e", ` const fs = require("fs"); - let code; + const results = []; for (let i = 0; i < 2; i++) { - try { - fs.readdirSync(${JSON.stringify(String(dir))}, { encoding: "buffer", recursive: true }); - throw new Error("expected readdirSync to throw"); - } catch (e) { - code = e.code; - } + const entries = fs.readdirSync(${JSON.stringify(String(dir))}, { encoding: "buffer", recursive: true }); + results.push(entries.map(e => Buffer.from(e).toString("utf8")).sort().join(",")); } - console.log(code); + if (results[0] !== results[1]) throw new Error("expected identical results across calls"); + console.log(results[0]); `, ], env: bunEnv, @@ -1219,10 +1212,103 @@ it.skipIf(isWindows)( const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]); - expect({ stdout: stdout.trim(), exitCode }).toEqual({ stdout: "ELOOP", exitCode: 0 }); + expect({ stdout: stdout.trim(), exitCode }).toEqual({ stdout: "a.txt,b.txt,c.txt,loop", exitCode: 0 }); }, ); +it.skipIf(isWindows)("recursive readdir with symlinks matches Node", async () => { + using dir = tempDir("readdir-recursive-symlinks", { + "outside/secret.txt": "secret", + "outside/deep/deeper.txt": "x", + "root/file.txt": "a", + "root/realdir/inner.txt": "b", + "root/target-inside/inside.txt": "c", + }); + const root = join(String(dir), "root"); + symlinkSync(join("..", "outside"), join(root, "link-outside")); + symlinkSync("target-inside", join(root, "link-inside")); + symlinkSync("missing", join(root, "link-dangling")); + symlinkSync("link-self", join(root, "link-self")); + + const descended = [ + "file.txt", + "link-dangling", + "link-inside", + "link-inside/inside.txt", + "link-outside", + "link-outside/deep", + "link-outside/deep/deeper.txt", + "link-outside/secret.txt", + "link-self", + "realdir", + "realdir/inner.txt", + "target-inside", + "target-inside/inside.txt", + ]; + const notDescended = [ + "file.txt", + "link-dangling", + "link-inside", + "link-outside", + "link-self", + "realdir", + "realdir/inner.txt", + "target-inside", + "target-inside/inside.txt", + ]; + + const direntPaths = (dirents: Dirent[]) => + dirents.map(d => relative(root, join((d as any).parentPath ?? (d as any).path, d.name))).sort(); + + const syncNames = (readdirSync(root, { recursive: true }) as string[]).sort(); + expect(syncNames).toEqual(descended); + + const syncDirents = readdirSync(root, { recursive: true, withFileTypes: true }); + expect(direntPaths(syncDirents)).toEqual(descended); + + const linkDirents = syncDirents.filter(d => d.name.startsWith("link-")); + expect(linkDirents.length).toBe(4); + for (const d of linkDirents) { + expect(d.isSymbolicLink()).toBe(true); + expect(d.isDirectory()).toBe(false); + } + + const callbackReaddir = promisify(fs.readdir); + const callbackNames = ((await callbackReaddir(root, { recursive: true })) as string[]).sort(); + expect(callbackNames).toEqual(descended); + + const callbackDirents = (await callbackReaddir(root, { recursive: true, withFileTypes: true })) as Dirent[]; + expect(direntPaths(callbackDirents)).toEqual(descended); + + const promisesNames = ((await promises.readdir(root, { recursive: true })) as string[]).sort(); + expect(promisesNames).toEqual(descended); + + const promisesDirents = await promises.readdir(root, { recursive: true, withFileTypes: true }); + expect(direntPaths(promisesDirents)).toEqual(notDescended); + for (const d of promisesDirents.filter(d => d.name.startsWith("link-"))) { + expect(d.isSymbolicLink()).toBe(true); + expect(d.isDirectory()).toBe(false); + } +}); + +it.skipIf(isWindows)("recursive readdir does not throw on a parent-directory symlink cycle", async () => { + using dir = tempDir("readdir-recursive-cycle", { + "root/file.txt": "a", + }); + const root = join(String(dir), "root"); + symlinkSync(".", join(root, "loop")); + + const names = readdirSync(root, { recursive: true }) as string[]; + expect(names).toContain("loop"); + expect(names).toContain("file.txt"); + + const dirents = readdirSync(root, { recursive: true, withFileTypes: true }); + expect(dirents.length).toBe(names.length); + + const fromPromises = await promises.readdir(root, { recursive: true }); + expect(fromPromises).toContain("loop"); +}); + describe("readSync", () => { const firstFourBytes = new Uint32Array(new TextEncoder().encode("File").buffer)[0]; diff --git a/test/js/node/fs/readdirSync-recursive-error-leak-fixture.js b/test/js/node/fs/readdirSync-recursive-error-leak-fixture.js index 9982ed4d52d8..1668414c680f 100644 --- a/test/js/node/fs/readdirSync-recursive-error-leak-fixture.js +++ b/test/js/node/fs/readdirSync-recursive-error-leak-fixture.js @@ -3,17 +3,24 @@ // be fully released. Each Dirent owns a ref to both .name and .path; previously // only .name was dereferenced on the sync error path, leaking the .path string. // -// This fixture builds a wide, shallow tree under a long path, with a -// self-referential symlink two levels deep. The recursive walker is -// breadth-first, so every depth-1 directory is fully scanned (allocating -// distinct Dirent.path strings) before the depth-2 symlink is opened and fails -// with ELOOP. It repeats the failing readdirSync many times and asserts RSS -// growth between a warmed-up baseline and the end of the run stays bounded. +// This fixture builds a wide, shallow tree under a long path, with an unreadable +// (mode 000) directory two levels deep. The recursive walker is breadth-first, so +// every depth-1 directory is fully scanned (allocating distinct Dirent.path +// strings) before the depth-2 directory is opened and fails with EACCES. It +// repeats the failing readdirSync many times and asserts RSS growth between a +// warmed-up baseline and the end of the run stays bounded. +// +// EACCES does not fire when running as root, so the measurement is skipped there. const fs = require("fs"); const path = require("path"); const os = require("os"); +if (typeof process.getuid === "function" && process.getuid() === 0) { + console.log("RSS delta 0 MB"); + process.exit(0); +} + const seg = (ch, n = 220) => Buffer.alloc(n, ch).toString(); const base = fs.mkdtempSync(path.join(os.tmpdir(), "readdir-err-leak-")); @@ -32,11 +39,12 @@ for (let i = 0; i < 8; i++) { subdirs.push(d); } -// Self-referential symlink at depth 2. Opened last (BFS), yields ELOOP, which -// is not in the silently-skipped set (NOENT/NOTDIR/PERM) and so propagates as +// Unreadable directory at depth 2. Opened last (BFS), yields EACCES, which is +// not in the silently-skipped set (NOENT/NOTDIR/PERM/LOOP) and so propagates as // an error after all the Dirents above have been collected. -const loop = path.join(subdirs[0], "zzloop"); -fs.symlinkSync("zzloop", loop); +const denied = path.join(subdirs[0], "zzdenied"); +fs.mkdirSync(denied); +fs.chmodSync(denied, 0o000); // Sanity: confirm the error path actually fires. let threw = false; @@ -45,7 +53,7 @@ try { } catch { threw = true; } -if (!threw) throw new Error("expected readdirSync to throw (symlink loop not triggering error path)"); +if (!threw) throw new Error("expected readdirSync to throw (unreadable directory not triggering error path)"); // Warmup: saturate allocator working set / ASAN quarantine so the baseline // measurement is taken after steady state is reached. @@ -68,6 +76,9 @@ const after = process.memoryUsage.rss(); const deltaMB = Math.round((after - before) / 1024 / 1024); console.log("RSS delta", deltaMB, "MB"); +try { + fs.chmodSync(denied, 0o700); +} catch {} try { fs.rmSync(base, { recursive: true, force: true }); } catch {} diff --git a/test/js/node/fs/readdirSync-recursive-error-leak.test.ts b/test/js/node/fs/readdirSync-recursive-error-leak.test.ts index 91bf83e109f0..d4788c535825 100644 --- a/test/js/node/fs/readdirSync-recursive-error-leak.test.ts +++ b/test/js/node/fs/readdirSync-recursive-error-leak.test.ts @@ -2,8 +2,8 @@ import { expect, test } from "bun:test"; import { bunEnv, bunExe, isWindows } from "harness"; import path from "path"; -// Windows: self-referential symlinks behave differently and the recursive -// walker takes a different open path there; this leak is posix-specific. +// Windows: chmod(0o000) does not make a directory unreadable there and the +// recursive walker takes a different open path; this leak is posix-specific. test.skipIf(isWindows)( "readdirSync({recursive:true, withFileTypes:true}) error path does not leak Dirent.path", async () => {