fs: fix recursive readdir with buffer encoding

lib/fs.js

@@ -109,7 +109,10 @@ const {
   getValidatedFd,
   getValidatedPath,
   handleErrorFromBinding,
+  isDirectoryPath,
+  join: joinPath,
   preprocessSymlinkDestination,
+  relativeToBasePath,
   Stats,
   getReadFileBuffer,
   getReadFileBufferByteLengthName,
@@ -1732,24 +1735,24 @@ function handleDirents({ result, currentPath, context }) {
   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 fullPath = joinPath(currentPath, names[i]);
     const dirent = getDirent(currentPath, names[i], types[i]);
     ArrayPrototypePush(context.readdirResults, dirent);
 
-    if (dirent.isDirectory() || binding.internalModuleStat(fullPath) === 1) {
+    if (dirent.isDirectory() || isDirectoryPath(fullPath)) {
       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(resultPath);
+    const resultPath = joinPath(currentPath, result[i]);
+    const relativeResultPath = relativeToBasePath(context.basePath, resultPath);
+    const stat = isDirectoryPath(resultPath);
     ArrayPrototypePush(context.readdirResults, relativeResultPath);
 
-    if (stat === 1) {
+    if (stat) {
       ArrayPrototypePush(context.pathsQueue, resultPath);
     }
   }

lib/internal/fs/promises.js

@@ -68,7 +68,10 @@ const {
   getValidatedPath,
   getReadFileBuffer,
   getReadFileBufferByteLengthName,
+  isDirectoryPath,
+  join: joinPath,
   preprocessSymlinkDestination,
+  relativeToBasePath,
   stringToFlags,
   stringToSymlinkType,
   toUnixTimestamp,
@@ -1640,7 +1643,7 @@ async function readdirRecursive(originalPath, options) {
       for (const dirent of getDirents(path, readdir)) {
         ArrayPrototypePush(result, dirent);
         if (dirent.isDirectory()) {
-          const direntPath = pathModule.join(path, dirent.name);
+          const direntPath = joinPath(path, dirent.name);
           ArrayPrototypePush(queue, [
             direntPath,
             await PromisePrototypeThen(
@@ -1661,13 +1664,13 @@ async function readdirRecursive(originalPath, options) {
     while (queue.length > 0) {
       const { 0: path, 1: readdir } = ArrayPrototypePop(queue);
       for (const ent of readdir) {
-        const direntPath = pathModule.join(path, ent);
-        const stat = binding.internalModuleStat(direntPath);
+        const direntPath = joinPath(path, ent);
+        const isDir = isDirectoryPath(direntPath);
         ArrayPrototypePush(
           result,
-          pathModule.relative(originalPath, direntPath),
+          relativeToBasePath(originalPath, direntPath),
         );
-        if (stat === 1) {
+        if (isDir) {
           ArrayPrototypePush(queue, [
             direntPath,
             await PromisePrototypeThen(

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,37 @@ function join(path, name) {
     'path', ['string', 'Buffer'], path);
 }
 
+// Computes the equivalent of `path.relative(basePath, fullPath)` when
+// either argument may be a Buffer (as with `readdir(..., { recursive: true,
+// encoding: 'buffer' })`). `fullPath` is always built by repeatedly calling
+// `join()` (above) starting from `basePath`, so stripping the `basePath`
+// prefix - and the separator `join()` would have inserted - gives the same
+// result as `path.relative()` without needing its general Buffer support.
+function relativeToBasePath(basePath, fullPath) {
+  if (typeof basePath === 'string' && typeof fullPath === 'string') {
+    return pathModule.relative(basePath, fullPath);
+  }
+  const baseBuffer = isUint8Array(basePath) ? basePath : Buffer.from(basePath);
+  let offset = baseBuffer.length;
+  if (offset !== 0 && baseBuffer[offset - 1] !== bufferSep[0]) {
+    offset += bufferSep.length;
+  }
+  return fullPath.subarray(offset);
+}
+
+// `internalModuleStat` is a CommonJS-module-resolution-specific binding
+// (see lib/internal/modules/cjs/loader.js) that only accepts strings. For
+// Buffer paths, fall back to the general-purpose `stat` binding used by
+// `fs.statSync()`, which handles Buffers correctly at the native layer
+// without a lossy string round-trip.
+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 +1160,9 @@ module.exports = {
   getDirent,
   getDirents,
   getOptions,
+  isDirectoryPath,
+  join,
+  relativeToBasePath,
   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')));
+  }));