fs: fix error handling

Right now there are multiple cases where the validated entry would
not be returned or a wrong error is thrown. This fixes both cases.

PR-URL: https://github.com/nodejs/node/pull/19445
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

lib/internal/fs.js

@@ -77,11 +77,16 @@ function isUint32(n) { return n === (n >>> 0); }
 function modeNum(m, def) {
   if (typeof m === 'number')
     return m;
-  if (typeof m === 'string')
+  if (typeof m === 'string') {
-    return parseInt(m, 8);
+    const parsed = parseInt(m, 8);
-  if (def)
+    if (Number.isNaN(parsed))
-    return modeNum(def);
+      return m;
-  return undefined;
+    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,
@@ -333,8 +338,14 @@ function validateBuffer(buffer) {
 function validateLen(len) {
   let err;
 
-  if (!isInt32(len))
+  if (!isInt32(len)) {
-    err = new ERR_INVALID_ARG_TYPE('len', 'integer');
+    if (typeof value !== 'number') {
+      err = new ERR_INVALID_ARG_TYPE('len', 'number', len);
+    } else {
+      // TODO(BridgeAR): Improve this error message.
+      err = new ERR_OUT_OF_RANGE('len', 'an integer', len);
+    }
+  }
 
   if (err !== undefined) {
     Error.captureStackTrace(err, validateLen);
@@ -388,12 +399,16 @@ function validatePath(path, propName = 'path') {
 }
 
 function validateUint32(value, propName) {
+  if (!isUint32(value)) {
     let err;
-
+    if (typeof value !== 'number') {
-  if (!isUint32(value))
+      err = new ERR_INVALID_ARG_TYPE(propName, 'number', value);
-    err = new ERR_INVALID_ARG_TYPE(propName, 'integer');
+    } else if (!Number.isInteger(value)) {
-
+      err = new ERR_OUT_OF_RANGE(propName, 'an integer', value);
-  if (err !== undefined) {
+    } else {
+      // 2 ** 32 === 4294967296
+      err = new ERR_OUT_OF_RANGE(propName, '>= 0 && < 4294967296', value);
+    }
     Error.captureStackTrace(err, validateUint32);
     throw err;
   }

test/parallel/test-fs-chmod.js

@@ -114,7 +114,8 @@ fs.open(file2, 'w', common.mustCall((err, fd) => {
       {
         code: 'ERR_INVALID_ARG_TYPE',
         type: TypeError,
-        message: 'The "mode" argument must be of type integer'
+        message: 'The "mode" argument must be of type number. ' +
+                 'Received type object'
       }
     );
 
@@ -150,7 +151,8 @@ if (fs.lchmod) {
   const errObj = {
     code: 'ERR_INVALID_ARG_TYPE',
     name: 'TypeError [ERR_INVALID_ARG_TYPE]',
-    message: 'The "fd" argument must be of type integer'
+    message: 'The "fd" argument must be of type number. ' +
+             `Received type ${typeof input}`
   };
   assert.throws(() => fs.fchmod(input, 0o000), errObj);
   assert.throws(() => fs.fchmodSync(input, 0o000), errObj);

test/parallel/test-fs-close-errors.js

@@ -11,7 +11,8 @@ const fs = require('fs');
   const errObj = {
     code: 'ERR_INVALID_ARG_TYPE',
     name: 'TypeError [ERR_INVALID_ARG_TYPE]',
-    message: 'The "fd" argument must be of type integer'
+    message: 'The "fd" argument must be of type number. ' +
+             `Received type ${typeof input}`
   };
   assert.throws(() => fs.close(input), errObj);
   assert.throws(() => fs.closeSync(input), errObj);

test/parallel/test-fs-fchmod.js

@@ -1,45 +1,66 @@
 'use strict';
 const common = require('../common');
+const assert = require('assert');
 const fs = require('fs');
 
 // This test ensures that input for fchmod is valid, testing for valid
 // inputs for fd and mode
 
 // Check input type
-['', false, null, undefined, {}, [], Infinity, -1].forEach((i) => {
+[false, null, undefined, {}, [], ''].forEach((input) => {
-  common.expectsError(
+  const errObj = {
-    () => fs.fchmod(i),
-    {
     code: 'ERR_INVALID_ARG_TYPE',
-      type: TypeError,
+    name: 'TypeError [ERR_INVALID_ARG_TYPE]',
-      message: 'The "fd" argument must be of type integer'
+    message: 'The "fd" argument must be of type number. Received type ' +
-    }
+             typeof input
-  );
+  };
-  common.expectsError(
+  assert.throws(() => fs.fchmod(input), errObj);
-    () => fs.fchmodSync(i),
+  assert.throws(() => fs.fchmodSync(input), errObj);
-    {
+  errObj.message = errObj.message.replace('fd', 'mode');
-      code: 'ERR_INVALID_ARG_TYPE',
+  assert.throws(() => fs.fchmod(1, input), errObj);
-      type: TypeError,
+  assert.throws(() => fs.fchmodSync(1, input), errObj);
-      message: 'The "fd" argument must be of type integer'
+});
-    }
-  );
 
-  common.expectsError(
+[-1, 2 ** 32].forEach((input) => {
-    () => fs.fchmod(1, i),
+  const errObj = {
-    {
+    code: 'ERR_OUT_OF_RANGE',
-      code: 'ERR_INVALID_ARG_TYPE',
+    name: 'RangeError [ERR_OUT_OF_RANGE]',
-      type: TypeError,
+    message: 'The value of "fd" is out of range. It must be >= 0 && < ' +
-      message: 'The "mode" argument must be of type integer'
+             `${2 ** 32}. Received ${input}`
-    }
+  };
-  );
+  assert.throws(() => fs.fchmod(input), errObj);
-  common.expectsError(
+  assert.throws(() => fs.fchmodSync(input), errObj);
-    () => fs.fchmodSync(1, i),
+  errObj.message = errObj.message.replace('fd', 'mode');
-    {
+  assert.throws(() => fs.fchmod(1, input), errObj);
-      code: 'ERR_INVALID_ARG_TYPE',
+  assert.throws(() => fs.fchmodSync(1, input), errObj);
-      type: TypeError,
+});
-      message: 'The "mode" argument must be of type integer'
+
-    }
+[NaN, Infinity].forEach((input) => {
-  );
+  const errObj = {
+    code: 'ERR_OUT_OF_RANGE',
+    name: 'RangeError [ERR_OUT_OF_RANGE]',
+    message: 'The value of "fd" is out of range. It must be an integer. ' +
+             `Received ${input}`
+  };
+  assert.throws(() => fs.fchmod(input), errObj);
+  assert.throws(() => fs.fchmodSync(input), errObj);
+  errObj.message = errObj.message.replace('fd', 'mode');
+  assert.throws(() => fs.fchmod(1, input), errObj);
+  assert.throws(() => fs.fchmodSync(1, input), errObj);
+});
+
+[1.5].forEach((input) => {
+  const errObj = {
+    code: 'ERR_OUT_OF_RANGE',
+    name: 'RangeError [ERR_OUT_OF_RANGE]',
+    message: 'The value of "fd" is out of range. It must be an integer. ' +
+             `Received ${input}`
+  };
+  assert.throws(() => fs.fchmod(input), errObj);
+  assert.throws(() => fs.fchmodSync(input), errObj);
+  errObj.message = errObj.message.replace('fd', 'mode');
+  assert.throws(() => fs.fchmod(1, input), errObj);
+  assert.throws(() => fs.fchmodSync(1, input), errObj);
 });
 
 // Check for mode values range

test/parallel/test-fs-fchown.js

@@ -1,57 +1,46 @@
 'use strict';
 
-const common = require('../common');
+require('../common');
+const assert = require('assert');
 const fs = require('fs');
 
-['', false, null, undefined, {}, [], Infinity, -1].forEach((i) => {
+function test(input, errObj) {
-  common.expectsError(
+  assert.throws(() => fs.fchown(input), errObj);
-    () => fs.fchown(i),
+  assert.throws(() => fs.fchownSync(input), errObj);
-    {
+  errObj.message = errObj.message.replace('fd', 'uid');
-      code: 'ERR_INVALID_ARG_TYPE',
+  assert.throws(() => fs.fchown(1, input), errObj);
-      type: TypeError,
+  assert.throws(() => fs.fchownSync(1, input), errObj);
-      message: 'The "fd" argument must be of type integer'
+  errObj.message = errObj.message.replace('uid', 'gid');
+  assert.throws(() => fs.fchown(1, 1, input), errObj);
+  assert.throws(() => fs.fchownSync(1, 1, input), errObj);
 }
-  );
-  common.expectsError(
-    () => fs.fchownSync(i),
-    {
-      code: 'ERR_INVALID_ARG_TYPE',
-      type: TypeError,
-      message: 'The "fd" argument must be of type integer'
-    }
-  );
 
-  common.expectsError(
+['', false, null, undefined, {}, []].forEach((input) => {
-    () => fs.fchown(1, i),
+  const errObj = {
-    {
     code: 'ERR_INVALID_ARG_TYPE',
-      type: TypeError,
+    name: 'TypeError [ERR_INVALID_ARG_TYPE]',
-      message: 'The "uid" argument must be of type integer'
+    message: 'The "fd" argument must be of type number. Received type ' +
-    }
+             typeof input
-  );
+  };
-  common.expectsError(
+  test(input, errObj);
-    () => fs.fchownSync(1, i),
+});
-    {
+
-      code: 'ERR_INVALID_ARG_TYPE',
+[Infinity, NaN].forEach((input) => {
-      type: TypeError,
+  const errObj = {
-      message: 'The "uid" argument must be of type integer'
+    code: 'ERR_OUT_OF_RANGE',
-    }
+    name: 'RangeError [ERR_OUT_OF_RANGE]',
-  );
+    message: 'The value of "fd" is out of range. It must be an integer. ' +
-
+             `Received ${input}`
-  common.expectsError(
+  };
-    () => fs.fchown(1, 1, i),
+  test(input, errObj);
-    {
+});
-      code: 'ERR_INVALID_ARG_TYPE',
+
-      type: TypeError,
+[-1, 2 ** 32].forEach((input) => {
-      message: 'The "gid" argument must be of type integer'
+  const errObj = {
-    }
+    code: 'ERR_OUT_OF_RANGE',
-  );
+    name: 'RangeError [ERR_OUT_OF_RANGE]',
-  common.expectsError(
+    message: 'The value of "fd" is out of range. It must be ' +
-    () => fs.fchownSync(1, 1, i),
+             `>= 0 && < 4294967296. Received ${input}`
-    {
+  };
-      code: 'ERR_INVALID_ARG_TYPE',
+  test(input, errObj);
-      type: TypeError,
-      message: 'The "gid" argument must be of type integer'
-    }
-  );
 });

test/parallel/test-fs-fsync.js

@@ -50,37 +50,15 @@ fs.open(fileTemp, 'a', 0o777, common.mustCall(function(err, fd) {
   }));
 }));
 
-['', false, null, undefined, {}, []].forEach((i) => {
+['', false, null, undefined, {}, []].forEach((input) => {
-  common.expectsError(
+  const errObj = {
-    () => fs.fdatasync(i),
-    {
     code: 'ERR_INVALID_ARG_TYPE',
-      type: TypeError,
+    name: 'TypeError [ERR_INVALID_ARG_TYPE]',
-      message: 'The "fd" argument must be of type integer'
+    message: 'The "fd" argument must be of type number. Received type ' +
-    }
+             typeof input
-  );
+  };
-  common.expectsError(
+  assert.throws(() => fs.fdatasync(input), errObj);
-    () => fs.fdatasyncSync(i),
+  assert.throws(() => fs.fdatasyncSync(input), errObj);
-    {
+  assert.throws(() => fs.fsync(input), errObj);
-      code: 'ERR_INVALID_ARG_TYPE',
+  assert.throws(() => fs.fsyncSync(input), errObj);
-      type: TypeError,
-      message: 'The "fd" argument must be of type integer'
-    }
-  );
-  common.expectsError(
-    () => fs.fsync(i),
-    {
-      code: 'ERR_INVALID_ARG_TYPE',
-      type: TypeError,
-      message: 'The "fd" argument must be of type integer'
-    }
-  );
-  common.expectsError(
-    () => fs.fsyncSync(i),
-    {
-      code: 'ERR_INVALID_ARG_TYPE',
-      type: TypeError,
-      message: 'The "fd" argument must be of type integer'
-    }
-  );
 });

test/parallel/test-fs-read-type.js

@@ -30,7 +30,8 @@ common.expectsError(
   }, {
     code: 'ERR_INVALID_ARG_TYPE',
     type: TypeError,
-    message: 'The "fd" argument must be of type integer'
+    message: 'The "fd" argument must be of type number. ' +
+             `Received type ${typeof value}`
   });
 });
 
@@ -75,7 +76,8 @@ common.expectsError(
   }, {
     code: 'ERR_INVALID_ARG_TYPE',
     type: TypeError,
-    message: 'The "fd" argument must be of type integer'
+    message: 'The "fd" argument must be of type number. ' +
+             `Received type ${typeof value}`
   });
 });
 

test/parallel/test-fs-stat.js

@@ -138,7 +138,8 @@ fs.stat(__filename, common.mustCall(function(err, s) {
       {
         code: 'ERR_INVALID_ARG_TYPE',
         name: 'TypeError [ERR_INVALID_ARG_TYPE]',
-        message: 'The "fd" argument must be of type integer'
+        message: 'The "fd" argument must be of type number. ' +
+                 `Received type ${typeof input}`
       }
     );
   });

test/parallel/test-fs-symlink.js

@@ -35,7 +35,7 @@ let fileTime;
 const tmpdir = require('../common/tmpdir');
 tmpdir.refresh();
 
-// test creating and reading symbolic link
+// Test creating and reading symbolic link
 const linkData = fixtures.path('/cycles/root.js');
 const linkPath = path.join(tmpdir.path, 'symlink1.js');
 
@@ -58,63 +58,29 @@ fs.symlink(linkData, linkPath, common.mustCall(function(err) {
   }));
 }));
 
-[false, 1, {}, [], null, undefined].forEach((i) => {
+[false, 1, {}, [], null, undefined].forEach((input) => {
-  common.expectsError(
+  const errObj = {
-    () => fs.symlink(i, '', common.mustNotCall()),
-    {
     code: 'ERR_INVALID_ARG_TYPE',
-      type: TypeError,
+    name: 'TypeError [ERR_INVALID_ARG_TYPE]',
-      message:
+    message: 'The "target" argument must be one of type string, Buffer, or ' +
-        'The "target" argument must be one of type string, Buffer, or URL'
+             `URL. Received type ${typeof input}`
-    }
+  };
-  );
+  assert.throws(() => fs.symlink(input, '', common.mustNotCall()), errObj);
-  common.expectsError(
+  assert.throws(() => fs.symlinkSync(input, ''), errObj);
-    () => fs.symlink('', i, common.mustNotCall()),
+
-    {
+  errObj.message = errObj.message.replace('target', 'path');
-      code: 'ERR_INVALID_ARG_TYPE',
+  assert.throws(() => fs.symlink('', input, common.mustNotCall()), errObj);
-      type: TypeError,
+  assert.throws(() => fs.symlinkSync('', input), errObj);
-      message:
-        'The "path" argument must be one of type string, Buffer, or URL'
-    }
-  );
-  common.expectsError(
-    () => fs.symlinkSync(i, ''),
-    {
-      code: 'ERR_INVALID_ARG_TYPE',
-      type: TypeError,
-      message:
-        'The "target" argument must be one of type string, Buffer, or URL'
-    }
-  );
-  common.expectsError(
-    () => fs.symlinkSync('', i),
-    {
-      code: 'ERR_INVALID_ARG_TYPE',
-      type: TypeError,
-      message:
-        'The "path" argument must be one of type string, Buffer, or URL'
-    }
-  );
 });
 
-common.expectsError(
+const errObj = {
-  () => fs.symlink('', '', '🍏', common.mustNotCall()),
-  {
   code: 'ERR_FS_INVALID_SYMLINK_TYPE',
-    type: Error,
+  name: 'Error [ERR_FS_INVALID_SYMLINK_TYPE]',
   message:
     'Symlink type must be one of "dir", "file", or "junction". Received "🍏"'
-  }
+};
-);
+assert.throws(() => fs.symlink('', '', '🍏', common.mustNotCall()), errObj);
-common.expectsError(
+assert.throws(() => fs.symlinkSync('', '', '🍏'), errObj);
-  () => fs.symlinkSync('', '', '🍏'),
-  {
-    code: 'ERR_FS_INVALID_SYMLINK_TYPE',
-    type: Error,
-    message:
-      'Symlink type must be one of "dir", "file", or "junction". Received "🍏"'
-  }
-);
 
 process.on('exit', function() {
   assert.notStrictEqual(linkTime, fileTime);

test/parallel/test-fs-truncate.js

@@ -184,7 +184,8 @@ function testFtruncate(cb) {
       {
         code: 'ERR_INVALID_ARG_TYPE',
         name: 'TypeError [ERR_INVALID_ARG_TYPE]',
-        message: 'The "len" argument must be of type integer'
+        message: 'The "len" argument must be of type number. ' +
+                 `Received type ${typeof input}`
       }
     );
   });
@@ -213,7 +214,8 @@ function testFtruncate(cb) {
       {
         code: 'ERR_INVALID_ARG_TYPE',
         name: 'TypeError [ERR_INVALID_ARG_TYPE]',
-        message: 'The "fd" argument must be of type integer'
+        message: 'The "fd" argument must be of type number. ' +
+                 `Received type ${typeof input}`
       }
     );
   });

test/parallel/test-fs-utimes.js

@@ -101,8 +101,10 @@ function testIt(atime, mtime, callback) {
     common.expectsError(
       () => fs.futimesSync(-1, atime, mtime),
       {
-        code: 'ERR_INVALID_ARG_TYPE',
+        code: 'ERR_OUT_OF_RANGE',
-        type: TypeError
+        type: RangeError,
+        message: 'The value of "fd" is out of range. ' +
+                'It must be >= 0 && < 4294967296. Received -1'
       }
     );
     tests_run++;
@@ -130,8 +132,10 @@ function testIt(atime, mtime, callback) {
         common.expectsError(
           () => fs.futimes(-1, atime, mtime, common.mustNotCall()),
           {
-            code: 'ERR_INVALID_ARG_TYPE',
+            code: 'ERR_OUT_OF_RANGE',
-            type: TypeError,
+            type: RangeError,
+            message: 'The value of "fd" is out of range. ' +
+                    'It must be >= 0 && < 4294967296. Received -1'
           }
         );