From ed1a29ad1e4c657e6adbe3f9bde275e14928e6c0 Mon Sep 17 00:00:00 2001 From: Can Date: Sun, 2 Aug 2026 13:00:57 +0300 Subject: [PATCH] fs: fix recursive readdir with buffer encoding `fs.readdir()`, `fs.readdirSync()`, and `fs.promises.readdir()` threw ERR_INVALID_ARG_TYPE (or crashed the process outright in the callback case) when called with both `{ recursive: true }` and `{ encoding: 'buffer' }`, because the internal recursive walk used `path.join()`, `path.relative()`, and a CommonJS-module-resolution specific native stat binding, none of which accept Buffer arguments. Adds `relativeToBasePath()` and `isDirectoryPath()` helpers to `internal/fs/utils`, alongside the existing (but previously unexported) `join()` helper, all of which handle both string and Buffer paths. The recursive readdir implementations in `lib/fs.js` and `lib/internal/fs/promises.js` now use these instead of calling `path`/the module-resolution stat binding directly. `isDirectoryPath()` falls back to the general-purpose `stat` binding (the same one `fs.statSync()` uses) for Buffer paths, so it also handles non-UTF8 file names correctly instead of round-tripping through a lossy string conversion. Fixes: https://github.com/nodejs/node/issues/58892 Signed-off-by: Can --- lib/fs.js | 15 +++--- lib/internal/fs/promises.js | 13 +++-- lib/internal/fs/utils.js | 35 +++++++++++++ .../test-fs-readdir-recursive-buffer.js | 49 +++++++++++++++++++ 4 files changed, 101 insertions(+), 11 deletions(-) create mode 100644 test/parallel/test-fs-readdir-recursive-buffer.js diff --git a/lib/fs.js b/lib/fs.js index 4fbdaf813018..ec2c261d8de1 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -109,7 +109,10 @@ const { getValidatedFd, getValidatedPath, handleErrorFromBinding, + isDirectoryPath, + join: joinPath, preprocessSymlinkDestination, + relativeToBasePath, Stats, getReadFileBuffer, getReadFileBufferByteLengthName, @@ -1732,11 +1735,11 @@ 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); } } @@ -1744,12 +1747,12 @@ function handleDirents({ result, currentPath, context }) { 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); } } diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index 3e336024a15a..cd391c425d99 100644 --- a/lib/internal/fs/promises.js +++ b/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( diff --git a/lib/internal/fs/utils.js b/lib/internal/fs/utils.js index 70aaa7e7c58e..6b8d872d8b76 100644 --- a/lib/internal/fs/utils.js +++ b/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, diff --git a/test/parallel/test-fs-readdir-recursive-buffer.js b/test/parallel/test-fs-readdir-recursive-buffer.js new file mode 100644 index 000000000000..734bf8f12fc2 --- /dev/null +++ b/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'))); + }));