lib: fix fs.readdir recursive async

Fixes: #56006 PR-URL: #56041 Reviewed-By: Ethan Arrowood <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>
nodejs · Dec 6, 2024 · 8faf918 · 8faf918
1 parent b6005b3
commit 8faf918
Show file tree

Hide file tree

Showing 2 changed files with 119 additions and 39 deletions.
diff --git a/lib/fs.js b/lib/fs.js
@@ -1369,6 +1369,102 @@ function mkdirSync(path, options) {
   }
 }
 
+/*
+ * An recursive algorithm for reading the entire contents of the `basePath` directory.
+ * This function does not validate `basePath` as a directory. It is passed directly to
+ * `binding.readdir`.
+ * @param {string} basePath
+ * @param {{ encoding: string, withFileTypes: boolean }} options
+ * @param {(
+ *   err?: Error,
+ *   files?: string[] | Buffer[] | Dirent[]
+ * ) => any} callback
+ * @returns {void}
+*/
+function readdirRecursive(basePath, options, callback) {
+  const context = {
+    withFileTypes: Boolean(options.withFileTypes),
+    encoding: options.encoding,
+    basePath,
+    readdirResults: [],
+    pathsQueue: [basePath],
+  };
+
+  let i = 0;
+
+  function read(path) {
+    const req = new FSReqCallback();
+    req.oncomplete = (err, result) => {
+      if (err) {
+        callback(err);
+        return;
+      }
+
+      if (result === undefined) {
+        callback(null, context.readdirResults);
+        return;
+      }
+
+      processReaddirResult({
+        result,
+        currentPath: path,
+        context,
+      });
+
+      if (i < context.pathsQueue.length) {
+        read(context.pathsQueue[i++]);
+      } else {
+        callback(null, context.readdirResults);
+      }
+    };
+
+    binding.readdir(
+      path,
+      context.encoding,
+      context.withFileTypes,
+      req,
+    );
+  }
+
+  read(context.pathsQueue[i++]);
+}
+
+// Calling `readdir` with `withFileTypes=true`, the result is an array of arrays.
+// The first array is the names, and the second array is the types.
+// They are guaranteed to be the same length; hence, setting `length` to the length
+// of the first array within the result.
+const processReaddirResult = (args) => (args.context.withFileTypes ? handleDirents(args) : handleFilePaths(args));
+
+function handleDirents({ result, currentPath, context }) {
+  const { 0: names, 1: types } = result;
+  const { length } = names;
+
+  for (let i = 0; i < length; i++) {
+    // Avoid excluding symlinks, as they are not directories.
+    // Refs: https://github.com/nodejs/node/issues/52663
+    const fullPath = pathModule.join(currentPath, names[i]);
+    const dirent = getDirent(currentPath, names[i], types[i]);
+    ArrayPrototypePush(context.readdirResults, dirent);
+
+    if (dirent.isDirectory() || binding.internalModuleStat(binding, fullPath) === 1) {
+      ArrayPrototypePush(context.pathsQueue, fullPath);
+    }
+  }
+}
+
+function handleFilePaths({ result, currentPath, context }) {
+  for (let i = 0; i < result.length; i++) {
+    const resultPath = pathModule.join(currentPath, result[i]);
+    const relativeResultPath = pathModule.relative(context.basePath, resultPath);
+    const stat = binding.internalModuleStat(binding, resultPath);
+    ArrayPrototypePush(context.readdirResults, relativeResultPath);
+
+    if (stat === 1) {
+      ArrayPrototypePush(context.pathsQueue, resultPath);
+    }
+  }
+}
+
 /**
  * An iterative algorithm for reading the entire contents of the `basePath` directory.
  * This function does not validate `basePath` as a directory. It is passed directly to
@@ -1378,58 +1474,37 @@ function mkdirSync(path, options) {
  * @returns {string[] | Dirent[]}
  */
 function readdirSyncRecursive(basePath, options) {
-  const withFileTypes = Boolean(options.withFileTypes);
-  const encoding = options.encoding;
-
-  const readdirResults = [];
-  const pathsQueue = [basePath];
+  const context = {
+    withFileTypes: Boolean(options.withFileTypes),
+    encoding: options.encoding,
+    basePath,
+    readdirResults: [],
+    pathsQueue: [basePath],
+  };
 
   function read(path) {
     const readdirResult = binding.readdir(
       path,
-      encoding,
-      withFileTypes,
+      context.encoding,
+      context.withFileTypes,
     );
 
     if (readdirResult === undefined) {
       return;
     }
 
-    if (withFileTypes) {
-      // Calling `readdir` with `withFileTypes=true`, the result is an array of arrays.
-      // The first array is the names, and the second array is the types.
-      // They are guaranteed to be the same length; hence, setting `length` to the length
-      // of the first array within the result.
-      const length = readdirResult[0].length;
-      for (let i = 0; i < length; i++) {
-        // Avoid excluding symlinks, as they are not directories.
-        // Refs: https://github.com/nodejs/node/issues/52663
-        const stat = binding.internalModuleStat(binding, pathModule.join(path, readdirResult[0][i]));
-        const dirent = getDirent(path, readdirResult[0][i], readdirResult[1][i]);
-        ArrayPrototypePush(readdirResults, dirent);
-        if (dirent.isDirectory() || stat === 1) {
-          ArrayPrototypePush(pathsQueue, pathModule.join(dirent.parentPath, dirent.name));
-        }
-      }
-    } else {
-      for (let i = 0; i < readdirResult.length; i++) {
-        const resultPath = pathModule.join(path, readdirResult[i]);
-        const relativeResultPath = pathModule.relative(basePath, resultPath);
-        const stat = binding.internalModuleStat(binding, resultPath);
-        ArrayPrototypePush(readdirResults, relativeResultPath);
-        // 1 indicates directory
-        if (stat === 1) {
-          ArrayPrototypePush(pathsQueue, resultPath);
-        }
-      }
-    }
+    processReaddirResult({
+      result: readdirResult,
+      currentPath: path,
+      context,
+    });
   }
 
-  for (let i = 0; i < pathsQueue.length; i++) {
-    read(pathsQueue[i]);
+  for (let i = 0; i < context.pathsQueue.length; i++) {
+    read(context.pathsQueue[i]);
   }
 
-  return readdirResults;
+  return context.readdirResults;
 }
 
 /**
@@ -1455,7 +1530,7 @@ function readdir(path, options, callback) {
   }
 
   if (options.recursive) {
-    callback(null, readdirSyncRecursive(path, options));
+    readdirRecursive(path, options, callback);
     return;
   }
 

diff --git a/test/fixtures/permission/fs-read.js b/test/fixtures/permission/fs-read.js
@@ -289,6 +289,11 @@ const regularFile = __filename;
     permission: 'FileSystemRead',
     resource: path.toNamespacedPath(blockedFolder),
   }));
+  fs.readdir(blockedFolder, { recursive: true }, common.expectsError({
+    code: 'ERR_ACCESS_DENIED',
+    permission: 'FileSystemRead',
+    resource: path.toNamespacedPath(blockedFolder),
+  }));
   assert.throws(() => {
     fs.readdirSync(blockedFolder);
   }, common.expectsError({