fs: fix flag and mode validation

The `flag` and `mode` options were not being validated correctly. Signed-off-by: James M Snell <jasnell@gmail.com> Fixes: #37430 PR-URL: #37480 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
nodejs · Mar 1, 2021 · da217d0 · da217d0
1 parent 7c63bc6
commit da217d0
Show file tree

Hide file tree

Showing 9 changed files with 67 additions and 43 deletions.
diff --git a/lib/fs.js b/lib/fs.js
@@ -347,7 +347,7 @@ function readFile(path, options, callback) {
     return;
   }
 
-  const flagsNumber = stringToFlags(options.flag);
+  const flagsNumber = stringToFlags(options.flag, 'options.flag');
   path = getValidatedPath(path);
 
   const req = new FSReqCallback();
@@ -1284,6 +1284,7 @@ function fchmodSync(fd, mode) {
 
 function lchmod(path, mode, callback) {
   callback = maybeCallback(callback);
+  mode = parseFileMode(mode, 'mode');
   fs.open(path, O_WRONLY | O_SYMLINK, (err, fd) => {
     if (err) {
       callback(err);

diff --git a/lib/internal/fs/utils.js b/lib/internal/fs/utils.js
@@ -526,8 +526,9 @@ function getStatsFromBinding(stats, offset = 0) {
   );
 }
 
-function stringToFlags(flags) {
+function stringToFlags(flags, name = 'flags') {
   if (typeof flags === 'number') {
+    validateInt32(flags, name);
     return flags;
   }
 

diff --git a/lib/internal/validators.js b/lib/internal/validators.js
@@ -56,26 +56,16 @@ const modeDesc = 'must be a 32-bit unsigned integer or an octal string';
  * @returns {number}
  */
 function parseFileMode(value, name, def) {
-  if (value == null && def !== undefined) {
-    return def;
-  }
-
-  if (isUint32(value)) {
-    return value;
-  }
-
-  if (typeof value === 'number') {
-    validateInt32(value, name, 0, 2 ** 32 - 1);
-  }
-
+  value ??= def;
   if (typeof value === 'string') {
     if (!RegExpPrototypeTest(octalReg, value)) {
       throw new ERR_INVALID_ARG_VALUE(name, value, modeDesc);
     }
-    return NumberParseInt(value, 8);
+    value = NumberParseInt(value, 8);
   }
 
-  throw new ERR_INVALID_ARG_VALUE(name, value, modeDesc);
+  validateInt32(value, name, 0, 2 ** 32 - 1);
+  return value;
 }
 
 const validateInteger = hideStackFrames(

diff --git a/test/parallel/test-file-validate-mode-flag.js b/test/parallel/test-file-validate-mode-flag.js
@@ -0,0 +1,39 @@
+'use strict';
+
+// Checks for crash regression: https://github.com/nodejs/node/issues/37430
+
+const common = require('../common');
+const assert = require('assert');
+const {
+  open,
+  openSync,
+  promises: {
+    open: openPromise,
+  },
+} = require('fs');
+
+// These should throw, not crash.
+
+assert.throws(() => open(__filename, 2176057344, common.mustNotCall()), {
+  code: 'ERR_OUT_OF_RANGE'
+});
+
+assert.throws(() => open(__filename, 0, 2176057344, common.mustNotCall()), {
+  code: 'ERR_OUT_OF_RANGE'
+});
+
+assert.throws(() => openSync(__filename, 2176057344), {
+  code: 'ERR_OUT_OF_RANGE'
+});
+
+assert.throws(() => openSync(__filename, 0, 2176057344), {
+  code: 'ERR_OUT_OF_RANGE'
+});
+
+assert.rejects(openPromise(__filename, 2176057344), {
+  code: 'ERR_OUT_OF_RANGE'
+});
+
+assert.rejects(openPromise(__filename, 0, 2176057344), {
+  code: 'ERR_OUT_OF_RANGE'
+});
diff --git a/test/parallel/test-fs-chmod.js b/test/parallel/test-fs-chmod.js
@@ -106,10 +106,7 @@ fs.open(file2, 'w', common.mustSucceed((fd) => {
     assert.throws(
       () => fs.fchmod(fd, {}),
       {
-        code: 'ERR_INVALID_ARG_VALUE',
-        name: 'TypeError',
-        message: 'The argument \'mode\' must be a 32-bit unsigned integer ' +
-                 'or an octal string. Received {}'
+        code: 'ERR_INVALID_ARG_TYPE',
       }
     );
 

diff --git a/test/parallel/test-fs-fchmod.js b/test/parallel/test-fs-fchmod.js
@@ -1,7 +1,6 @@
 'use strict';
 const 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
@@ -20,17 +19,18 @@ const fs = require('fs');
 });
 
 
-[false, null, undefined, {}, [], '', '123x'].forEach((input) => {
+[false, null, {}, []].forEach((input) => {
   const errObj = {
-    code: 'ERR_INVALID_ARG_VALUE',
-    name: 'TypeError',
-    message: 'The argument \'mode\' must be a 32-bit unsigned integer or an ' +
-             `octal string. Received ${util.inspect(input)}`
+    code: 'ERR_INVALID_ARG_TYPE',
   };
   assert.throws(() => fs.fchmod(1, input), errObj);
   assert.throws(() => fs.fchmodSync(1, input), errObj);
 });
 
+assert.throws(() => fs.fchmod(1, '123x'), {
+  code: 'ERR_INVALID_ARG_VALUE'
+});
+
 [-1, 2 ** 32].forEach((input) => {
   const errObj = {
     code: 'ERR_OUT_OF_RANGE',

diff --git a/test/parallel/test-fs-lchmod.js b/test/parallel/test-fs-lchmod.js
@@ -2,7 +2,6 @@
 
 const common = require('../common');
 const assert = require('assert');
-const util = require('util');
 const fs = require('fs');
 const { promises } = fs;
 const f = __filename;
@@ -38,18 +37,22 @@ assert.throws(() => fs.lchmod(f, {}), { code: 'ERR_INVALID_CALLBACK' });
 });
 
 // Check mode
-[false, null, undefined, {}, [], '', '123x'].forEach((input) => {
+[false, null, {}, []].forEach((input) => {
   const errObj = {
-    code: 'ERR_INVALID_ARG_VALUE',
-    name: 'TypeError',
-    message: 'The argument \'mode\' must be a 32-bit unsigned integer or an ' +
-             `octal string. Received ${util.inspect(input)}`
+    code: 'ERR_INVALID_ARG_TYPE',
   };
 
   assert.rejects(promises.lchmod(f, input, () => {}), errObj);
   assert.throws(() => fs.lchmodSync(f, input), errObj);
 });
 
+assert.throws(() => fs.lchmod(f, '123x', common.mustNotCall()), {
+  code: 'ERR_INVALID_ARG_VALUE'
+});
+assert.throws(() => fs.lchmodSync(f, '123x'), {
+  code: 'ERR_INVALID_ARG_VALUE'
+});
+
 [-1, 2 ** 32].forEach((input) => {
   const errObj = {
     code: 'ERR_OUT_OF_RANGE',

diff --git a/test/parallel/test-fs-open.js b/test/parallel/test-fs-open.js
@@ -102,22 +102,19 @@ for (const extra of [[], ['r'], ['r', 0], ['r', 0, 'bad callback']]) {
   assert.throws(
     () => fs.open(__filename, 'r', mode, common.mustNotCall()),
     {
-      message: /'mode' must be a 32-bit/,
-      code: 'ERR_INVALID_ARG_VALUE'
+      code: 'ERR_INVALID_ARG_TYPE'
     }
   );
   assert.throws(
     () => fs.openSync(__filename, 'r', mode, common.mustNotCall()),
     {
-      message: /'mode' must be a 32-bit/,
-      code: 'ERR_INVALID_ARG_VALUE'
+      code: 'ERR_INVALID_ARG_TYPE'
     }
   );
   assert.rejects(
     fs.promises.open(__filename, 'r', mode),
     {
-      message: /'mode' must be a 32-bit/,
-      code: 'ERR_INVALID_ARG_VALUE'
+      code: 'ERR_INVALID_ARG_TYPE'
     }
   );
 });
diff --git a/test/parallel/test-process-umask.js b/test/parallel/test-process-umask.js
@@ -53,17 +53,13 @@ assert.strictEqual(process.umask(), old);
 assert.throws(() => {
   process.umask({});
 }, {
-  code: 'ERR_INVALID_ARG_VALUE',
-  message: 'The argument \'mask\' must be a 32-bit unsigned integer ' +
-           'or an octal string. Received {}'
+  code: 'ERR_INVALID_ARG_TYPE',
 });
 
 ['123x', 'abc', '999'].forEach((value) => {
   assert.throws(() => {
     process.umask(value);
   }, {
     code: 'ERR_INVALID_ARG_VALUE',
-    message: 'The argument \'mask\' must be a 32-bit unsigned integer ' +
-             `or an octal string. Received '${value}'`
   });
 });