From 7b01758ded981fc0d95aa719b221aaa4e255421b Mon Sep 17 00:00:00 2001 From: Rod Vagg Date: Sun, 3 Nov 2024 03:24:29 +1100 Subject: [PATCH] Revert "fs,win: fix bug in paths with trailing slashes" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 00b2f07f9ddeb8ffd2fb2108b0ed9ffa81ea000d. PR-URL: https://github.com/nodejs/node/pull/55527 Fixes: https://github.com/nodejs/node/issues/17801 Reviewed-By: Rafael Gonzaga Reviewed-By: Juan José Arboleda Reviewed-By: Marco Ippolito Reviewed-By: Luigi Pinca Reviewed-By: Matteo Collina Reviewed-By: Stefan Stojanovic --- lib/fs.js | 26 ++--- lib/internal/fs/promises.js | 6 +- lib/internal/fs/streams.js | 4 +- lib/internal/fs/utils.js | 47 +------- test/sequential/test-fs-path-dir.js | 174 ---------------------------- 5 files changed, 20 insertions(+), 237 deletions(-) delete mode 100644 test/sequential/test-fs-path-dir.js diff --git a/lib/fs.js b/lib/fs.js index e6ae9855a0d..9db60a4f89a 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -385,11 +385,7 @@ function readFile(path, options, callback) { const req = new FSReqCallback(); req.context = context; req.oncomplete = readFileAfterOpen; - binding.open( - getValidatedPath(path, 'path', { expectFile: true, syscall: 'read' }), - flagsNumber, - 0o666, - req); + binding.open(getValidatedPath(path), flagsNumber, 0o666, req); } function tryStatSync(fd, isUserFd) { @@ -441,9 +437,7 @@ function readFileSync(path, options) { if (options.encoding === 'utf8' || options.encoding === 'utf-8') { if (!isInt32(path)) { - path = getValidatedPath(path, - 'path', - { expectFile: true, syscall: 'read' }); + path = getValidatedPath(path); } return binding.readFileUtf8(path, stringToFlags(options.flag)); } @@ -537,7 +531,7 @@ function closeSync(fd) { * @returns {void} */ function open(path, flags, mode, callback) { - path = getValidatedPath(path, 'path', { expectFile: true, syscall: 'open' }); + path = getValidatedPath(path); if (arguments.length < 3) { callback = flags; flags = 'r'; @@ -566,7 +560,7 @@ function open(path, flags, mode, callback) { */ function openSync(path, flags, mode) { return binding.open( - getValidatedPath(path, 'path', { expectFile: true, syscall: 'open' }), + getValidatedPath(path), stringToFlags(flags), parseFileMode(mode, 'mode', 0o666), ); @@ -2345,9 +2339,7 @@ function writeFileSync(path, data, options) { // C++ fast path for string data and UTF8 encoding if (typeof data === 'string' && (options.encoding === 'utf8' || options.encoding === 'utf-8')) { if (!isInt32(path)) { - path = getValidatedPath(path, - 'path', - { expectFile: true, syscall: 'write' }); + path = getValidatedPath(path); } return binding.writeFileUtf8( @@ -2992,8 +2984,8 @@ function copyFile(src, dest, mode, callback) { mode = 0; } - src = getValidatedPath(src, 'src', { expectFile: true, syscall: 'cp' }); - dest = getValidatedPath(dest, 'dest', { expectFile: true, syscall: 'cp' }); + src = getValidatedPath(src, 'src'); + dest = getValidatedPath(dest, 'dest'); callback = makeCallback(callback); const req = new FSReqCallback(); @@ -3011,8 +3003,8 @@ function copyFile(src, dest, mode, callback) { */ function copyFileSync(src, dest, mode) { binding.copyFile( - getValidatedPath(src, 'src', { expectFile: true, syscall: 'cp' }), - getValidatedPath(dest, 'dest', { expectFile: true, syscall: 'cp' }), + getValidatedPath(src, 'src'), + getValidatedPath(dest, 'dest'), mode, ); } diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index f5a9a3a8542..76460d2957b 100644 --- a/lib/internal/fs/promises.js +++ b/lib/internal/fs/promises.js @@ -620,8 +620,8 @@ async function cp(src, dest, options) { async function copyFile(src, dest, mode) { return await PromisePrototypeThen( binding.copyFile( - getValidatedPath(src, 'src', { expectFile: true, syscall: 'cp' }), - getValidatedPath(dest, 'dest', { expectFile: true, syscall: 'cp' }), + getValidatedPath(src, 'src'), + getValidatedPath(dest, 'dest'), mode, kUsePromises, ), @@ -633,7 +633,7 @@ async function copyFile(src, dest, mode) { // Note that unlike fs.open() which uses numeric file descriptors, // fsPromises.open() uses the fs.FileHandle class. async function open(path, flags, mode) { - path = getValidatedPath(path, 'path', { expectFile: true, syscall: 'open' }); + path = getValidatedPath(path); const flagsNumber = stringToFlags(flags); mode = parseFileMode(mode, 'mode', 0o666); return new FileHandle(await PromisePrototypeThen( diff --git a/lib/internal/fs/streams.js b/lib/internal/fs/streams.js index 6e7ae9d6804..43f06d0104d 100644 --- a/lib/internal/fs/streams.js +++ b/lib/internal/fs/streams.js @@ -182,7 +182,7 @@ function ReadStream(path, options) { this.flags = options.flags === undefined ? 'r' : options.flags; this.mode = options.mode === undefined ? 0o666 : options.mode; - validatePath(this.path, 'path', { expectFile: true, syscall: 'read' }); + validatePath(this.path); } else { this.fd = getValidatedFd(importFd(this, options)); } @@ -337,7 +337,7 @@ function WriteStream(path, options) { this.flags = options.flags === undefined ? 'w' : options.flags; this.mode = options.mode === undefined ? 0o666 : options.mode; - validatePath(this.path, 'path', { expectFile: true, syscall: 'write' }); + validatePath(this.path); } else { this.fd = getValidatedFd(importFd(this, options)); } diff --git a/lib/internal/fs/utils.js b/lib/internal/fs/utils.js index 5080bbf2580..d54ab211246 100644 --- a/lib/internal/fs/utils.js +++ b/lib/internal/fs/utils.js @@ -710,38 +710,13 @@ const validateOffsetLengthWrite = hideStackFrames( }, ); -/** - * Validates the given path based on the expected type (file or directory). - * @param {string} path - The path to validate. - * @param {string} [propName='path'] - The name of the property being validated. - * @param {object} options - Additional options for validation. - * @param {boolean} [options.expectFile] - If true, expects the path to be a file. - * @param {string} [options.syscall] - The name of the syscall. - * @throws {TypeError} Throws if the path is not a string, Uint8Array, or URL without null bytes. - * @throws {Error} Throws if the path does not meet the expected type (file). - */ -const validatePath = hideStackFrames((path, propName = 'path', options) => { +const validatePath = hideStackFrames((path, propName = 'path') => { if (typeof path !== 'string' && !isUint8Array(path)) { throw new ERR_INVALID_ARG_TYPE.HideStackFramesError(propName, ['string', 'Buffer', 'URL'], path); } const pathIsString = typeof path === 'string'; const pathIsUint8Array = isUint8Array(path); - if (options?.expectFile) { - const lastCharacter = path[path.length - 1]; - if ( - lastCharacter === '/' || lastCharacter === 47 || - (isWindows && (lastCharacter === '\\' || lastCharacter === 92)) - ) { - throw new ERR_FS_EISDIR({ - code: 'ERR_FS_EISDIR', - message: 'is a directory', - path, - syscall: options.syscall, - errno: ERR_FS_EISDIR, - }); - } - } // We can only perform meaningful checks on strings and Uint8Arrays. if ((!pathIsString && !pathIsUint8Array) || @@ -757,21 +732,11 @@ const validatePath = hideStackFrames((path, propName = 'path', options) => { ); }); -/** - * Validates and returns the given file URL or path based on the expected type (file or directory). - * @param {string|URL} fileURLOrPath - The file URL or path to validate. - * @param {string} [propName='path'] - The name of the property being validated. - * @param {object} options - Additional options for validation. - * @param {boolean} [options.expectFile] - If true, expects the path to be a file. - * @param {string} [options.syscall] - The name of the syscall. - * @returns {string} The validated path. - */ -const getValidatedPath = - hideStackFrames((fileURLOrPath, propName = 'path', options) => { - const path = toPathIfFileURL(fileURLOrPath); - validatePath(path, propName, options); - return path; - }); +const getValidatedPath = hideStackFrames((fileURLOrPath, propName = 'path') => { + const path = toPathIfFileURL(fileURLOrPath); + validatePath(path, propName); + return path; +}); const getValidatedFd = hideStackFrames((fd, propName = 'fd') => { if (ObjectIs(fd, -0)) { diff --git a/test/sequential/test-fs-path-dir.js b/test/sequential/test-fs-path-dir.js deleted file mode 100644 index 0926a207574..00000000000 --- a/test/sequential/test-fs-path-dir.js +++ /dev/null @@ -1,174 +0,0 @@ -'use strict'; - -const common = require('../common'); -const tmpdir = require('../common/tmpdir'); -const assert = require('assert'); -const path = require('path'); -const fs = require('fs'); -const { pathToFileURL } = require('url'); - -tmpdir.refresh(); - -let fileCounter = 1; -const nextFile = () => path.join(tmpdir.path, `file${fileCounter++}`); - -const generateStringPath = (file, suffix = '') => file + suffix; - -const generateURLPath = (file, suffix = '') => - pathToFileURL(file + suffix); - -const generateUint8ArrayPath = (file, suffix = '') => - new Uint8Array(Buffer.from(file + suffix)); - -const generatePaths = (file, suffix = '') => [ - generateStringPath(file, suffix), - generateURLPath(file, suffix), - generateUint8ArrayPath(file, suffix), -]; - -const generateNewPaths = (suffix = '') => [ - generateStringPath(nextFile(), suffix), - generateURLPath(nextFile(), suffix), - generateUint8ArrayPath(nextFile(), suffix), -]; - -function checkSyncFn(syncFn, p, args, fail) { - const result = fail ? 'must fail' : 'must succeed'; - const failMsg = `failed testing sync ${p} ${syncFn.name} ${result}`; - if (!fail) { - try { - syncFn(p, ...args); - } catch (e) { - console.log(failMsg, e); - throw e; - } - } else { - assert.throws(() => syncFn(p, ...args), { - code: 'ERR_FS_EISDIR', - }, failMsg); - } -} - -function checkAsyncFn(asyncFn, p, args, fail) { - const result = fail ? 'must fail' : 'must succeed'; - const failMsg = `failed testing async ${p} ${asyncFn.name} ${result}`; - if (!fail) { - try { - asyncFn(p, ...args, common.mustCall()); - } catch (e) { - console.log(failMsg, e); - throw e; - } - } else { - assert.throws( - () => asyncFn(p, ...args, common.mustNotCall()), { - code: 'ERR_FS_EISDIR', - }, - failMsg - ); - } -} - -function checkPromiseFn(promiseFn, p, args, fail) { - const result = fail ? 'must fail' : 'must succeed'; - const failMsg = `failed testing promise ${p} ${promiseFn.name} ${result}`; - if (!fail) { - const r = promiseFn(p, ...args).catch((err) => { - console.log(failMsg, err); - throw err; - }); - r?.close?.(); - } else { - assert.rejects( - promiseFn(p, ...args), { - code: 'ERR_FS_EISDIR', - }, - failMsg - ).then(common.mustCall()); - } -} - -function check(asyncFn, syncFn, promiseFn, - { suffix, fail, args = [] }) { - const file = nextFile(); - fs.writeFile(file, 'data', (e) => { - assert.ifError(e); - const paths = generatePaths(file, suffix); - for (const p of paths) { - if (asyncFn) checkAsyncFn(asyncFn, p, args, fail); - if (promiseFn) checkPromiseFn(promiseFn, p, args, fail); - if (syncFn) checkSyncFn(syncFn, p, args, fail); - } - }); -} - -function checkManyToMany(asyncFn, syncFn, promiseFn, - { suffix, fail, args = [] }) { - const source = nextFile(); - fs.writeFile(source, 'data', (err) => { - assert.ifError(err); - if (asyncFn) { - generateNewPaths(suffix) - .forEach((p) => checkAsyncFn(asyncFn, source, [p, ...args], fail)); - } - if (promiseFn) { - generateNewPaths(suffix) - .forEach((p) => checkPromiseFn(promiseFn, source, [p, ...args], fail)); - } - if (syncFn) { - generateNewPaths(suffix) - .forEach((p) => checkSyncFn(syncFn, source, [p, ...args], fail)); - } - }); -} -check(fs.readFile, fs.readFileSync, fs.promises.readFile, - { suffix: '', fail: false }); -check(fs.readFile, fs.readFileSync, fs.promises.readFile, - { suffix: '/', fail: true }); -check(fs.writeFile, fs.writeFileSync, fs.promises.writeFile, - { suffix: '', fail: false, args: ['data'] }); -check(fs.writeFile, fs.writeFileSync, fs.promises.writeFile, - { suffix: '/', fail: true, args: ['data'] }); -check(fs.appendFile, fs.appendFileSync, fs.promises.appendFile, - { suffix: '', fail: false, args: ['data'] }); -check(fs.appendFile, fs.appendFileSync, fs.promises.appendFile, - { suffix: '/', fail: true, args: ['data'] }); -check(undefined, fs.createReadStream, undefined, - { suffix: '', fail: false }); -check(undefined, fs.createReadStream, undefined, - { suffix: '/', fail: true }); -check(undefined, fs.createWriteStream, undefined, - { suffix: '', fail: false }); -check(undefined, fs.createWriteStream, undefined, - { suffix: '/', fail: true }); -check(fs.truncate, fs.truncateSync, fs.promises.truncate, - { suffix: '', fail: false, args: [42] }); -check(fs.truncate, fs.truncateSync, fs.promises.truncate, - { suffix: '/', fail: true, args: [42] }); - -check(fs.open, fs.openSync, fs.promises.open, { suffix: '', fail: false }); -check(fs.open, fs.openSync, fs.promises.open, { suffix: '/', fail: true }); - -checkManyToMany(fs.copyFile, fs.copyFileSync, fs.promises.copyFile, - { suffix: '', fail: false }); - -checkManyToMany(fs.copyFile, fs.copyFileSync, fs.promises.copyFile, - { suffix: '/', fail: true }); - -if (common.isWindows) { - check(fs.readFile, fs.readFileSync, fs.promises.readFile, - { suffix: '\\', fail: true }); - check(fs.writeFile, fs.writeFileSync, fs.promises.writeFile, - { suffix: '\\', fail: true, args: ['data'] }); - check(fs.appendFile, fs.appendFileSync, fs.promises.appendFile, - { suffix: '\\', fail: true, args: ['data'] }); - check(undefined, fs.createReadStream, undefined, - { suffix: '\\', fail: true }); - check(undefined, fs.createWriteStream, undefined, - { suffix: '\\', fail: true }); - check(fs.truncate, fs.truncateSync, fs.promises.truncate, - { suffix: '\\', fail: true, args: [42] }); - check(fs.open, fs.openSync, fs.promises.open, { suffix: '\\', fail: true }); - checkManyToMany(fs.copyFile, fs.copyFileSync, fs.promises.copyFile, - { suffix: '\\', fail: true }); -}