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: #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>
nodejs · Mar 25, 2018 · acc3c77 · acc3c77
1 parent 058e7fb
commit acc3c77
Show file tree

Hide file tree

Showing 11 changed files with 178 additions and 197 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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}`
       }
     );
   });