From acc3c770e7717673ee87fa37076fc50fcb91e4ea Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Mon, 19 Mar 2018 14:17:50 +0100 Subject: [PATCH] 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 Reviewed-By: Matteo Collina Reviewed-By: Joyee Cheung Reviewed-By: Anna Henningsen --- lib/internal/fs.js | 41 ++++++++---- test/parallel/test-fs-chmod.js | 6 +- test/parallel/test-fs-close-errors.js | 3 +- test/parallel/test-fs-fchmod.js | 87 ++++++++++++++++---------- test/parallel/test-fs-fchown.js | 89 ++++++++++++--------------- test/parallel/test-fs-fsync.js | 44 ++++--------- test/parallel/test-fs-read-type.js | 6 +- test/parallel/test-fs-stat.js | 3 +- test/parallel/test-fs-symlink.js | 78 +++++++---------------- test/parallel/test-fs-truncate.js | 6 +- test/parallel/test-fs-utimes.js | 12 ++-- 11 files changed, 178 insertions(+), 197 deletions(-) diff --git a/lib/internal/fs.js b/lib/internal/fs.js index bcbb199ec32e42..4834d92fa0acd7 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 a167fa49bad30c..6727ec2144f2ce 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 3be8b4ce337b13..48af5eb4856d5f 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 22e6a490c9ca5d..edf5cc32ea8dca 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 a7e6bf6cbca571..500c06a47ca09c 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 1f575881e3743d..4d96091f346501 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 b47a7be177e7a3..55049ed79f84eb 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 bbb328118737e7..1003890bb8f459 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 19903fff58c761..ddcf7a63ffbe18 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 fca491de4a23e8..bb6b4bc8b5dec1 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 be2d1d24dfae30..bad9067864955c 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' } );