fs: fix recursive readdir with buffer encoding

lib/fs.js

@@ -111,6 +111,8 @@ const {
   getValidatedFd,
   getValidatedPath,
   handleErrorFromBinding,
+  isDirectoryPath,
+  join: joinPath,
   preprocessSymlinkDestination,
   Stats,
   getReadFileBuffer,
@@ -1749,32 +1751,33 @@ function mkdirSync(path, options) {
  * string results in `context.prefixes`). `result` is a `binding.readdir()`
  * result with file types, so only symbolic links and entries of unknown type
  * need a stat() to find out whether they lead to a directory.
- * @param {string} dir
- * @param {string} prefix
+ * @param {string | Buffer} dir
+ * @param {string | Buffer} prefix
  * @param {[string[], number[]]} result
  * @param {{ withFileTypes: boolean, results: (string | Dirent)[], dirs: string[], prefixes: string[] }} context
  */
 function collectRecursiveReaddirResult(dir, prefix, { 0: names, 1: types }, context) {
   const { length } = names;
   for (let i = 0; i < length; i++) {
     const name = names[i];
-    const relative = prefix === '' ? name : `${prefix}${pathModule.sep}${name}`;
+    const relative = prefix === '' ? name : joinPath(prefix, name);
+    const fullPath = joinPath(dir, name);
     let isDirectory;
     if (context.withFileTypes) {
       const dirent = getDirent(dir, name, types[i]);
       ArrayPrototypePush(context.results, dirent);
       // Follow symbolic links to directories, see https://github.com/nodejs/node/issues/52663
       isDirectory = dirent.isDirectory() ||
-        (dirent.isSymbolicLink() && binding.internalModuleStat(pathModule.join(dir, name)) === 1);
+        (dirent.isSymbolicLink() && isDirectoryPath(fullPath));
     } else {
       ArrayPrototypePush(context.results, relative);
       const type = types[i];
       isDirectory = type === UV_DIRENT_DIR ||
         ((type === UV_DIRENT_LINK || type === UV_DIRENT_UNKNOWN) &&
-         binding.internalModuleStat(pathModule.join(dir, name)) === 1);
+         isDirectoryPath(fullPath));
     }
     if (isDirectory) {
-      ArrayPrototypePush(context.dirs, pathModule.join(dir, name));
+      ArrayPrototypePush(context.dirs, fullPath);
       ArrayPrototypePush(context.prefixes, relative);
     }
   }

lib/internal/fs/promises.js

@@ -74,6 +74,8 @@ const {
   getValidatedPath,
   getReadFileBuffer,
   getReadFileBufferByteLengthName,
+  isDirectoryPath,
+  join: joinPath,
   preprocessSymlinkDestination,
   stringToFlags,
   stringToSymlinkType,
@@ -1662,7 +1664,8 @@ async function readdirRecursive(originalPath, options) {
     const { 0: path, 1: prefix, 2: { 0: names, 1: types } } = ArrayPrototypePop(queue);
     for (let i = 0; i < names.length; i++) {
       const name = names[i];
-      const relative = prefix === '' ? name : `${prefix}${pathModule.sep}${name}`;
+      const relative = prefix === '' ? name : joinPath(prefix, name);
+      const direntPath = joinPath(path, name);
       let isDirectory;
       if (withFileTypes) {
         const dirent = getDirent(path, name, types[i]);
@@ -1674,10 +1677,9 @@ async function readdirRecursive(originalPath, options) {
         const type = types[i];
         isDirectory = type === UV_DIRENT_DIR ||
           ((type === UV_DIRENT_LINK || type === UV_DIRENT_UNKNOWN) &&
-           binding.internalModuleStat(pathModule.join(path, name)) === 1);
+           isDirectoryPath(direntPath));
       }
       if (isDirectory) {
-        const direntPath = pathModule.join(path, name);
         ArrayPrototypePush(queue, [direntPath, relative, await readdirWithTypes(direntPath)]);
       }
     }

lib/internal/fs/utils.js

@@ -65,6 +65,7 @@ const {
   validateUint32,
 } = require('internal/validators');
 const pathModule = require('path');
+const binding = internalBinding('fs');
 const kType = Symbol('type');
 const kStats = Symbol('stats');
 const kPartialAtimeNs = Symbol('partialAtimeNs');
@@ -249,6 +250,14 @@ function join(path, name) {
     'path', ['string', 'Buffer'], path);
 }
 
+function isDirectoryPath(path) {
+  if (typeof path === 'string') {
+    return binding.internalModuleStat(path) === 1;
+  }
+  const stats = binding.stat(path, false, undefined, false);
+  return stats !== undefined && getStatsFromBinding(stats).isDirectory();
+}
+
 function getDirents(path, { 0: names, 1: types }, callback) {
   let i;
   if (typeof callback === 'function') {
@@ -1128,6 +1137,8 @@ module.exports = {
   getDirent,
   getDirents,
   getOptions,
+  isDirectoryPath,
+  join,
   getValidatedFd,
   getValidatedPath,
   handleErrorFromBinding,

test/parallel/test-fs-readdir-recursive-buffer.js

@@ -0,0 +1,49 @@
+'use strict';
+
+// Regression test for https://github.com/nodejs/node/issues/58892
+// `readdir`/`readdirSync` with `{ recursive: true }` throw
+// ERR_INVALID_ARG_TYPE when `encoding: 'buffer'` is used, because the
+// internal recursive walk joins path segments with `path.join()`, which
+// does not accept Buffer arguments.
+
+const common = require('../common');
+const assert = require('assert');
+const fs = require('fs');
+const path = require('path');
+const tmpdir = require('../common/tmpdir');
+
+tmpdir.refresh();
+
+const nested = path.join(tmpdir.path, 'a', 'b');
+fs.mkdirSync(nested, { recursive: true });
+fs.writeFileSync(path.join(nested, 'file.txt'), 'hello');
+
+// readdirSync
+const syncResult = fs.readdirSync(tmpdir.path, { recursive: true, encoding: 'buffer' });
+assert.ok(syncResult.every((entry) => Buffer.isBuffer(entry)));
+assert.ok(syncResult.some((entry) => entry.toString().includes('file.txt')));
+
+// readdirSync with withFileTypes
+const syncDirents = fs.readdirSync(
+  tmpdir.path,
+  { recursive: true, encoding: 'buffer', withFileTypes: true }
+);
+assert.ok(syncDirents.some((dirent) => dirent.name.toString() === 'file.txt'));
+
+// readdir (callback)
+fs.readdir(
+  tmpdir.path,
+  { recursive: true, encoding: 'buffer' },
+  common.mustSucceed((entries) => {
+    assert.ok(entries.every((entry) => Buffer.isBuffer(entry)));
+    assert.ok(entries.some((entry) => entry.toString().includes('file.txt')));
+  })
+);
+
+// fs.promises.readdir
+fs.promises
+  .readdir(tmpdir.path, { recursive: true, encoding: 'buffer' })
+  .then(common.mustCall((entries) => {
+    assert.ok(entries.every((entry) => Buffer.isBuffer(entry)));
+    assert.ok(entries.some((entry) => entry.toString().includes('file.txt')));
+  }));