diff --git a/benchmark/fs/bench-accessSync.js b/benchmark/fs/bench-accessSync.js index a80504620580ce..e11ab7dcf51b2b 100644 --- a/benchmark/fs/bench-accessSync.js +++ b/benchmark/fs/bench-accessSync.js @@ -9,12 +9,12 @@ const tmpfile = tmpdir.resolve(`.existing-file-${process.pid}`); fs.writeFileSync(tmpfile, 'this-is-for-a-benchmark', 'utf8'); const bench = common.createBenchmark(main, { - type: ['existing', 'non-existing', 'non-flat-existing'], + type: ['existing', 'non-existing', 'non-flat-existing', 'invalid-mode'], n: [1e5], }); function main({ n, type }) { - let path; + let path, mode; switch (type) { case 'existing': @@ -26,6 +26,10 @@ function main({ n, type }) { case 'non-existing': path = tmpdir.resolve(`.non-existing-file-${process.pid}`); break; + case 'invalid-mode': + path = __filename; + mode = -1; + break; default: new Error('Invalid type'); } @@ -33,7 +37,7 @@ function main({ n, type }) { bench.start(); for (let i = 0; i < n; i++) { try { - fs.accessSync(path); + fs.accessSync(path, mode); } catch { // do nothing } diff --git a/benchmark/fs/bench-copyFileSync.js b/benchmark/fs/bench-copyFileSync.js index af77fbaaaaa004..2bbb27d0b0c181 100644 --- a/benchmark/fs/bench-copyFileSync.js +++ b/benchmark/fs/bench-copyFileSync.js @@ -6,19 +6,23 @@ const tmpdir = require('../../test/common/tmpdir'); tmpdir.refresh(); const bench = common.createBenchmark(main, { - type: ['invalid', 'valid'], + type: ['invalid-mode', 'invalid-path', 'valid'], n: [1e4], }); function main({ n, type }) { tmpdir.refresh(); const dest = tmpdir.resolve(`copy-file-bench-${process.pid}`); - let path; + let path, mode; switch (type) { - case 'invalid': + case 'invalid-path': path = tmpdir.resolve(`.existing-file-${process.pid}`); break; + case 'invalid-mode': + path = __filename; + mode = -1; + break; case 'valid': path = __filename; break; @@ -28,7 +32,7 @@ function main({ n, type }) { bench.start(); for (let i = 0; i < n; i++) { try { - fs.copyFileSync(path, dest); + fs.copyFileSync(path, dest, mode); } catch { // do nothing } diff --git a/lib/fs.js b/lib/fs.js index d682759d8b0a14..789068b658e034 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -103,7 +103,6 @@ const { getOptions, getValidatedFd, getValidatedPath, - getValidMode, handleErrorFromBinding, possiblyTransformPath, preprocessSymlinkDestination, @@ -226,7 +225,6 @@ function access(path, mode, callback) { } path = getValidatedPath(path); - mode = getValidMode(mode, 'access'); callback = makeCallback(callback); const req = new FSReqCallback(); @@ -243,7 +241,6 @@ function access(path, mode, callback) { */ function accessSync(path, mode) { path = getValidatedPath(path); - mode = getValidMode(mode, 'access'); binding.access(pathModule.toNamespacedPath(path), mode); } @@ -2962,7 +2959,6 @@ function copyFile(src, dest, mode, callback) { src = pathModule._makeLong(src); dest = pathModule._makeLong(dest); - mode = getValidMode(mode, 'copyFile'); callback = makeCallback(callback); const req = new FSReqCallback(); @@ -2985,7 +2981,7 @@ function copyFileSync(src, dest, mode) { binding.copyFile( pathModule.toNamespacedPath(src), pathModule.toNamespacedPath(dest), - getValidMode(mode, 'copyFile'), + mode, ); } diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index a656ca411584b2..f17f38e8919285 100644 --- a/lib/internal/fs/promises.js +++ b/lib/internal/fs/promises.js @@ -57,7 +57,6 @@ const { getStatFsFromBinding, getStatsFromBinding, getValidatedPath, - getValidMode, preprocessSymlinkDestination, stringToFlags, stringToSymlinkType, @@ -559,7 +558,6 @@ async function readFileHandle(filehandle, options) { async function access(path, mode = F_OK) { path = getValidatedPath(path); - mode = getValidMode(mode, 'access'); return binding.access(pathModule.toNamespacedPath(path), mode, kUsePromises); } @@ -574,7 +572,6 @@ async function cp(src, dest, options) { async function copyFile(src, dest, mode) { src = getValidatedPath(src, 'src'); dest = getValidatedPath(dest, 'dest'); - mode = getValidMode(mode, 'copyFile'); return binding.copyFile(pathModule.toNamespacedPath(src), pathModule.toNamespacedPath(dest), mode, diff --git a/lib/internal/fs/utils.js b/lib/internal/fs/utils.js index f84133296e86fb..dba5e6a1602d65 100644 --- a/lib/internal/fs/utils.js +++ b/lib/internal/fs/utils.js @@ -10,7 +10,6 @@ const { FunctionPrototypeCall, Number, NumberIsFinite, - MathMin, MathRound, ObjectIs, ObjectSetPrototypeOf, @@ -62,17 +61,9 @@ const { const pathModule = require('path'); const kType = Symbol('type'); const kStats = Symbol('stats'); -const assert = require('internal/assert'); const { fs: { - F_OK = 0, - W_OK = 0, - R_OK = 0, - X_OK = 0, - COPYFILE_EXCL, - COPYFILE_FICLONE, - COPYFILE_FICLONE_FORCE, O_APPEND, O_CREAT, O_EXCL, @@ -107,26 +98,6 @@ const { }, } = internalBinding('constants'); -// The access modes can be any of F_OK, R_OK, W_OK or X_OK. Some might not be -// available on specific systems. They can be used in combination as well -// (F_OK | R_OK | W_OK | X_OK). -const kMinimumAccessMode = MathMin(F_OK, W_OK, R_OK, X_OK); -const kMaximumAccessMode = F_OK | W_OK | R_OK | X_OK; - -const kDefaultCopyMode = 0; -// The copy modes can be any of COPYFILE_EXCL, COPYFILE_FICLONE or -// COPYFILE_FICLONE_FORCE. They can be used in combination as well -// (COPYFILE_EXCL | COPYFILE_FICLONE | COPYFILE_FICLONE_FORCE). -const kMinimumCopyMode = MathMin( - kDefaultCopyMode, - COPYFILE_EXCL, - COPYFILE_FICLONE, - COPYFILE_FICLONE_FORCE, -); -const kMaximumCopyMode = COPYFILE_EXCL | - COPYFILE_FICLONE | - COPYFILE_FICLONE_FORCE; - // Most platforms don't allow reads or writes >= 2 GiB. // See https://github.com/libuv/libuv/pull/1501. const kIoMaxLength = 2 ** 31 - 1; @@ -795,7 +766,6 @@ const validateCpOptions = hideStackFrames((options) => { validateBoolean(options.preserveTimestamps, 'options.preserveTimestamps'); validateBoolean(options.recursive, 'options.recursive'); validateBoolean(options.verbatimSymlinks, 'options.verbatimSymlinks'); - options.mode = getValidMode(options.mode, 'copyFile'); if (options.dereference === true && options.verbatimSymlinks === true) { throw new ERR_INCOMPATIBLE_OPTION_PAIR('dereference', 'verbatimSymlinks'); } @@ -888,24 +858,6 @@ const validateRmdirOptions = hideStackFrames( return options; }); -const getValidMode = hideStackFrames((mode, type) => { - let min = kMinimumAccessMode; - let max = kMaximumAccessMode; - let def = F_OK; - if (type === 'copyFile') { - min = kMinimumCopyMode; - max = kMaximumCopyMode; - def = mode || kDefaultCopyMode; - } else { - assert(type === 'access'); - } - if (mode == null) { - return def; - } - validateInteger(mode, 'mode', min, max); - return mode; -}); - const validateStringAfterArrayBufferView = hideStackFrames((buffer, name) => { if (typeof buffer !== 'string') { throw new ERR_INVALID_ARG_TYPE( @@ -949,7 +901,6 @@ module.exports = { getOptions, getValidatedFd, getValidatedPath, - getValidMode, handleErrorFromBinding, nullCheck, possiblyTransformPath, diff --git a/src/node_file.cc b/src/node_file.cc index 9430aff5c29a37..30a7507136ef0d 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -89,6 +89,24 @@ constexpr char kPathSeparator = '/'; const char* const kPathSeparator = "\\/"; #endif +// The access modes can be any of F_OK, R_OK, W_OK or X_OK. Some might not be +// available on specific systems. They can be used in combination as well +// (F_OK | R_OK | W_OK | X_OK). +constexpr int kMaximumAccessMode = F_OK | W_OK | R_OK | X_OK; +constexpr int kMinimumAccessMode = std::min({F_OK, W_OK, R_OK, X_OK}); + +constexpr int kDefaultCopyMode = 0; +// The copy modes can be any of UV_FS_COPYFILE_EXCL, UV_FS_COPYFILE_FICLONE or +// UV_FS_COPYFILE_FICLONE_FORCE. They can be used in combination as well +// (US_FS_COPYFILE_EXCL | US_FS_COPYFILE_FICLONE | +// US_FS_COPYFILE_FICLONE_FORCE). +constexpr int kMinimumCopyMode = std::min({kDefaultCopyMode, + UV_FS_COPYFILE_EXCL, + UV_FS_COPYFILE_FICLONE, + UV_FS_COPYFILE_FICLONE_FORCE}); +constexpr int kMaximumCopyMode = + UV_FS_COPYFILE_EXCL | UV_FS_COPYFILE_FICLONE | UV_FS_COPYFILE_FICLONE_FORCE; + std::string Basename(const std::string& str, const std::string& extension) { // Remove everything leading up to and including the final path separator. std::string::size_type pos = str.find_last_of(kPathSeparator); @@ -854,6 +872,43 @@ void FromNamespacedPath(std::string* path) { #endif } +static inline int GetValidMode(Environment* env, + Local mode_v, + std::string_view type) { + if (!mode_v->IsInt32() && !mode_v->IsNullOrUndefined()) { + THROW_ERR_INVALID_ARG_TYPE(env, "mode must be int32 or null/undefined"); + return -1; + } + + int min = kMinimumAccessMode; + int max = kMaximumAccessMode; + int def = F_OK; + + if (type == "copyFile") { + min = kMinimumCopyMode; + max = kMaximumCopyMode; + def = mode_v->IsNullOrUndefined() ? kDefaultCopyMode + : mode_v.As()->Value(); + } else if (type != "access") { + THROW_ERR_INVALID_ARG_TYPE( + env, "type must be equal to \"copyFile\" or \"access\""); + return -1; + } + + if (mode_v->IsNullOrUndefined()) { + return def; + } + + const int mode = mode_v.As()->Value(); + if (mode < min || mode > max) { + THROW_ERR_OUT_OF_RANGE( + env, "mode is out of range: >= %d && <= %d", min, max); + return -1; + } + + return mode; +} + void AfterMkdirp(uv_fs_t* req) { FSReqBase* req_wrap = FSReqBase::from_req(req); FSReqAfterScope after(req_wrap, req); @@ -973,8 +1028,8 @@ void Access(const FunctionCallbackInfo& args) { const int argc = args.Length(); CHECK_GE(argc, 2); - CHECK(args[1]->IsInt32()); - int mode = args[1].As()->Value(); + int mode = GetValidMode(env, args[1], "access"); + if (mode == -1) return; BufferValue path(isolate, args[0]); CHECK_NOT_NULL(*path); @@ -2086,6 +2141,9 @@ static void CopyFile(const FunctionCallbackInfo& args) { const int argc = args.Length(); CHECK_GE(argc, 3); + const int flags = GetValidMode(env, args[2], "copyFile"); + if (flags == -1) return; + BufferValue src(isolate, args[0]); CHECK_NOT_NULL(*src); THROW_IF_INSUFFICIENT_PERMISSIONS( @@ -2096,9 +2154,6 @@ static void CopyFile(const FunctionCallbackInfo& args) { THROW_IF_INSUFFICIENT_PERMISSIONS( env, permission::PermissionScope::kFileSystemWrite, dest.ToStringView()); - CHECK(args[2]->IsInt32()); - const int flags = args[2].As()->Value(); - if (argc > 3) { // copyFile(src, dest, flags, req) FSReqBase* req_wrap_async = GetReqWrap(args, 3); FS_ASYNC_TRACE_BEGIN2(UV_FS_COPYFILE, diff --git a/test/parallel/test-fs-access.js b/test/parallel/test-fs-access.js index 5a8b85433eeefa..ee65ca16e42843 100644 --- a/test/parallel/test-fs-access.js +++ b/test/parallel/test-fs-access.js @@ -163,6 +163,8 @@ fs.accessSync(readWriteFile, mode); { [Symbol.toPrimitive]() { return fs.constants.R_OK; } }, [1], 'r', + NaN, + Infinity, ].forEach((mode, i) => { console.log(mode, i); assert.throws( @@ -183,8 +185,6 @@ fs.accessSync(readWriteFile, mode); [ -1, 8, - Infinity, - NaN, ].forEach((mode, i) => { console.log(mode, i); assert.throws( diff --git a/test/parallel/test-fs-cp.mjs b/test/parallel/test-fs-cp.mjs index 63bc813ae226c7..7646bd89d7d6d5 100644 --- a/test/parallel/test-fs-cp.mjs +++ b/test/parallel/test-fs-cp.mjs @@ -133,8 +133,10 @@ function nextdir() { // It rejects if options.mode is invalid. { + const src = './test/fixtures/copy/kitchen-sink'; + const dest = nextdir(); assert.throws( - () => cpSync('a', 'b', { mode: -1 }), + () => cpSync(src, dest, mustNotMutateObjectDeep({ recursive: true, mode: -1 })), { code: 'ERR_OUT_OF_RANGE' } ); } @@ -858,10 +860,11 @@ if (!isWindows) { // It throws if options is not object. { - assert.throws( - () => cp('a', 'b', { mode: -1 }, () => {}), - { code: 'ERR_OUT_OF_RANGE' } - ); + const src = './test/fixtures/copy/kitchen-sink'; + const dest = nextdir(); + cp(src, dest, mustNotMutateObjectDeep({ recursive: true, mode: -1 }), (err) => { + assert.strictEqual(err.code, 'ERR_OUT_OF_RANGE'); + }); } // Promises implementation of copy. @@ -943,10 +946,13 @@ if (!isWindows) { // It rejects if options.mode is invalid. { + const src = './test/fixtures/copy/kitchen-sink'; + const dest = nextdir(); await assert.rejects( - fs.promises.cp('a', 'b', { + fs.promises.cp(src, dest, mustNotMutateObjectDeep({ + recursive: true, mode: -1, - }), + })), { code: 'ERR_OUT_OF_RANGE' } ); }