node:fs: match Node's recursive readdir symlink and cycle handling

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);

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::<paths::platform::Auto>(
@@ -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);

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];
 

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 () => {