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: #20636 Fixes: #20498 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
nodejs · May 17, 2018 · a18e130 · a18e130
1 parent 0d9500d
commit a18e130
Show file tree

Hide file tree

Showing 13 changed files with 323 additions and 141 deletions.
diff --git a/lib/fs.js b/lib/fs.js
@@ -52,7 +52,6 @@ const internalUtil = require('internal/util');
 const {
   copyObject,
   getOptions,
-  modeNum,
   nullCheck,
   preprocessSymlinkDestination,
   Stats,
@@ -71,6 +70,7 @@ const {
 } = require('internal/constants');
 const {
   isUint32,
+  validateAndMaskMode,
   validateInt32,
   validateUint32
 } = require('internal/validators');
@@ -531,32 +531,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;
@@ -834,12 +838,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;
@@ -849,8 +857,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);
@@ -1032,25 +1039,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);
@@ -1091,11 +1091,10 @@ if (constants.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;
@@ -1105,8 +1104,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
@@ -12,16 +12,14 @@ 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');
 const {
   copyObject,
   getOptions,
   getStatsFromBinding,
-  modeNum,
   nullCheck,
   preprocessSymlinkDestination,
   stringToFlags,
@@ -34,6 +32,7 @@ const {
 } = require('internal/fs/utils');
 const {
   isUint32,
+  validateAndMaskMode,
   validateInt32,
   validateUint32
 } = require('internal/validators');
@@ -191,10 +190,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),
@@ -287,10 +285,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);
 }
 
@@ -361,19 +358,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
@@ -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/process/methods.js b/lib/internal/process/methods.js
@@ -2,10 +2,10 @@
 
 const {
   ERR_INVALID_ARG_TYPE,
-  ERR_INVALID_ARG_VALUE,
   ERR_UNKNOWN_CREDENTIAL
 } = require('internal/errors').codes;
 const {
+  validateAndMaskMode,
   validateUint32
 } = require('internal/validators');
 
@@ -30,29 +30,13 @@ function setupProcessMethods() {
     return _chdir(directory);
   }
 
-  const octalReg = /^[0-7]+$/;
   function umask(mask) {
-    if (typeof mask === 'undefined') {
+    if (mask === undefined) {
+      // Get the mask
       return _umask(mask);
     }
-
-    if (typeof mask === 'number') {
-      validateUint32(mask, 'mask');
-      return _umask(mask);
-    }
-
-    if (typeof mask === 'string') {
-      if (!octalReg.test(mask)) {
-        throw new ERR_INVALID_ARG_VALUE('mask', mask,
-                                        'must be an octal string');
-      }
-      const octal = Number.parseInt(mask, 8);
-      validateUint32(octal, 'mask');
-      return _umask(octal);
-    }
-
-    throw new ERR_INVALID_ARG_TYPE('mask', ['number', 'string', 'undefined'],
-                                   mask);
+    mask = validateAndMaskMode(mask, 'mask');
+    return _umask(mask);
   }
 }
 

diff --git 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 validateInt32(value, name) {
   if (!isInt32(value)) {
     let err;
@@ -53,6 +88,7 @@ function validateUint32(value, name, positive) {
 module.exports = {
   isInt32,
   isUint32,
+  validateAndMaskMode,
   validateInt32,
   validateUint32
 };