fs: bubble errors, improving UX

Author: MatricalDefunkt

lib/internal/fs/glob.js

@@ -182,12 +182,14 @@ class Pattern {
   indexes;
   symlinks;
   last;
+  visited;
 
-  constructor(pattern, globStrings, indexes, symlinks) {
+  constructor(pattern, globStrings, indexes, symlinks, visited = null) {
     this.#pattern = pattern;
     this.#globStrings = globStrings;
     this.indexes = indexes;
     this.symlinks = symlinks;
+    this.visited = visited;
     this.last = pattern.length - 1;
   }
 
@@ -205,8 +207,8 @@ class Pattern {
   at(index) {
     return ArrayPrototypeAt(this.#pattern, index);
   }
-  child(indexes, symlinks = new SafeSet()) {
-    return new Pattern(this.#pattern, this.#globStrings, indexes, symlinks);
+  child(indexes, symlinks = new SafeSet(), visited = null) {
+    return new Pattern(this.#pattern, this.#globStrings, indexes, symlinks, visited ?? this.visited);
   }
   test(index, path) {
     if (index > this.#pattern.length) {
@@ -432,12 +434,24 @@ class Glob {
       this.#cache.addToStatCache(join(fullpath, entry.name), entry);
 
       let isDirectory = entry.isDirectory();
+      let symlinkStat = null;
       if (entry.isSymbolicLink() && this.#followSymlinks) {
-        try {
-          const stat = fsStatSync(join(fullpath, entry.name));
-          isDirectory = stat.isDirectory();
-        } catch {
-          // ignore
+        const stat = fsStatSync(join(fullpath, entry.name));
+        isDirectory = stat.isDirectory();
+        if (isDirectory) {
+          // Check for cycles by looking at visited inodes
+          let node = pattern.visited;
+          while (node) {
+            if (node.ino === stat.ino && node.dev === stat.dev) {
+              // Cycle detected, skip this symlink
+              isDirectory = false;
+              break;
+            }
+            node = node.parent;
+          }
+          if (isDirectory) {
+            symlinkStat = stat;
+          }
         }
       }
 
@@ -548,7 +562,12 @@ class Glob {
       }
       if (subPatterns.size > 0) {
         // If there are potential patterns, add to queue
-        this.#addSubpattern(entryPath, pattern.child(subPatterns, nSymlinks));
+        let newVisited = pattern.visited;
+        if (symlinkStat) {
+          // Add this symlink target to the visited chain
+          newVisited = { __proto__: null, ino: symlinkStat.ino, dev: symlinkStat.dev, parent: pattern.visited };
+        }
+        this.#addSubpattern(entryPath, pattern.child(subPatterns, nSymlinks, newVisited));
       }
     }
   }
@@ -652,12 +671,24 @@ class Glob {
       this.#cache.addToStatCache(join(fullpath, entry.name), entry);
 
       let isDirectory = entry.isDirectory();
+      let symlinkStat = null;
       if (entry.isSymbolicLink() && this.#followSymlinks) {
-        try {
-          const s = await fsStat(join(fullpath, entry.name));
-          isDirectory = s.isDirectory();
-        } catch {
-          // ignore
+        const s = await fsStat(join(fullpath, entry.name));
+        isDirectory = s.isDirectory();
+        if (isDirectory) {
+          // Check for cycles by looking at visited inodes
+          let node = pattern.visited;
+          while (node) {
+            if (node.ino === s.ino && node.dev === s.dev) {
+              // Cycle detected, skip this symlink
+              isDirectory = false;
+              break;
+            }
+            node = node.parent;
+          }
+          if (isDirectory) {
+            symlinkStat = s;
+          }
         }
       }
 
@@ -788,7 +819,11 @@ class Glob {
       }
       if (subPatterns.size > 0) {
         // If there are potential patterns, add to queue
-        this.#addSubpattern(entryPath, pattern.child(subPatterns, nSymlinks));
+        let newVisited = pattern.visited;
+        if (symlinkStat) {
+          newVisited = { __proto__: null, ino: symlinkStat.ino, dev: symlinkStat.dev, parent: pattern.visited };
+        }
+        this.#addSubpattern(entryPath, pattern.child(subPatterns, nSymlinks, newVisited));
       }
     }
   }

test-stat-identity.mjs

@@ -0,0 +1,34 @@
+// test-stat-identity.mjs
+import { statSync, rmSync } from 'fs';
+import { mkdirSync, symlinkSync } from 'fs';
+import { join, resolve } from 'path';
+
+const tmp = resolve('tmp_stat_test');
+try {
+  rmSync(tmp, { recursive: true, force: true });
+} catch {}
+try {
+  mkdirSync(tmp);
+} catch {}
+
+const dir = join(tmp, 'real_dir');
+try {
+  mkdirSync(dir);
+} catch {}
+
+const link1 = join(tmp, 'link1');
+// Link points to 'real_dir' which is in the same folder as 'link1'
+try {
+  symlinkSync('real_dir', link1);
+} catch {}
+
+try {
+  const s1 = statSync(dir);
+  const s2 = statSync(link1);
+
+  console.log(`Real: dev=${s1.dev}, ino=${s1.ino}`);
+  console.log(`Link: dev=${s2.dev}, ino=${s2.ino}`);
+  console.log(`Match? ${s1.dev === s2.dev && s1.ino === s2.ino}`);
+} catch (e) {
+  console.error(e);
+}

test/parallel/test-fs-glob.mjs

@@ -2,7 +2,7 @@ import * as common from '../common/index.mjs';
 import tmpdir from '../common/tmpdir.js';
 import { resolve, dirname, sep, relative, join, isAbsolute } from 'node:path';
 import { mkdir, writeFile, symlink, glob as asyncGlob } from 'node:fs/promises';
-import { glob, globSync, Dirent, chmodSync } from 'node:fs';
+import { glob, globSync, Dirent, chmodSync, mkdirSync, writeFileSync, symlinkSync } from 'node:fs';
 import { test, describe } from 'node:test';
 import { pathToFileURL } from 'node:url';
 import { promisify } from 'node:util';
@@ -544,21 +544,66 @@ describe('glob - with restricted directory', function() {
   });
 });
 
-describe('glob follow', () => {
-  test('should return matched files in symlinked directory when follow is true', async () => {
+describe('glob followSymlinks', () => {
+  test('should not throw ELOOP with followSymlinks on symlink loop (async)', async () => {
     if (common.isWindows) return;
-    const relFilesPromise = asyncGlob('**', { cwd: fixtureDir, followSymlinks: true });
-    let count = 0;
-    // eslint-disable-next-line no-unused-vars
-    for await (const file of relFilesPromise) {
-      count++;
+    const files = [];
+    for await (const file of asyncGlob('**', { cwd: fixtureDir, followSymlinks: true })) {
+      files.push(file);
     }
-    assert.ok(count > 0);
+    // Should have results but no infinite loop
+    assert.ok(files.length > 0);
+    assert.ok(files.some((f) => f === 'a/symlink/a/b/c'));
+    assert.ok(files.some((f) => f.startsWith('a/symlink/a/b/c/a/')));
+    const depth = Math.max(...files.filter((f) => f.startsWith('a/symlink/')).map((f) => f.split('/').length));
+    assert.ok(depth < 20);
   });
 
-  test('should return matched files in symlinked directory when follow is true (sync)', () => {
+  test('should not throw ELOOP with followSymlinks on symlink loop (sync)', () => {
     if (common.isWindows) return;
-    const relFiles = globSync('**', { cwd: fixtureDir, followSymlinks: true });
-    assert.ok(relFiles.length > 0);
+    const files = globSync('**', { cwd: fixtureDir, followSymlinks: true });
+    assert.ok(files.length > 0);
+    assert.ok(files.some((f) => f === 'a/symlink/a/b/c'));
+    assert.ok(files.some((f) => f.startsWith('a/symlink/a/b/c/a/')));
+    const depth = Math.max(...files.filter((f) => f.startsWith('a/symlink/')).map((f) => f.split('/').length));
+    assert.ok(depth < 20);
+  });
+
+  test('should handle symlinks without cycles (async)', async () => {
+    if (common.isWindows) return;
+    const deepLinkDir = tmpdir.resolve('deep-links');
+    await mkdir(deepLinkDir, { recursive: true });
+
+    await mkdir(join(deepLinkDir, 'level1'), { recursive: true });
+    await mkdir(join(deepLinkDir, 'level1', 'level2'), { recursive: true });
+    await writeFile(join(deepLinkDir, 'level1', 'level2', 'file.txt'), 'deep');
+    await symlink('level1/level2', join(deepLinkDir, 'link-to-level2'));
+
+    const files = [];
+    for await (const file of asyncGlob('**/*.txt', { cwd: deepLinkDir, followSymlinks: true })) {
+      files.push(file);
+    }
+
+    assert.ok(files.some((f) => f.includes('level1/level2/file.txt')));
+    assert.ok(files.some((f) => f.includes('link-to-level2/file.txt')));
+  });
+
+  test('should handle symlinks without cycles (sync)', () => {
+    if (common.isWindows) return;
+    const deepLinkDir = tmpdir.resolve('deep-links-sync');
+    try {
+      mkdirSync(deepLinkDir, { recursive: true });
+      mkdirSync(join(deepLinkDir, 'level1'), { recursive: true });
+      mkdirSync(join(deepLinkDir, 'level1', 'level2'), { recursive: true });
+      writeFileSync(join(deepLinkDir, 'level1', 'level2', 'file.txt'), 'deep');
+      symlinkSync('level1/level2', join(deepLinkDir, 'link-to-level2'));
+
+      const files = globSync('**/*.txt', { cwd: deepLinkDir, followSymlinks: true });
+
+      assert.ok(files.some((f) => f.includes('level1/level2/file.txt')));
+      assert.ok(files.some((f) => f.includes('link-to-level2/file.txt')));
+    } catch {
+      // Cleanup errors are ok
+    }
   });
 });