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>

11 files changed, 178 insertions, 197 deletions

diff --git a/lib/internal/fs.js b/lib/internal/fs.js
index bcbb199ec3..4834d92fa0 100644
--- a/lib/internal/fs.js
+++ b/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')
-    return parseInt(m, 8);
-  if (def)
-    return modeNum(def);
-  return undefined;
+  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,
@@ -333,8 +338,14 @@ function validateBuffer(buffer) {
 function validateLen(len) {
   let err;
 
-  if (!isInt32(len))
-    err = new ERR_INVALID_ARG_TYPE('len', 'integer');
+  if (!isInt32(len)) {
+    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) {
-  let err;
-
-  if (!isUint32(value))
-    err = new ERR_INVALID_ARG_TYPE(propName, 'integer');
-
-  if (err !== undefined) {
+  if (!isUint32(value)) {
+    let err;
+    if (typeof value !== 'number') {
+      err = new ERR_INVALID_ARG_TYPE(propName, 'number', value);
+    } else if (!Number.isInteger(value)) {
+      err = new ERR_OUT_OF_RANGE(propName, 'an integer', value);
+    } else {
+      // 2 ** 32 === 4294967296
+      err = new ERR_OUT_OF_RANGE(propName, '>= 0 && < 4294967296', value);
+    }
     Error.captureStackTrace(err, validateUint32);
     throw err;
   }
diff --git a/test/parallel/test-fs-chmod.js b/test/parallel/test-fs-chmod.js
index a167fa49ba..6727ec2144 100644
--- a/test/parallel/test-fs-chmod.js
+++ b/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);
diff --git a/test/parallel/test-fs-close-errors.js b/test/parallel/test-fs-close-errors.js
index 3be8b4ce33..48af5eb485 100644
--- a/test/parallel/test-fs-close-errors.js
+++ b/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);
diff --git a/test/parallel/test-fs-fchmod.js b/test/parallel/test-fs-fchmod.js
index 22e6a490c9..edf5cc32ea 100644
--- a/test/parallel/test-fs-fchmod.js
+++ b/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) => {
-  common.expectsError(
-    () => fs.fchmod(i),
-    {
-      code: 'ERR_INVALID_ARG_TYPE',
-      type: TypeError,
-      message: 'The "fd" argument must be of type integer'
-    }
-  );
-  common.expectsError(
-    () => fs.fchmodSync(i),
-    {
-      code: 'ERR_INVALID_ARG_TYPE',
-      type: TypeError,
-      message: 'The "fd" argument must be of type integer'
-    }
-  );
+[false, null, undefined, {}, [], ''].forEach((input) => {
+  const errObj = {
+    code: 'ERR_INVALID_ARG_TYPE',
+    name: 'TypeError [ERR_INVALID_ARG_TYPE]',
+    message: 'The "fd" argument must be of type number. Received type ' +
+             typeof 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, 2 ** 32].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 >= 0 && < ' +
+             `${2 ** 32}. 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);
+});
 
-  common.expectsError(
-    () => fs.fchmod(1, i),
-    {
-      code: 'ERR_INVALID_ARG_TYPE',
-      type: TypeError,
-      message: 'The "mode" argument must be of type integer'
-    }
-  );
-  common.expectsError(
-    () => fs.fchmodSync(1, i),
-    {
-      code: 'ERR_INVALID_ARG_TYPE',
-      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
diff --git a/test/parallel/test-fs-fchown.js b/test/parallel/test-fs-fchown.js
index a7e6bf6cbc..500c06a47c 100644
--- a/test/parallel/test-fs-fchown.js
+++ b/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) => {
-  common.expectsError(
-    () => fs.fchown(i),
-    {
-      code: 'ERR_INVALID_ARG_TYPE',
-      type: TypeError,
-      message: 'The "fd" argument must be of type integer'
-    }
-  );
-  common.expectsError(
-    () => fs.fchownSync(i),
-    {
-      code: 'ERR_INVALID_ARG_TYPE',
-      type: TypeError,
-      message: 'The "fd" argument must be of type integer'
-    }
-  );
+function test(input, errObj) {
+  assert.throws(() => fs.fchown(input), errObj);
+  assert.throws(() => fs.fchownSync(input), errObj);
+  errObj.message = errObj.message.replace('fd', 'uid');
+  assert.throws(() => fs.fchown(1, input), errObj);
+  assert.throws(() => fs.fchownSync(1, input), errObj);
+  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.fchown(1, i),
-    {
-      code: 'ERR_INVALID_ARG_TYPE',
-      type: TypeError,
-      message: 'The "uid" argument must be of type integer'
-    }
-  );
-  common.expectsError(
-    () => fs.fchownSync(1, i),
-    {
-      code: 'ERR_INVALID_ARG_TYPE',
-      type: TypeError,
-      message: 'The "uid" argument must be of type integer'
-    }
-  );
+['', false, null, undefined, {}, []].forEach((input) => {
+  const errObj = {
+    code: 'ERR_INVALID_ARG_TYPE',
+    name: 'TypeError [ERR_INVALID_ARG_TYPE]',
+    message: 'The "fd" argument must be of type number. Received type ' +
+             typeof input
+  };
+  test(input, errObj);
+});
+
+[Infinity, NaN].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}`
+  };
+  test(input, errObj);
+});
 
-  common.expectsError(
-    () => fs.fchown(1, 1, i),
-    {
-      code: 'ERR_INVALID_ARG_TYPE',
-      type: TypeError,
-      message: 'The "gid" argument must be of type integer'
-    }
-  );
-  common.expectsError(
-    () => fs.fchownSync(1, 1, i),
-    {
-      code: 'ERR_INVALID_ARG_TYPE',
-      type: TypeError,
-      message: 'The "gid" argument must be of type integer'
-    }
-  );
+[-1, 2 ** 32].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 ' +
+             `>= 0 && < 4294967296. Received ${input}`
+  };
+  test(input, errObj);
 });
diff --git a/test/parallel/test-fs-fsync.js b/test/parallel/test-fs-fsync.js
index 1f575881e3..4d96091f34 100644
--- a/test/parallel/test-fs-fsync.js
+++ b/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) => {
-  common.expectsError(
-    () => fs.fdatasync(i),
-    {
-      code: 'ERR_INVALID_ARG_TYPE',
-      type: TypeError,
-      message: 'The "fd" argument must be of type integer'
-    }
-  );
-  common.expectsError(
-    () => fs.fdatasyncSync(i),
-    {
-      code: 'ERR_INVALID_ARG_TYPE',
-      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'
-    }
-  );
+['', false, null, undefined, {}, []].forEach((input) => {
+  const errObj = {
+    code: 'ERR_INVALID_ARG_TYPE',
+    name: 'TypeError [ERR_INVALID_ARG_TYPE]',
+    message: 'The "fd" argument must be of type number. Received type ' +
+             typeof input
+  };
+  assert.throws(() => fs.fdatasync(input), errObj);
+  assert.throws(() => fs.fdatasyncSync(input), errObj);
+  assert.throws(() => fs.fsync(input), errObj);
+  assert.throws(() => fs.fsyncSync(input), errObj);
 });
diff --git a/test/parallel/test-fs-read-type.js b/test/parallel/test-fs-read-type.js
index b47a7be177..55049ed79f 100644
--- a/test/parallel/test-fs-read-type.js
+++ b/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}`
   });
 });
 
diff --git a/test/parallel/test-fs-stat.js b/test/parallel/test-fs-stat.js
index bbb3281187..1003890bb8 100644
--- a/test/parallel/test-fs-stat.js
+++ b/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}`
       }
     );
   });
diff --git a/test/parallel/test-fs-symlink.js b/test/parallel/test-fs-symlink.js
index 19903fff58..ddcf7a63ff 100644
--- a/test/parallel/test-fs-symlink.js
+++ b/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) => {
-  common.expectsError(
-    () => fs.symlink(i, '', common.mustNotCall()),
-    {
-      code: 'ERR_INVALID_ARG_TYPE',
-      type: TypeError,
-      message:
-        'The "target" argument must be one of type string, Buffer, or URL'
-    }
-  );
-  common.expectsError(
-    () => fs.symlink('', i, common.mustNotCall()),
-    {
-      code: 'ERR_INVALID_ARG_TYPE',
-      type: TypeError,
-      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'
-    }
-  );
+[false, 1, {}, [], null, undefined].forEach((input) => {
+  const errObj = {
+    code: 'ERR_INVALID_ARG_TYPE',
+    name: 'TypeError [ERR_INVALID_ARG_TYPE]',
+    message: 'The "target" argument must be one of type string, Buffer, or ' +
+             `URL. Received type ${typeof input}`
+  };
+  assert.throws(() => fs.symlink(input, '', common.mustNotCall()), errObj);
+  assert.throws(() => fs.symlinkSync(input, ''), errObj);
+
+  errObj.message = errObj.message.replace('target', 'path');
+  assert.throws(() => fs.symlink('', input, common.mustNotCall()), errObj);
+  assert.throws(() => fs.symlinkSync('', input), errObj);
 });
 
-common.expectsError(
-  () => fs.symlink('', '', '🍏', common.mustNotCall()),
-  {
-    code: 'ERR_FS_INVALID_SYMLINK_TYPE',
-    type: Error,
-    message:
-      'Symlink type must be one of "dir", "file", or "junction". Received "🍏"'
-  }
-);
-common.expectsError(
-  () => fs.symlinkSync('', '', '🍏'),
-  {
-    code: 'ERR_FS_INVALID_SYMLINK_TYPE',
-    type: Error,
-    message:
-      'Symlink type must be one of "dir", "file", or "junction". Received "🍏"'
-  }
-);
+const errObj = {
+  code: 'ERR_FS_INVALID_SYMLINK_TYPE',
+  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);
+assert.throws(() => fs.symlinkSync('', '', '🍏'), errObj);
 
 process.on('exit', function() {
   assert.notStrictEqual(linkTime, fileTime);
diff --git a/test/parallel/test-fs-truncate.js b/test/parallel/test-fs-truncate.js
index fca491de4a..bb6b4bc8b5 100644
--- a/test/parallel/test-fs-truncate.js
+++ b/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}`
       }
     );
   });
diff --git a/test/parallel/test-fs-utimes.js b/test/parallel/test-fs-utimes.js
index be2d1d24df..bad9067864 100644
--- a/test/parallel/test-fs-utimes.js
+++ b/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',
-        type: TypeError
+        code: 'ERR_OUT_OF_RANGE',
+        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',
-            type: TypeError,
+            code: 'ERR_OUT_OF_RANGE',
+            type: RangeError,
+            message: 'The value of "fd" is out of range. ' +
+                    'It must be >= 0 && < 4294967296. Received -1'
           }
         );