From 2fe88d2218bcfbcb19a237d322069ce72687c414 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Wed, 9 May 2018 22:44:44 +0800 Subject: [PATCH] lib: mask mode_t type of arguments with 0o777 - Introduce the `validateAndMaskMode` validator that validates `mode_t` arguments and mask them with 0o777 if they are 32-bit unsigned integer or octal string to be more consistent with POSIX APIs. - Use the validator in fs APIs and process.umask for consistency. - Add tests for 32-bit unsigned modes larger than 0o777. PR-URL: https://github.com/nodejs/node/pull/20636 Fixes: https://github.com/nodejs/node/issues/20498 Reviewed-By: James M Snell Reviewed-By: Anatoli Papirovski Backport-PR-URL: https://github.com/nodejs/node/pull/21172 --- lib/fs.js | 63 ++++++++-------- lib/internal/fs/promises.js | 19 ++--- lib/internal/fs/utils.js | 16 ---- lib/internal/validators.js | 36 +++++++++ test/parallel/test-fs-chmod-mask.js | 95 ++++++++++++++++++++++++ test/parallel/test-fs-chmod.js | 8 +- test/parallel/test-fs-fchmod.js | 38 +++------- test/parallel/test-fs-mkdir-mode-mask.js | 45 +++++++++++ test/parallel/test-fs-open-mode-mask.js | 48 ++++++++++++ test/parallel/test-fs-promises.js | 26 +------ test/parallel/test-process-umask-mask.js | 29 ++++++++ test/parallel/test-process-umask.js | 6 +- 12 files changed, 313 insertions(+), 116 deletions(-) create mode 100644 test/parallel/test-fs-chmod-mask.js create mode 100644 test/parallel/test-fs-mkdir-mode-mask.js create mode 100644 test/parallel/test-fs-open-mode-mask.js create mode 100644 test/parallel/test-process-umask-mask.js diff --git a/lib/fs.js b/lib/fs.js index 7168b7875498c5..2e4d8499ef62e3 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -65,7 +65,6 @@ const internalUtil = require('internal/util'); const { copyObject, getOptions, - modeNum, nullCheck, preprocessSymlinkDestination, Stats, @@ -85,6 +84,7 @@ const { } = require('internal/constants'); const { isUint32, + validateAndMaskMode, validateInteger, validateUint32 } = require('internal/validators'); @@ -549,32 +549,36 @@ fs.closeSync = function(fd) { handleErrorFromBinding(ctx); }; -fs.open = function(path, flags, mode, callback_) { - var callback = makeCallback(arguments[arguments.length - 1]); - mode = modeNum(mode, 0o666); - +fs.open = function(path, flags, mode, callback) { path = getPathFromURL(path); validatePath(path); - validateUint32(mode, 'mode'); + const flagsNumber = stringToFlags(flags); + if (arguments.length < 4) { + callback = makeCallback(mode); + mode = 0o666; + } else { + mode = validateAndMaskMode(mode, 'mode', 0o666); + callback = makeCallback(callback); + } const req = new FSReqWrap(); req.oncomplete = callback; binding.open(pathModule.toNamespacedPath(path), - stringToFlags(flags), + flagsNumber, mode, req); }; fs.openSync = function(path, flags, mode) { - mode = modeNum(mode, 0o666); path = getPathFromURL(path); validatePath(path); - validateUint32(mode, 'mode'); + const flagsNumber = stringToFlags(flags); + mode = validateAndMaskMode(mode, 'mode', 0o666); const ctx = { path }; const result = binding.open(pathModule.toNamespacedPath(path), - stringToFlags(flags), mode, + flagsNumber, mode, undefined, ctx); handleErrorFromBinding(ctx); return result; @@ -849,12 +853,16 @@ fs.fsyncSync = function(fd) { }; fs.mkdir = function(path, mode, callback) { - if (typeof mode === 'function') callback = mode; - callback = makeCallback(callback); path = getPathFromURL(path); validatePath(path); - mode = modeNum(mode, 0o777); - validateUint32(mode, 'mode'); + + if (arguments.length < 3) { + callback = makeCallback(mode); + mode = 0o777; + } else { + callback = makeCallback(callback); + mode = validateAndMaskMode(mode, 'mode', 0o777); + } const req = new FSReqWrap(); req.oncomplete = callback; @@ -864,8 +872,7 @@ fs.mkdir = function(path, mode, callback) { fs.mkdirSync = function(path, mode) { path = getPathFromURL(path); validatePath(path); - mode = modeNum(mode, 0o777); - validateUint32(mode, 'mode'); + mode = validateAndMaskMode(mode, 'mode', 0o777); const ctx = { path }; binding.mkdir(pathModule.toNamespacedPath(path), mode, undefined, ctx); handleErrorFromBinding(ctx); @@ -1047,25 +1054,18 @@ fs.unlinkSync = function(path) { }; fs.fchmod = function(fd, mode, callback) { - mode = modeNum(mode); validateUint32(fd, 'fd'); - validateUint32(mode, 'mode'); - // Values for mode < 0 are already checked via the validateUint32 function - if (mode > 0o777) - throw new ERR_OUT_OF_RANGE('mode', undefined, mode); + mode = validateAndMaskMode(mode, 'mode'); + callback = makeCallback(callback); const req = new FSReqWrap(); - req.oncomplete = makeCallback(callback); + req.oncomplete = callback; binding.fchmod(fd, mode, req); }; fs.fchmodSync = function(fd, mode) { - mode = modeNum(mode); validateUint32(fd, 'fd'); - validateUint32(mode, 'mode'); - // Values for mode < 0 are already checked via the validateUint32 function - if (mode > 0o777) - throw new ERR_OUT_OF_RANGE('mode', undefined, mode); + mode = validateAndMaskMode(mode, 'mode'); const ctx = {}; binding.fchmod(fd, mode, undefined, ctx); handleErrorFromBinding(ctx); @@ -1106,11 +1106,10 @@ if (O_SYMLINK !== undefined) { fs.chmod = function(path, mode, callback) { - callback = makeCallback(callback); path = getPathFromURL(path); validatePath(path); - mode = modeNum(mode); - validateUint32(mode, 'mode'); + mode = validateAndMaskMode(mode, 'mode'); + callback = makeCallback(callback); const req = new FSReqWrap(); req.oncomplete = callback; @@ -1120,8 +1119,8 @@ fs.chmod = function(path, mode, callback) { fs.chmodSync = function(path, mode) { path = getPathFromURL(path); validatePath(path); - mode = modeNum(mode); - validateUint32(mode, 'mode'); + mode = validateAndMaskMode(mode, 'mode'); + const ctx = { path }; binding.chmod(pathModule.toNamespacedPath(path), mode, undefined, ctx); handleErrorFromBinding(ctx); diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index 3646a4121c7b3a..382407e89c4be8 100644 --- a/lib/internal/fs/promises.js +++ b/lib/internal/fs/promises.js @@ -12,8 +12,7 @@ const { Buffer, kMaxLength } = require('buffer'); const { ERR_FS_FILE_TOO_LARGE, ERR_INVALID_ARG_TYPE, - ERR_METHOD_NOT_IMPLEMENTED, - ERR_OUT_OF_RANGE + ERR_METHOD_NOT_IMPLEMENTED } = require('internal/errors').codes; const { getPathFromURL } = require('internal/url'); const { isUint8Array } = require('internal/util/types'); @@ -21,7 +20,6 @@ const { copyObject, getOptions, getStatsFromBinding, - modeNum, nullCheck, preprocessSymlinkDestination, stringToFlags, @@ -33,6 +31,7 @@ const { validatePath } = require('internal/fs/utils'); const { + validateAndMaskMode, validateInteger, validateUint32 } = require('internal/validators'); @@ -190,10 +189,9 @@ async function copyFile(src, dest, flags) { // Note that unlike fs.open() which uses numeric file descriptors, // fsPromises.open() uses the fs.FileHandle class. async function open(path, flags, mode) { - mode = modeNum(mode, 0o666); path = getPathFromURL(path); validatePath(path); - validateUint32(mode, 'mode'); + mode = validateAndMaskMode(mode, 'mode', 0o666); return new FileHandle( await binding.openFileHandle(pathModule.toNamespacedPath(path), stringToFlags(flags), @@ -286,10 +284,9 @@ async function fsync(handle) { } async function mkdir(path, mode) { - mode = modeNum(mode, 0o777); path = getPathFromURL(path); validatePath(path); - validateUint32(mode, 'mode'); + mode = validateAndMaskMode(mode, 'mode', 0o777); return binding.mkdir(pathModule.toNamespacedPath(path), mode, kUsePromises); } @@ -360,19 +357,15 @@ async function unlink(path) { } async function fchmod(handle, mode) { - mode = modeNum(mode); validateFileHandle(handle); - validateUint32(mode, 'mode'); - if (mode > 0o777) - throw new ERR_OUT_OF_RANGE('mode', undefined, mode); + mode = validateAndMaskMode(mode, 'mode'); return binding.fchmod(handle.fd, mode, kUsePromises); } async function chmod(path, mode) { path = getPathFromURL(path); validatePath(path); - mode = modeNum(mode); - validateUint32(mode, 'mode'); + mode = validateAndMaskMode(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 2f7a8d8ced176e..46b3a97f741572 100644 --- a/lib/internal/fs/utils.js +++ b/lib/internal/fs/utils.js @@ -70,21 +70,6 @@ function getOptions(options, defaultOptions) { return options; } -function modeNum(m, def) { - if (typeof m === 'number') - return m; - if (typeof m === 'string') { - const parsed = parseInt(m, 8); - if (Number.isNaN(parsed)) - return m; - return parsed; - } - // TODO(BridgeAR): Only return `def` in case `m == null` - if (def !== undefined) - return def; - return m; -} - // Check if the path contains null types if it is a string nor Uint8Array, // otherwise return silently. function nullCheck(path, propName, throwError = true) { @@ -391,7 +376,6 @@ module.exports = { assertEncoding, copyObject, getOptions, - modeNum, nullCheck, preprocessSymlinkDestination, realpathCacheKey: Symbol('realpathCacheKey'), diff --git a/lib/internal/validators.js b/lib/internal/validators.js index aabe71ef33979a..991af52fee5b95 100644 --- a/lib/internal/validators.js +++ b/lib/internal/validators.js @@ -2,6 +2,7 @@ const { ERR_INVALID_ARG_TYPE, + ERR_INVALID_ARG_VALUE, ERR_OUT_OF_RANGE } = require('internal/errors').codes; @@ -13,6 +14,40 @@ function isUint32(value) { return value === (value >>> 0); } +const octalReg = /^[0-7]+$/; +const modeDesc = 'must be a 32-bit unsigned integer or an octal string'; +// Validator for mode_t (the S_* constants). Valid numbers or octal strings +// will be masked with 0o777 to be consistent with the behavior in POSIX APIs. +function validateAndMaskMode(value, name, def) { + if (isUint32(value)) { + return value & 0o777; + } + + if (typeof value === 'number') { + if (!Number.isInteger(value)) { + throw new ERR_OUT_OF_RANGE(name, 'an integer', value); + } else { + // 2 ** 32 === 4294967296 + throw new ERR_OUT_OF_RANGE(name, '>= 0 && < 4294967296', value); + } + } + + if (typeof value === 'string') { + if (!octalReg.test(value)) { + throw new ERR_INVALID_ARG_VALUE(name, value, modeDesc); + } + const parsed = parseInt(value, 8); + return parsed & 0o777; + } + + // TODO(BridgeAR): Only return `def` in case `value == null` + if (def !== undefined) { + return def; + } + + throw new ERR_INVALID_ARG_VALUE(name, value, modeDesc); +} + function validateInteger(value, name) { let err; @@ -67,6 +102,7 @@ function validateUint32(value, name, positive) { module.exports = { isInt32, isUint32, + validateAndMaskMode, validateInteger, validateInt32, validateUint32 diff --git a/test/parallel/test-fs-chmod-mask.js b/test/parallel/test-fs-chmod-mask.js new file mode 100644 index 00000000000000..5f3a8d5ab82864 --- /dev/null +++ b/test/parallel/test-fs-chmod-mask.js @@ -0,0 +1,95 @@ +'use strict'; + +// This tests that mode > 0o777 will be masked off with 0o777 in fs APIs. + +const common = require('../common'); +const assert = require('assert'); +const path = require('path'); +const fs = require('fs'); + +let mode; +// On Windows chmod is only able to manipulate write permission +if (common.isWindows) { + mode = 0o444; // read-only +} else { + mode = 0o777; +} + +const maskToIgnore = 0o10000; + +const tmpdir = require('../common/tmpdir'); +tmpdir.refresh(); + +function test(mode, asString) { + const suffix = asString ? 'str' : 'num'; + const input = asString ? + (mode | maskToIgnore).toString(8) : (mode | maskToIgnore); + + { + const file = path.join(tmpdir.path, `chmod-async-${suffix}.txt`); + fs.writeFileSync(file, 'test', 'utf-8'); + + fs.chmod(file, input, common.mustCall((err) => { + assert.ifError(err); + assert.strictEqual(fs.statSync(file).mode & 0o777, mode); + })); + } + + { + const file = path.join(tmpdir.path, `chmodSync-${suffix}.txt`); + fs.writeFileSync(file, 'test', 'utf-8'); + + fs.chmodSync(file, input); + assert.strictEqual(fs.statSync(file).mode & 0o777, mode); + } + + { + const file = path.join(tmpdir.path, `fchmod-async-${suffix}.txt`); + fs.writeFileSync(file, 'test', 'utf-8'); + fs.open(file, 'w', common.mustCall((err, fd) => { + assert.ifError(err); + + fs.fchmod(fd, input, common.mustCall((err) => { + assert.ifError(err); + assert.strictEqual(fs.fstatSync(fd).mode & 0o777, mode); + fs.close(fd, assert.ifError); + })); + })); + } + + { + const file = path.join(tmpdir.path, `fchmodSync-${suffix}.txt`); + fs.writeFileSync(file, 'test', 'utf-8'); + const fd = fs.openSync(file, 'w'); + + fs.fchmodSync(fd, input); + assert.strictEqual(fs.fstatSync(fd).mode & 0o777, mode); + + fs.close(fd, assert.ifError); + } + + if (fs.lchmod) { + const link = path.join(tmpdir.path, `lchmod-src-${suffix}`); + const file = path.join(tmpdir.path, `lchmod-dest-${suffix}`); + fs.writeFileSync(file, 'test', 'utf-8'); + fs.symlinkSync(file, link); + + fs.lchmod(link, input, common.mustCall((err) => { + assert.ifError(err); + assert.strictEqual(fs.lstatSync(link).mode & 0o777, mode); + })); + } + + if (fs.lchmodSync) { + const link = path.join(tmpdir.path, `lchmodSync-src-${suffix}`); + const file = path.join(tmpdir.path, `lchmodSync-dest-${suffix}`); + fs.writeFileSync(file, 'test', 'utf-8'); + fs.symlinkSync(file, link); + + fs.lchmodSync(link, input); + assert.strictEqual(fs.lstatSync(link).mode & 0o777, mode); + } +} + +test(mode, true); +test(mode, false); diff --git a/test/parallel/test-fs-chmod.js b/test/parallel/test-fs-chmod.js index 6727ec2144f2ce..ed5919ce887bc8 100644 --- a/test/parallel/test-fs-chmod.js +++ b/test/parallel/test-fs-chmod.js @@ -62,7 +62,7 @@ function closeSync() { } -// On Windows chmod is only able to manipulate read-only bit +// On Windows chmod is only able to manipulate write permission if (common.isWindows) { mode_async = 0o400; // read-only mode_sync = 0o600; // read-write @@ -112,10 +112,10 @@ fs.open(file2, 'w', common.mustCall((err, fd) => { common.expectsError( () => fs.fchmod(fd, {}), { - code: 'ERR_INVALID_ARG_TYPE', + code: 'ERR_INVALID_ARG_VALUE', type: TypeError, - message: 'The "mode" argument must be of type number. ' + - 'Received type object' + message: 'The argument \'mode\' must be a 32-bit unsigned integer ' + + 'or an octal string. Received {}' } ); diff --git a/test/parallel/test-fs-fchmod.js b/test/parallel/test-fs-fchmod.js index 63d780a57dfc66..df7748538a5cc4 100644 --- a/test/parallel/test-fs-fchmod.js +++ b/test/parallel/test-fs-fchmod.js @@ -1,6 +1,7 @@ 'use strict'; -const common = require('../common'); +require('../common'); const assert = require('assert'); +const util = require('util'); const fs = require('fs'); // This test ensures that input for fchmod is valid, testing for valid @@ -16,7 +17,16 @@ const fs = require('fs'); }; assert.throws(() => fs.fchmod(input), errObj); assert.throws(() => fs.fchmodSync(input), errObj); - errObj.message = errObj.message.replace('fd', 'mode'); +}); + + +[false, null, undefined, {}, [], '', '123x'].forEach((input) => { + const errObj = { + code: 'ERR_INVALID_ARG_VALUE', + name: 'TypeError [ERR_INVALID_ARG_VALUE]', + message: 'The argument \'mode\' must be a 32-bit unsigned integer or an ' + + `octal string. Received ${util.inspect(input)}` + }; assert.throws(() => fs.fchmod(1, input), errObj); assert.throws(() => fs.fchmodSync(1, input), errObj); }); @@ -62,27 +72,3 @@ const fs = require('fs'); assert.throws(() => fs.fchmod(1, input), errObj); assert.throws(() => fs.fchmodSync(1, input), errObj); }); - -// Check for mode values range -const modeUpperBoundaryValue = 0o777; -fs.fchmod(1, modeUpperBoundaryValue, common.mustCall()); -fs.fchmodSync(1, modeUpperBoundaryValue); - -// umask of 0o777 is equal to 775 -const modeOutsideUpperBoundValue = 776; -assert.throws( - () => fs.fchmod(1, modeOutsideUpperBoundValue), - { - code: 'ERR_OUT_OF_RANGE', - name: 'RangeError [ERR_OUT_OF_RANGE]', - message: 'The value of "mode" is out of range. Received 776' - } -); -assert.throws( - () => fs.fchmodSync(1, modeOutsideUpperBoundValue), - { - code: 'ERR_OUT_OF_RANGE', - name: 'RangeError [ERR_OUT_OF_RANGE]', - message: 'The value of "mode" is out of range. Received 776' - } -); diff --git a/test/parallel/test-fs-mkdir-mode-mask.js b/test/parallel/test-fs-mkdir-mode-mask.js new file mode 100644 index 00000000000000..e4e8a423483376 --- /dev/null +++ b/test/parallel/test-fs-mkdir-mode-mask.js @@ -0,0 +1,45 @@ +'use strict'; + +// This tests that mode > 0o777 will be masked off with 0o777 in fs.mkdir(). + +const common = require('../common'); +const assert = require('assert'); +const path = require('path'); +const fs = require('fs'); + +let mode; + +if (common.isWindows) { + common.skip('mode is not supported in mkdir on Windows'); + return; +} else { + mode = 0o644; +} + +const maskToIgnore = 0o10000; + +const tmpdir = require('../common/tmpdir'); +tmpdir.refresh(); + +function test(mode, asString) { + const suffix = asString ? 'str' : 'num'; + const input = asString ? + (mode | maskToIgnore).toString(8) : (mode | maskToIgnore); + + { + const dir = path.join(tmpdir.path, `mkdirSync-${suffix}`); + fs.mkdirSync(dir, input); + assert.strictEqual(fs.statSync(dir).mode & 0o777, mode); + } + + { + const dir = path.join(tmpdir.path, `mkdir-${suffix}`); + fs.mkdir(dir, input, common.mustCall((err) => { + assert.ifError(err); + assert.strictEqual(fs.statSync(dir).mode & 0o777, mode); + })); + } +} + +test(mode, true); +test(mode, false); diff --git a/test/parallel/test-fs-open-mode-mask.js b/test/parallel/test-fs-open-mode-mask.js new file mode 100644 index 00000000000000..4db41864af099c --- /dev/null +++ b/test/parallel/test-fs-open-mode-mask.js @@ -0,0 +1,48 @@ +'use strict'; + +// This tests that mode > 0o777 will be masked off with 0o777 in fs.open(). + +const common = require('../common'); +const assert = require('assert'); +const path = require('path'); +const fs = require('fs'); + +let mode; + +if (common.isWindows) { + mode = 0o444; +} else { + mode = 0o644; +} + +const maskToIgnore = 0o10000; + +const tmpdir = require('../common/tmpdir'); +tmpdir.refresh(); + +function test(mode, asString) { + const suffix = asString ? 'str' : 'num'; + const input = asString ? + (mode | maskToIgnore).toString(8) : (mode | maskToIgnore); + + { + const file = path.join(tmpdir.path, `openSync-${suffix}.txt`); + const fd = fs.openSync(file, 'w+', input); + assert.strictEqual(fs.fstatSync(fd).mode & 0o777, mode); + fs.closeSync(fd); + assert.strictEqual(fs.statSync(file).mode & 0o777, mode); + } + + { + const file = path.join(tmpdir.path, `open-${suffix}.txt`); + fs.open(file, 'w+', input, common.mustCall((err, fd) => { + assert.ifError(err); + assert.strictEqual(fs.fstatSync(fd).mode & 0o777, mode); + fs.closeSync(fd); + assert.strictEqual(fs.statSync(file).mode & 0o777, mode); + })); + } +} + +test(mode, true); +test(mode, false); diff --git a/test/parallel/test-fs-promises.js b/test/parallel/test-fs-promises.js index b7e01544162e81..53219342d93af8 100644 --- a/test/parallel/test-fs-promises.js +++ b/test/parallel/test-fs-promises.js @@ -114,28 +114,10 @@ function verifyStatObject(stat) { await fchmod(handle, 0o666); await handle.chmod(0o666); - // `mode` can't be > 0o777 - assert.rejects( - async () => chmod(handle, (0o777 + 1)), - { - code: 'ERR_INVALID_ARG_TYPE', - name: 'TypeError [ERR_INVALID_ARG_TYPE]' - } - ); - assert.rejects( - async () => fchmod(handle, (0o777 + 1)), - { - code: 'ERR_OUT_OF_RANGE', - name: 'RangeError [ERR_OUT_OF_RANGE]' - } - ); - assert.rejects( - async () => handle.chmod(handle, (0o777 + 1)), - { - code: 'ERR_INVALID_ARG_TYPE', - name: 'TypeError [ERR_INVALID_ARG_TYPE]' - } - ); + // Mode larger than 0o777 should be masked off. + await chmod(dest, (0o777 + 1)); + await fchmod(handle, 0o777 + 1); + await handle.chmod(0o777 + 1); await utimes(dest, new Date(), new Date()); diff --git a/test/parallel/test-process-umask-mask.js b/test/parallel/test-process-umask-mask.js new file mode 100644 index 00000000000000..c312c061d2b76a --- /dev/null +++ b/test/parallel/test-process-umask-mask.js @@ -0,0 +1,29 @@ +'use strict'; + +// This tests that mask > 0o777 will be masked off with 0o777 in +// process.umask() + +const common = require('../common'); +const assert = require('assert'); + +let mask; + +if (common.isWindows) { + mask = 0o600; +} else { + mask = 0o664; +} + +const maskToIgnore = 0o10000; + +const old = process.umask(); + +function test(input, output) { + process.umask(input); + assert.strictEqual(process.umask(), output); + + process.umask(old); +} + +test(mask | maskToIgnore, mask); +test((mask | maskToIgnore).toString(8), mask); diff --git a/test/parallel/test-process-umask.js b/test/parallel/test-process-umask.js index 1ac32cb7018906..c0c00f3ba397f8 100644 --- a/test/parallel/test-process-umask.js +++ b/test/parallel/test-process-umask.js @@ -33,13 +33,13 @@ if (common.isWindows) { const old = process.umask(mask); -assert.strictEqual(parseInt(mask, 8), process.umask(old)); +assert.strictEqual(process.umask(old), parseInt(mask, 8)); // confirm reading the umask does not modify it. // 1. If the test fails, this call will succeed, but the mask will be set to 0 -assert.strictEqual(old, process.umask()); +assert.strictEqual(process.umask(), old); // 2. If the test fails, process.umask() will return 0 -assert.strictEqual(old, process.umask()); +assert.strictEqual(process.umask(), old); assert.throws(() => { process.umask({});