fs: improve mode validation

Do not return a default mode in case invalid mode values are provided. This strictens and simplifies the function by reusing existing functionality. PR-URL: #26575 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Masashi Hirano <shisama07@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>
nodejs · Mar 15, 2019 · 1cdeb9f · 1cdeb9f
1 parent 6f77af5
commit 1cdeb9f
Show file tree

Hide file tree

Showing 4 changed files with 41 additions and 31 deletions.
diff --git a/lib/internal/validators.js b/lib/internal/validators.js
@@ -35,24 +35,17 @@ function validateMode(value, name, def) {
   }
 
   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);
-    }
+    validateInt32(value, name, 0, 2 ** 32 - 1);
   }
 
   if (typeof value === 'string') {
     if (!octalReg.test(value)) {
       throw new ERR_INVALID_ARG_VALUE(name, value, modeDesc);
     }
-    const parsed = parseInt(value, 8);
-    return parsed;
+    return parseInt(value, 8);
   }
 
-  // TODO(BridgeAR): Only return `def` in case `value == null`
-  if (def !== undefined) {
+  if (def !== undefined && value == null) {
     return def;
   }
 

diff --git a/test/parallel/test-fs-fchmod.js b/test/parallel/test-fs-fchmod.js
@@ -46,8 +46,8 @@ const fs = require('fs');
   const errObj = {
     code: 'ERR_OUT_OF_RANGE',
     name: 'RangeError [ERR_OUT_OF_RANGE]',
-    message: 'The value of "mode" is out of range. It must be >= 0 && < ' +
-             `4294967296. Received ${input}`
+    message: 'The value of "mode" is out of range. It must be >= 0 && <= ' +
+             `4294967295. Received ${input}`
   };
 
   assert.throws(() => fs.fchmod(1, input), errObj);

diff --git a/test/parallel/test-fs-lchmod.js b/test/parallel/test-fs-lchmod.js
@@ -46,22 +46,18 @@ assert.throws(() => fs.lchmod(f, {}), { code: 'ERR_INVALID_CALLBACK' });
              `octal string. Received ${util.inspect(input)}`
   };
 
-  promises.lchmod(f, input, () => {})
-    .then(common.mustNotCall())
-    .catch(common.expectsError(errObj));
+  assert.rejects(promises.lchmod(f, input, () => {}), errObj);
   assert.throws(() => fs.lchmodSync(f, input), errObj);
 });
 
 [-1, 2 ** 32].forEach((input) => {
   const errObj = {
     code: 'ERR_OUT_OF_RANGE',
     name: 'RangeError [ERR_OUT_OF_RANGE]',
-    message: 'The value of "mode" is out of range. It must be >= 0 && < ' +
-             `4294967296. Received ${input}`
+    message: 'The value of "mode" is out of range. It must be >= 0 && <= ' +
+             `4294967295. Received ${input}`
   };
 
-  promises.lchmod(f, input, () => {})
-    .then(common.mustNotCall())
-    .catch(common.expectsError(errObj));
+  assert.rejects(promises.lchmod(f, input, () => {}), errObj);
   assert.throws(() => fs.lchmodSync(f, input), errObj);
 });
diff --git a/test/parallel/test-fs-open.js b/test/parallel/test-fs-open.js
@@ -98,15 +98,36 @@ for (const extra of [[], ['r'], ['r', 0], ['r', 0, 'bad callback']]) {
       type: TypeError
     }
   );
-  fs.promises.open(i, 'r')
-    .then(common.mustNotCall())
-    .catch(common.mustCall((err) => {
-      common.expectsError(
-        () => { throw err; },
-        {
-          code: 'ERR_INVALID_ARG_TYPE',
-          type: TypeError
-        }
-      );
-    }));
+  assert.rejects(
+    fs.promises.open(i, 'r'),
+    {
+      code: 'ERR_INVALID_ARG_TYPE',
+      name: 'TypeError [ERR_INVALID_ARG_TYPE]'
+    }
+  );
+});
+
+// Check invalid modes.
+[false, [], {}].forEach((mode) => {
+  assert.throws(
+    () => fs.open(__filename, 'r', mode, common.mustNotCall()),
+    {
+      message: /'mode' must be a 32-bit/,
+      code: 'ERR_INVALID_ARG_VALUE'
+    }
+  );
+  assert.throws(
+    () => fs.openSync(__filename, 'r', mode, common.mustNotCall()),
+    {
+      message: /'mode' must be a 32-bit/,
+      code: 'ERR_INVALID_ARG_VALUE'
+    }
+  );
+  assert.rejects(
+    fs.promises.open(__filename, 'r', mode),
+    {
+      message: /'mode' must be a 32-bit/,
+      code: 'ERR_INVALID_ARG_VALUE'
+    }
+  );
 });