From ceb8df9f09e037b4a69de9443a00e210bce4d235 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Thu, 26 Sep 2019 00:34:05 +0200 Subject: [PATCH 1/3] doc: fix mode and flags being mistaken in fs Multiple `fs` functions have a `mode` and or `flag` option. In some cases those have been mistaken and named wrongly. All of those faulty entries have been fixed. Besides that some indentation is also aligned with the rest of the document. --- doc/api/fs.md | 93 +++++++++++++++++++++++++-------------------------- 1 file changed, 46 insertions(+), 47 deletions(-) diff --git a/doc/api/fs.md b/doc/api/fs.md index a363822bbf952d..ec86064a94f6e5 100644 --- a/doc/api/fs.md +++ b/doc/api/fs.md @@ -1556,14 +1556,14 @@ Returns an object containing commonly used constants for file system operations. The specific constants currently defined are described in [FS Constants][]. -## `fs.copyFile(src, dest[, flags], callback)` +## `fs.copyFile(src, dest[, mode], callback)` * `src` {string|Buffer|URL} source filename to copy * `dest` {string|Buffer|URL} destination filename of the copy operation -* `flags` {number} modifiers for copy operation. **Default:** `0`. +* `mode` {integer} modifiers for copy operation. **Default:** `0`. * `callback` {Function} Asynchronously copies `src` to `dest`. By default, `dest` is overwritten if it @@ -1572,7 +1572,7 @@ callback function. Node.js makes no guarantees about the atomicity of the copy operation. If an error occurs after the destination file has been opened for writing, Node.js will attempt to remove the destination. -`flags` is an optional integer that specifies the behavior +`mode` is an optional integer that specifies the behavior of the copy operation. It is possible to create a mask consisting of the bitwise OR of two or more values (e.g. `fs.constants.COPYFILE_EXCL | fs.constants.COPYFILE_FICLONE`). @@ -1596,7 +1596,7 @@ fs.copyFile('source.txt', 'destination.txt', (err) => { }); ``` -If the third argument is a number, then it specifies `flags`: +If the third argument is a number, then it specifies `mode`: ```js const fs = require('fs'); @@ -1606,21 +1606,21 @@ const { COPYFILE_EXCL } = fs.constants; fs.copyFile('source.txt', 'destination.txt', COPYFILE_EXCL, callback); ``` -## `fs.copyFileSync(src, dest[, flags])` +## `fs.copyFileSync(src, dest[, mode])` * `src` {string|Buffer|URL} source filename to copy * `dest` {string|Buffer|URL} destination filename of the copy operation -* `flags` {number} modifiers for copy operation. **Default:** `0`. +* `mode` {integer} modifiers for copy operation. **Default:** `0`. Synchronously copies `src` to `dest`. By default, `dest` is overwritten if it already exists. Returns `undefined`. Node.js makes no guarantees about the atomicity of the copy operation. If an error occurs after the destination file has been opened for writing, Node.js will attempt to remove the destination. -`flags` is an optional integer that specifies the behavior +`mode` is an optional integer that specifies the behavior of the copy operation. It is possible to create a mask consisting of the bitwise OR of two or more values (e.g. `fs.constants.COPYFILE_EXCL | fs.constants.COPYFILE_FICLONE`). @@ -1642,7 +1642,7 @@ fs.copyFileSync('source.txt', 'destination.txt'); console.log('source.txt was copied to destination.txt'); ``` -If the third argument is a number, then it specifies `flags`: +If the third argument is a number, then it specifies `mode`: ```js const fs = require('fs'); @@ -1796,12 +1796,11 @@ changes: * `fs` {Object|null} **Default:** `null` * Returns: {fs.WriteStream} See [Writable Stream][]. -`options` may also include a `start` option to allow writing data at -some position past the beginning of the file, allowed values are in the -[0, [`Number.MAX_SAFE_INTEGER`][]] range. Modifying a file rather -than replacing it may require a `flags` mode of `r+` rather than the -default mode `w`. The `encoding` can be any one of those accepted by -[`Buffer`][]. +`options` may also include a `start` option to allow writing data at some +position past the beginning of the file, allowed values are in the +[0, [`Number.MAX_SAFE_INTEGER`][]] range. Modifying a file rather than replacing +it may require the `flags` option to be set to `r+` rather than the default `w`. +The `encoding` can be any one of those accepted by [`Buffer`][]. If `autoClose` is set to true (default behavior) on `'error'` or `'finish'` the file descriptor will be closed automatically. If `autoClose` is false, @@ -2466,7 +2465,7 @@ changes: Asynchronously creates a directory. No arguments other than a possible exception are given to the completion callback. -The optional `options` argument can be an integer specifying mode (permission +The optional `options` argument can be an integer specifying `mode` (permission and sticky bits), or an object with a `mode` property and a `recursive` property indicating whether parent folders should be created. Calling `fs.mkdir()` when `path` is a directory that exists results in an error only @@ -2617,7 +2616,7 @@ changes: description: The `flags` argument is now optional and defaults to `'r'`. - version: v9.9.0 pr-url: https://github.com/nodejs/node/pull/18801 - description: The `as` and `as+` modes are supported now. + description: The `as` and `as+` flags are supported now. - version: v7.6.0 pr-url: https://github.com/nodejs/node/pull/10739 description: The `path` parameter can be a WHATWG `URL` object using `file:` @@ -2709,7 +2708,7 @@ changes: description: The `flags` argument is now optional and defaults to `'r'`. - version: v9.9.0 pr-url: https://github.com/nodejs/node/pull/18801 - description: The `as` and `as+` modes are supported now. + description: The `as` and `as+` flags are supported now. - version: v7.6.0 pr-url: https://github.com/nodejs/node/pull/10739 description: The `path` parameter can be a WHATWG `URL` object using `file:` @@ -3276,16 +3275,16 @@ changes: * `path` {string|Buffer|URL} * `options` {Object} * `maxRetries` {integer} If an `EBUSY`, `EMFILE`, `ENFILE`, `ENOTEMPTY`, or - `EPERM` error is encountered, Node.js will retry the operation with a linear - backoff wait of `retryDelay` ms longer on each try. This option represents the - number of retries. This option is ignored if the `recursive` option is not - `true`. **Default:** `0`. + `EPERM` error is encountered, Node.js will retry the operation with a linear + backoff wait of `retryDelay` ms longer on each try. This option represents + the number of retries. This option is ignored if the `recursive` option is + not `true`. **Default:** `0`. * `recursive` {boolean} If `true`, perform a recursive directory removal. In - recursive mode, errors are not reported if `path` does not exist, and - operations are retried on failure. **Default:** `false`. + recursive mode, errors are not reported if `path` does not exist, and + operations are retried on failure. **Default:** `false`. * `retryDelay` {integer} The amount of time in milliseconds to wait between - retries. This option is ignored if the `recursive` option is not `true`. - **Default:** `100`. + retries. This option is ignored if the `recursive` option is not `true`. + **Default:** `100`. * `callback` {Function} * `err` {Error} @@ -3321,16 +3320,16 @@ changes: * `path` {string|Buffer|URL} * `options` {Object} * `maxRetries` {integer} If an `EBUSY`, `EMFILE`, `ENFILE`, `ENOTEMPTY`, or - `EPERM` error is encountered, Node.js will retry the operation with a linear - backoff wait of `retryDelay` ms longer on each try. This option represents the - number of retries. This option is ignored if the `recursive` option is not - `true`. **Default:** `0`. + `EPERM` error is encountered, Node.js will retry the operation with a linear + backoff wait of `retryDelay` ms longer on each try. This option represents + the number of retries. This option is ignored if the `recursive` option is + not `true`. **Default:** `0`. * `recursive` {boolean} If `true`, perform a recursive directory removal. In - recursive mode, errors are not reported if `path` does not exist, and - operations are retried on failure. **Default:** `false`. + recursive mode, errors are not reported if `path` does not exist, and + operations are retried on failure. **Default:** `false`. * `retryDelay` {integer} The amount of time in milliseconds to wait between - retries. This option is ignored if the `recursive` option is not `true`. - **Default:** `100`. + retries. This option is ignored if the `recursive` option is not `true`. + **Default:** `100`. Synchronous rmdir(2). Returns `undefined`. @@ -4687,14 +4686,14 @@ added: v10.0.0 Changes the ownership of a file then resolves the `Promise` with no arguments upon success. -### `fsPromises.copyFile(src, dest[, flags])` +### `fsPromises.copyFile(src, dest[, mode])` * `src` {string|Buffer|URL} source filename to copy * `dest` {string|Buffer|URL} destination filename of the copy operation -* `flags` {number} modifiers for copy operation. **Default:** `0`. +* `mode` {integer} modifiers for copy operation. **Default:** `0`. * Returns: {Promise} Asynchronously copies `src` to `dest`. By default, `dest` is overwritten if it @@ -4704,7 +4703,7 @@ Node.js makes no guarantees about the atomicity of the copy operation. If an error occurs after the destination file has been opened for writing, Node.js will attempt to remove the destination. -`flags` is an optional integer that specifies the behavior +`mode` is an optional integer that specifies the behavior of the copy operation. It is possible to create a mask consisting of the bitwise OR of two or more values (e.g. `fs.constants.COPYFILE_EXCL | fs.constants.COPYFILE_FICLONE`). @@ -4727,7 +4726,7 @@ fsPromises.copyFile('source.txt', 'destination.txt') .catch(() => console.log('The file could not be copied')); ``` -If the third argument is a number, then it specifies `flags`: +If the third argument is a number, then it specifies `mode`: ```js const fs = require('fs'); @@ -4813,7 +4812,7 @@ added: v10.0.0 Asynchronously creates a directory then resolves the `Promise` with no arguments upon success. -The optional `options` argument can be an integer specifying mode (permission +The optional `options` argument can be an integer specifying `mode` (permission and sticky bits), or an object with a `mode` property and a `recursive` property indicating whether parent folders should be created. Calling `fsPromises.mkdir()` when `path` is a directory that exists results in a @@ -5044,16 +5043,16 @@ changes: * `path` {string|Buffer|URL} * `options` {Object} * `maxRetries` {integer} If an `EBUSY`, `EMFILE`, `ENFILE`, `ENOTEMPTY`, or - `EPERM` error is encountered, Node.js will retry the operation with a linear - backoff wait of `retryDelay` ms longer on each try. This option represents the - number of retries. This option is ignored if the `recursive` option is not - `true`. **Default:** `0`. + `EPERM` error is encountered, Node.js will retry the operation with a linear + backoff wait of `retryDelay` ms longer on each try. This option represents + the number of retries. This option is ignored if the `recursive` option is + not `true`. **Default:** `0`. * `recursive` {boolean} If `true`, perform a recursive directory removal. In - recursive mode, errors are not reported if `path` does not exist, and - operations are retried on failure. **Default:** `false`. + recursive mode, errors are not reported if `path` does not exist, and + operations are retried on failure. **Default:** `false`. * `retryDelay` {integer} The amount of time in milliseconds to wait between - retries. This option is ignored if the `recursive` option is not `true`. - **Default:** `100`. + retries. This option is ignored if the `recursive` option is not `true`. + **Default:** `100`. * Returns: {Promise} Removes the directory identified by `path` then resolves the `Promise` with @@ -5541,7 +5540,7 @@ the file contents. [`fs.access()`]: #fs_fs_access_path_mode_callback [`fs.chmod()`]: #fs_fs_chmod_path_mode_callback [`fs.chown()`]: #fs_fs_chown_path_uid_gid_callback -[`fs.copyFile()`]: #fs_fs_copyfile_src_dest_flags_callback +[`fs.copyFile()`]: #fs_fs_copyfile_src_dest_mode_callback [`fs.createWriteStream()`]: #fs_fs_createwritestream_path_options [`fs.exists()`]: fs.html#fs_fs_exists_path_callback [`fs.fstat()`]: #fs_fs_fstat_fd_options_callback From 047fbc8f135d85a7007432797f33f2438e5f6185 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Tue, 2 Apr 2019 01:21:36 +0200 Subject: [PATCH 2/3] fs: improve mode and flags validation This fixes a few bugs in `fs`. E.g., `fs.promises.access` accepted strings as mode. It should have only accepted numbers. It will now always validate the flags and the mode argument in an consistent way. --- doc/api/fs.md | 4 +- lib/fs.js | 82 +++++++++---------- .../switches/does_own_process_state.js | 4 +- lib/internal/fs/promises.js | 21 ++--- lib/internal/fs/utils.js | 57 +++++++++++++ lib/internal/validators.js | 12 +-- test/parallel/test-fs-access.js | 49 +++++++++++ test/parallel/test-fs-copyfile.js | 37 ++++++++- test/parallel/test-fs-error-messages.js | 18 ++-- test/parallel/test-fs-open-flags.js | 5 -- .../test-fs-promises-file-handle-sync.js | 17 +++- test/parallel/test-fs-promises.js | 32 ++++++-- 12 files changed, 246 insertions(+), 92 deletions(-) diff --git a/doc/api/fs.md b/doc/api/fs.md index ec86064a94f6e5..3917001d6c07c0 100644 --- a/doc/api/fs.md +++ b/doc/api/fs.md @@ -5493,8 +5493,8 @@ On Linux, positional writes don't work when the file is opened in append mode. The kernel ignores the position argument and always appends the data to the end of the file. -Modifying a file rather than replacing it may require a flags mode of `'r+'` -rather than the default mode `'w'`. +Modifying a file rather than replacing it may require the `flag` option to be +set to `'r+'` rather than the default `'w'`. The behavior of some flags are platform-specific. As such, opening a directory on macOS and Linux with the `'a+'` flag, as in the example below, will return an diff --git a/lib/fs.js b/lib/fs.js index 3810ad2e6ca916..66113899a5bfdc 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -72,6 +72,7 @@ const { getDirents, getOptions, getValidatedPath, + getValidMode, handleErrorFromBinding, nullCheck, preprocessSymlinkDestination, @@ -99,7 +100,7 @@ const { } = require('internal/constants'); const { isUint32, - parseMode, + parseFileMode, validateBuffer, validateInteger, validateInt32, @@ -183,8 +184,7 @@ function access(path, mode, callback) { } path = getValidatedPath(path); - - mode = mode | 0; + mode = getValidMode(mode, 'access'); const req = new FSReqCallback(); req.oncomplete = makeCallback(callback); binding.access(pathModule.toNamespacedPath(path), mode, req); @@ -192,11 +192,7 @@ function access(path, mode, callback) { function accessSync(path, mode) { path = getValidatedPath(path); - - if (mode === undefined) - mode = F_OK; - else - mode = mode | 0; + mode = getValidMode(mode, 'access'); const ctx = { path }; binding.access(pathModule.toNamespacedPath(path), mode, undefined, ctx); @@ -310,8 +306,9 @@ function readFile(path, options, callback) { } path = getValidatedPath(path); + const flagsNumber = stringToFlags(options.flags); binding.open(pathModule.toNamespacedPath(path), - stringToFlags(options.flag || 'r'), + flagsNumber, 0o666, req); } @@ -428,11 +425,10 @@ function open(path, flags, mode, callback) { } else if (typeof mode === 'function') { callback = mode; mode = 0o666; + } else { + mode = parseFileMode(mode, 'mode', 0o666); } const flagsNumber = stringToFlags(flags); - if (arguments.length >= 4) { - mode = parseMode(mode, 'mode', 0o666); - } callback = makeCallback(callback); const req = new FSReqCallback(); @@ -447,8 +443,8 @@ function open(path, flags, mode, callback) { function openSync(path, flags, mode) { path = getValidatedPath(path); - const flagsNumber = stringToFlags(flags || 'r'); - mode = parseMode(mode, 'mode', 0o666); + const flagsNumber = stringToFlags(flags); + mode = parseFileMode(mode, 'mode', 0o666); const ctx = { path }; const result = binding.open(pathModule.toNamespacedPath(path), @@ -814,16 +810,18 @@ function fsyncSync(fd) { } function mkdir(path, options, callback) { + let mode = 0o777; + let recursive = false; if (typeof options === 'function') { callback = options; - options = {}; } else if (typeof options === 'number' || typeof options === 'string') { - options = { mode: options }; + mode = options; + } else if (options) { + if (options.recursive !== undefined) + recursive = options.recursive; + if (options.mode !== undefined) + mode = options.mode; } - const { - recursive = false, - mode = 0o777 - } = options || {}; callback = makeCallback(callback); path = getValidatedPath(path); @@ -833,25 +831,27 @@ function mkdir(path, options, callback) { const req = new FSReqCallback(); req.oncomplete = callback; binding.mkdir(pathModule.toNamespacedPath(path), - parseMode(mode, 'mode', 0o777), recursive, req); + parseFileMode(mode, 'mode'), recursive, req); } function mkdirSync(path, options) { + let mode = 0o777; + let recursive = false; if (typeof options === 'number' || typeof options === 'string') { - options = { mode: options }; + mode = options; + } else if (options) { + if (options.recursive !== undefined) + recursive = options.recursive; + if (options.mode !== undefined) + mode = options.mode; } - const { - recursive = false, - mode = 0o777 - } = options || {}; - path = getValidatedPath(path); if (typeof recursive !== 'boolean') throw new ERR_INVALID_ARG_TYPE('recursive', 'boolean', recursive); const ctx = { path }; binding.mkdir(pathModule.toNamespacedPath(path), - parseMode(mode, 'mode', 0o777), recursive, undefined, + parseFileMode(mode, 'mode'), recursive, undefined, ctx); handleErrorFromBinding(ctx); } @@ -1070,7 +1070,7 @@ function unlinkSync(path) { function fchmod(fd, mode, callback) { validateInt32(fd, 'fd', 0); - mode = parseMode(mode, 'mode'); + mode = parseFileMode(mode, 'mode'); callback = makeCallback(callback); const req = new FSReqCallback(); @@ -1080,7 +1080,7 @@ function fchmod(fd, mode, callback) { function fchmodSync(fd, mode) { validateInt32(fd, 'fd', 0); - mode = parseMode(mode, 'mode'); + mode = parseFileMode(mode, 'mode'); const ctx = {}; binding.fchmod(fd, mode, undefined, ctx); handleErrorFromBinding(ctx); @@ -1120,7 +1120,7 @@ function lchmodSync(path, mode) { function chmod(path, mode, callback) { path = getValidatedPath(path); - mode = parseMode(mode, 'mode'); + mode = parseFileMode(mode, 'mode'); callback = makeCallback(callback); const req = new FSReqCallback(); @@ -1130,7 +1130,7 @@ function chmod(path, mode, callback) { function chmodSync(path, mode) { path = getValidatedPath(path); - mode = parseMode(mode, 'mode'); + mode = parseFileMode(mode, 'mode'); const ctx = { path }; binding.chmod(pathModule.toNamespacedPath(path), mode, undefined, ctx); @@ -1791,10 +1791,10 @@ function mkdtempSync(prefix, options) { } -function copyFile(src, dest, flags, callback) { - if (typeof flags === 'function') { - callback = flags; - flags = 0; +function copyFile(src, dest, mode, callback) { + if (typeof mode === 'function') { + callback = mode; + mode = 0; } else if (typeof callback !== 'function') { throw new ERR_INVALID_CALLBACK(callback); } @@ -1804,14 +1804,14 @@ function copyFile(src, dest, flags, callback) { src = pathModule._makeLong(src); dest = pathModule._makeLong(dest); - flags = flags | 0; + mode = getValidMode(mode, 'copyFile'); const req = new FSReqCallback(); req.oncomplete = makeCallback(callback); - binding.copyFile(src, dest, flags, req); + binding.copyFile(src, dest, mode, req); } -function copyFileSync(src, dest, flags) { +function copyFileSync(src, dest, mode) { src = getValidatedPath(src, 'src'); dest = getValidatedPath(dest, 'dest'); @@ -1819,8 +1819,8 @@ function copyFileSync(src, dest, flags) { src = pathModule._makeLong(src); dest = pathModule._makeLong(dest); - flags = flags | 0; - binding.copyFile(src, dest, flags, undefined, ctx); + mode = getValidMode(mode, 'copyFile'); + binding.copyFile(src, dest, mode, undefined, ctx); handleErrorFromBinding(ctx); } diff --git a/lib/internal/bootstrap/switches/does_own_process_state.js b/lib/internal/bootstrap/switches/does_own_process_state.js index 15023ea3f55fed..5ee7f079d10124 100644 --- a/lib/internal/bootstrap/switches/does_own_process_state.js +++ b/lib/internal/bootstrap/switches/does_own_process_state.js @@ -23,7 +23,7 @@ if (credentials.implementsPosixCredentials) { // ---- compare the setups side-by-side ----- const { - parseMode, + parseFileMode, validateString } = require('internal/validators'); @@ -119,7 +119,7 @@ function wrappedChdir(directory) { function wrappedUmask(mask) { if (mask !== undefined) { - mask = parseMode(mask, 'mask'); + mask = parseFileMode(mask, 'mask'); } return rawMethods.umask(mask); } diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index 65e4fdb63c7ecb..5f8be113f44c0c 100644 --- a/lib/internal/fs/promises.js +++ b/lib/internal/fs/promises.js @@ -30,6 +30,7 @@ const { getOptions, getStatsFromBinding, getValidatedPath, + getValidMode, nullCheck, preprocessSymlinkDestination, stringToFlags, @@ -43,7 +44,7 @@ const { } = require('internal/fs/utils'); const { opendir } = require('internal/fs/dir'); const { - parseMode, + parseFileMode, validateBuffer, validateInteger, validateUint32 @@ -189,27 +190,27 @@ async function readFileHandle(filehandle, options) { async function access(path, mode = F_OK) { path = getValidatedPath(path); - mode = mode | 0; + mode = getValidMode(mode, 'access'); return binding.access(pathModule.toNamespacedPath(path), mode, kUsePromises); } -async function copyFile(src, dest, flags) { +async function copyFile(src, dest, mode) { src = getValidatedPath(src, 'src'); dest = getValidatedPath(dest, 'dest'); - flags = flags | 0; + mode = getValidMode(mode, 'copyFile'); return binding.copyFile(pathModule.toNamespacedPath(src), pathModule.toNamespacedPath(dest), - flags, kUsePromises); + mode, + kUsePromises); } // 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); - if (arguments.length < 2) flags = 'r'; const flagsNumber = stringToFlags(flags); - mode = parseMode(mode, 'mode', 0o666); + mode = parseFileMode(mode, 'mode', 0o666); return new FileHandle( await binding.openFileHandle(pathModule.toNamespacedPath(path), flagsNumber, mode, kUsePromises)); @@ -342,7 +343,7 @@ async function mkdir(path, options) { throw new ERR_INVALID_ARG_TYPE('recursive', 'boolean', recursive); return binding.mkdir(pathModule.toNamespacedPath(path), - parseMode(mode, 'mode', 0o777), recursive, + parseFileMode(mode, 'mode', 0o777), recursive, kUsePromises); } @@ -410,13 +411,13 @@ async function unlink(path) { async function fchmod(handle, mode) { validateFileHandle(handle); - mode = parseMode(mode, 'mode'); + mode = parseFileMode(mode, 'mode'); return binding.fchmod(handle.fd, mode, kUsePromises); } async function chmod(path, mode) { path = getValidatedPath(path); - mode = parseMode(mode, 'mode'); + mode = parseFileMode(mode, 'mode'); return binding.chmod(pathModule.toNamespacedPath(path), mode, kUsePromises); } diff --git a/lib/internal/fs/utils.js b/lib/internal/fs/utils.js index bca8c618dab60c..18d8e07d78e3cb 100644 --- a/lib/internal/fs/utils.js +++ b/lib/internal/fs/utils.js @@ -7,6 +7,7 @@ const { Error, Number, NumberIsFinite, + MathMin, ObjectSetPrototypeOf, ReflectOwnKeys, Symbol, @@ -40,8 +41,16 @@ const { const pathModule = require('path'); const kType = Symbol('type'); const kStats = Symbol('stats'); +const assert = require('internal/assert'); const { + 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, @@ -70,6 +79,26 @@ const { UV_DIRENT_BLOCK } = internalBinding('constants').fs; +// 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; + const isWindows = process.platform === 'win32'; let fs; @@ -434,6 +463,10 @@ function stringToFlags(flags) { return flags; } + if (flags == null) { + return O_RDONLY; + } + switch (flags) { case 'r' : return O_RDONLY; case 'rs' : // Fall through. @@ -594,6 +627,29 @@ const validateRmdirOptions = hideStackFrames((options) => { 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; + } + if (Number.isInteger(mode) && mode >= min && mode <= max) { + return mode; + } + if (typeof mode !== 'number') { + throw new ERR_INVALID_ARG_TYPE('mode', 'integer', mode); + } + throw new ERR_OUT_OF_RANGE( + 'mode', `an integer >= ${min} && <= ${max}`, mode); +}); module.exports = { assertEncoding, @@ -604,6 +660,7 @@ module.exports = { getDirents, getOptions, getValidatedPath, + getValidMode, handleErrorFromBinding, nullCheck, preprocessSymlinkDestination, diff --git a/lib/internal/validators.js b/lib/internal/validators.js index 079ab3d2f53c20..329410ef3d8d0a 100644 --- a/lib/internal/validators.js +++ b/lib/internal/validators.js @@ -44,7 +44,11 @@ const modeDesc = 'must be a 32-bit unsigned integer or an octal string'; * @param {number} def If specified, will be returned for invalid values * @returns {number} */ -function parseMode(value, name, def) { +function parseFileMode(value, name, def) { + if (value == null && def !== undefined) { + return def; + } + if (isUint32(value)) { return value; } @@ -60,10 +64,6 @@ function parseMode(value, name, def) { return parseInt(value, 8); } - if (def !== undefined && value == null) { - return def; - } - throw new ERR_INVALID_ARG_VALUE(name, value, modeDesc); } @@ -158,7 +158,7 @@ function validateEncoding(data, encoding) { module.exports = { isInt32, isUint32, - parseMode, + parseFileMode, validateBuffer, validateEncoding, validateInteger, diff --git a/test/parallel/test-fs-access.js b/test/parallel/test-fs-access.js index 2a431134af322e..ec35623c1400f1 100644 --- a/test/parallel/test-fs-access.js +++ b/test/parallel/test-fs-access.js @@ -158,6 +158,55 @@ fs.accessSync(__filename); const mode = fs.F_OK | fs.R_OK | fs.W_OK; fs.accessSync(readWriteFile, mode); +// Invalid modes should throw. +[ + false, + 1n, + { [Symbol.toPrimitive]() { return fs.R_OK; } }, + [1], + 'r' +].forEach((mode, i) => { + console.log(mode, i); + assert.throws( + () => fs.access(readWriteFile, mode, common.mustNotCall()), + { + code: 'ERR_INVALID_ARG_TYPE', + message: /"mode" argument.+integer/ + } + ); + assert.throws( + () => fs.accessSync(readWriteFile, mode), + { + code: 'ERR_INVALID_ARG_TYPE', + message: /"mode" argument.+integer/ + } + ); +}); + +// Out of range modes should throw +[ + -1, + 8, + Infinity, + NaN +].forEach((mode, i) => { + console.log(mode, i); + assert.throws( + () => fs.access(readWriteFile, mode, common.mustNotCall()), + { + code: 'ERR_OUT_OF_RANGE', + message: /"mode".+It must be an integer >= 0 && <= 7/ + } + ); + assert.throws( + () => fs.accessSync(readWriteFile, mode), + { + code: 'ERR_OUT_OF_RANGE', + message: /"mode".+It must be an integer >= 0 && <= 7/ + } + ); +}); + assert.throws( () => { fs.accessSync(doesNotExist); }, (err) => { diff --git a/test/parallel/test-fs-copyfile.js b/test/parallel/test-fs-copyfile.js index ab27ed66848c6b..97828528527852 100644 --- a/test/parallel/test-fs-copyfile.js +++ b/test/parallel/test-fs-copyfile.js @@ -114,28 +114,57 @@ assert.throws(() => { () => fs.copyFile(i, dest, common.mustNotCall()), { code: 'ERR_INVALID_ARG_TYPE', - name: 'TypeError' + name: 'TypeError', + message: /src/ } ); assert.throws( () => fs.copyFile(src, i, common.mustNotCall()), { code: 'ERR_INVALID_ARG_TYPE', - name: 'TypeError' + name: 'TypeError', + message: /dest/ } ); assert.throws( () => fs.copyFileSync(i, dest), { code: 'ERR_INVALID_ARG_TYPE', - name: 'TypeError' + name: 'TypeError', + message: /src/ } ); assert.throws( () => fs.copyFileSync(src, i), { code: 'ERR_INVALID_ARG_TYPE', - name: 'TypeError' + name: 'TypeError', + message: /dest/ } ); }); + +assert.throws(() => { + fs.copyFileSync(src, dest, 'r'); +}, { + code: 'ERR_INVALID_ARG_TYPE', + name: 'TypeError', + message: /mode/ +}); + +assert.throws(() => { + fs.copyFileSync(src, dest, 8); +}, { + code: 'ERR_OUT_OF_RANGE', + name: 'RangeError', + message: 'The value of "mode" is out of range. It must be an integer ' + + '>= 0 && <= 7. Received 8' +}); + +assert.throws(() => { + fs.copyFile(src, dest, 'r', common.mustNotCall()); +}, { + code: 'ERR_INVALID_ARG_TYPE', + name: 'TypeError', + message: /mode/ +}); diff --git a/test/parallel/test-fs-error-messages.js b/test/parallel/test-fs-error-messages.js index 05e4515ad40abd..dd9639d99657b6 100644 --- a/test/parallel/test-fs-error-messages.js +++ b/test/parallel/test-fs-error-messages.js @@ -663,21 +663,17 @@ if (!common.isAIX) { ); } -// Check copyFile with invalid flags. +// Check copyFile with invalid modes. { const validateError = { - // TODO: Make sure the error message always also contains the src. - message: `EINVAL: invalid argument, copyfile -> '${nonexistentFile}'`, - errno: UV_EINVAL, - code: 'EINVAL', - syscall: 'copyfile' + message: /"mode".+must be an integer >= 0 && <= 7\. Received -1/, + code: 'ERR_OUT_OF_RANGE' }; - fs.copyFile(existingFile, nonexistentFile, -1, - common.expectsError(validateError)); - - validateError.message = 'EINVAL: invalid argument, copyfile ' + - `'${existingFile}' -> '${nonexistentFile}'`; + assert.throws( + () => fs.copyFile(existingFile, nonexistentFile, -1, () => {}), + validateError + ); assert.throws( () => fs.copyFileSync(existingFile, nonexistentFile, -1), validateError diff --git a/test/parallel/test-fs-open-flags.js b/test/parallel/test-fs-open-flags.js index 016363b8028817..80e54d169d5264 100644 --- a/test/parallel/test-fs-open-flags.js +++ b/test/parallel/test-fs-open-flags.js @@ -84,11 +84,6 @@ assert.throws( { code: 'ERR_INVALID_OPT_VALUE', name: 'TypeError' } ); -assert.throws( - () => stringToFlags(null), - { code: 'ERR_INVALID_OPT_VALUE', name: 'TypeError' } -); - if (common.isLinux || common.isOSX) { const tmpdir = require('../common/tmpdir'); tmpdir.refresh(); diff --git a/test/parallel/test-fs-promises-file-handle-sync.js b/test/parallel/test-fs-promises-file-handle-sync.js index 2c0eca13a73b8c..53eb2424549f93 100644 --- a/test/parallel/test-fs-promises-file-handle-sync.js +++ b/test/parallel/test-fs-promises-file-handle-sync.js @@ -7,11 +7,22 @@ const tmpdir = require('../common/tmpdir'); const { access, copyFile, open } = require('fs').promises; const path = require('path'); -async function validateSync() { +async function validate() { tmpdir.refresh(); const dest = path.resolve(tmpdir.path, 'baz.js'); + await assert.rejects( + copyFile(fixtures.path('baz.js'), dest, 'r'), + { + code: 'ERR_INVALID_ARG_TYPE', + message: /mode.*integer.*string/ + } + ); await copyFile(fixtures.path('baz.js'), dest); - await access(dest, 'r'); + await assert.rejects( + access(dest, 'r'), + { code: 'ERR_INVALID_ARG_TYPE', message: /mode/ } + ); + await access(dest); const handle = await open(dest, 'r+'); await handle.datasync(); await handle.sync(); @@ -23,4 +34,4 @@ async function validateSync() { await handle.close(); } -validateSync(); +validate(); diff --git a/test/parallel/test-fs-promises.js b/test/parallel/test-fs-promises.js index b9d9b36ea898f3..08b86ea4ed41f0 100644 --- a/test/parallel/test-fs-promises.js +++ b/test/parallel/test-fs-promises.js @@ -47,17 +47,33 @@ assert.strictEqual( ); { - access(__filename, 'r') + access(__filename, 0) .then(common.mustCall()); - access('this file does not exist', 'r') - .then(common.mustNotCall()) - .catch(common.expectsError({ + assert.rejects( + access('this file does not exist', 0), + { code: 'ENOENT', name: 'Error', - message: - /^ENOENT: no such file or directory, access/ - })); + message: /^ENOENT: no such file or directory, access/ + } + ); + + assert.rejects( + access(__filename, 8), + { + code: 'ERR_OUT_OF_RANGE', + message: /"mode".*must be an integer >= 0 && <= 7\. Received 8$/ + } + ); + + assert.rejects( + access(__filename, { [Symbol.toPrimitive]() { return 5; } }), + { + code: 'ERR_INVALID_ARG_TYPE', + message: /"mode" argument.+integer\. Received an instance of Object$/ + } + ); } function verifyStatObject(stat) { @@ -68,7 +84,7 @@ function verifyStatObject(stat) { async function getHandle(dest) { await copyFile(fixtures.path('baz.js'), dest); - await access(dest, 'r'); + await access(dest); return open(dest, 'r+'); } From acc80ae59c6c533abbcac5bb5e4f36cb24c44c21 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Thu, 26 Sep 2019 14:13:49 +0200 Subject: [PATCH 3/3] doc: use shorter fs.copyFile examples This consolidates some examples to concentrate the reader on the important aspects and to reduce reading overhead. --- doc/api/fs.md | 37 ++++++++++++------------------------- 1 file changed, 12 insertions(+), 25 deletions(-) diff --git a/doc/api/fs.md b/doc/api/fs.md index 3917001d6c07c0..a4b7e81ecde301 100644 --- a/doc/api/fs.md +++ b/doc/api/fs.md @@ -1588,19 +1588,15 @@ then the operation will fail. ```js const fs = require('fs'); +const { COPYFILE_EXCL } = fs.constants; -// destination.txt will be created or overwritten by default. -fs.copyFile('source.txt', 'destination.txt', (err) => { +function callback(err) { if (err) throw err; console.log('source.txt was copied to destination.txt'); -}); -``` - -If the third argument is a number, then it specifies `mode`: +} -```js -const fs = require('fs'); -const { COPYFILE_EXCL } = fs.constants; +// destination.txt will be created or overwritten by default. +fs.copyFile('source.txt', 'destination.txt', callback); // By using COPYFILE_EXCL, the operation will fail if destination.txt exists. fs.copyFile('source.txt', 'destination.txt', COPYFILE_EXCL, callback); @@ -1636,17 +1632,11 @@ then the operation will fail. ```js const fs = require('fs'); +const { COPYFILE_EXCL } = fs.constants; // destination.txt will be created or overwritten by default. fs.copyFileSync('source.txt', 'destination.txt'); console.log('source.txt was copied to destination.txt'); -``` - -If the third argument is a number, then it specifies `mode`: - -```js -const fs = require('fs'); -const { COPYFILE_EXCL } = fs.constants; // By using COPYFILE_EXCL, the operation will fail if destination.txt exists. fs.copyFileSync('source.txt', 'destination.txt', COPYFILE_EXCL); @@ -4718,20 +4708,17 @@ create a copy-on-write reflink. If the platform does not support copy-on-write, then the operation will fail. ```js -const fsPromises = require('fs').promises; +const { + promises: fsPromises, + constants: { + COPYFILE_EXCL + } +} = require('fs'); // destination.txt will be created or overwritten by default. fsPromises.copyFile('source.txt', 'destination.txt') .then(() => console.log('source.txt was copied to destination.txt')) .catch(() => console.log('The file could not be copied')); -``` - -If the third argument is a number, then it specifies `mode`: - -```js -const fs = require('fs'); -const fsPromises = fs.promises; -const { COPYFILE_EXCL } = fs.constants; // By using COPYFILE_EXCL, the operation will fail if destination.txt exists. fsPromises.copyFile('source.txt', 'destination.txt', COPYFILE_EXCL)