From c5b87f184ba3d05a20ae88663da66e83e06f952e Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sun, 26 Nov 2017 12:44:20 -0800 Subject: [PATCH 01/10] fs: move type checking for fs.close to js --- lib/fs.js | 83 ++++++++++++++++++++++++--- src/node_file.cc | 5 +- test/parallel/test-fs-close-errors.js | 42 ++++++++++++++ 3 files changed, 118 insertions(+), 12 deletions(-) create mode 100644 test/parallel/test-fs-close-errors.js diff --git a/lib/fs.js b/lib/fs.js index a61cfaf4697bdd..5da17d086d6012 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -636,12 +636,22 @@ fs.readFileSync = function(path, options) { }; fs.close = function(fd, callback) { - var req = new FSReqWrap(); + if (!Number.isInteger(fd)) + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'fd', 'number'); + if (fd < 0 || fd > 0xFFFFFFFF) + throw new errors.RangeError('ERR_OUT_OF_RANGE', 'fd'); + + const req = new FSReqWrap(); req.oncomplete = makeCallback(callback); binding.close(fd, req); }; fs.closeSync = function(fd) { + if (!Number.isInteger(fd)) + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'fd', 'number'); + if (fd < 0 || fd > 0xFFFFFFFF) + throw new errors.RangeError('ERR_OUT_OF_RANGE', 'fd'); + return binding.close(fd); }; @@ -854,7 +864,14 @@ fs.ftruncate = function(fd, len, callback) { } else if (len === undefined) { len = 0; } - var req = new FSReqWrap(); + if (typeof fd !== 'number') + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'fd', 'number'); + if (fd < 0 || fd > 0xFFFFFFFF) + throw new errors.RangeError('ERR_OUT_OF_RANGE', 'fd'); + if (typeof len !== 'number') + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'len', 'number'); + len = Math.max(0, len); + const req = new FSReqWrap(); req.oncomplete = makeCallback(callback); binding.ftruncate(fd, len, req); }; @@ -863,6 +880,13 @@ fs.ftruncateSync = function(fd, len) { if (len === undefined) { len = 0; } + if (typeof fd !== 'number') + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'fd', 'number'); + if (fd < 0 || fd > 0xFFFFFFFF) + throw new errors.RangeError('ERR_OUT_OF_RANGE', 'fd'); + if (typeof len !== 'number') + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'len', 'number'); + len = Math.max(0, len); return binding.ftruncate(fd, len); }; @@ -883,22 +907,38 @@ fs.rmdirSync = function(path) { }; fs.fdatasync = function(fd, callback) { - var req = new FSReqWrap(); + if (typeof fd !== 'number') + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'fd', 'number'); + if (fd < 0 || fd > 0xFFFFFFFF) + throw new errors.RangeError('ERR_OUT_OF_RANGE', 'fd'); + const req = new FSReqWrap(); req.oncomplete = makeCallback(callback); binding.fdatasync(fd, req); }; fs.fdatasyncSync = function(fd) { + if (typeof fd !== 'number') + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'fd', 'number'); + if (fd < 0 || fd > 0xFFFFFFFF) + throw new errors.RangeError('ERR_OUT_OF_RANGE', 'fd'); return binding.fdatasync(fd); }; fs.fsync = function(fd, callback) { - var req = new FSReqWrap(); + if (typeof fd !== 'number') + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'fd', 'number'); + if (fd < 0 || fd > 0xFFFFFFFF) + throw new errors.RangeError('ERR_OUT_OF_RANGE', 'fd'); + const req = new FSReqWrap(); req.oncomplete = makeCallback(callback); binding.fsync(fd, req); }; fs.fsyncSync = function(fd) { + if (typeof fd !== 'number') + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'fd', 'number'); + if (fd < 0 || fd > 0xFFFFFFFF) + throw new errors.RangeError('ERR_OUT_OF_RANGE', 'fd'); return binding.fsync(fd); }; @@ -941,7 +981,11 @@ fs.readdirSync = function(path, options) { }; fs.fstat = function(fd, callback) { - var req = new FSReqWrap(); + if (typeof fd !== 'number') + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'fd', 'number'); + if (fd < 0 || fd > 0xFFFFFFFF) + throw new errors.RangeError('ERR_OUT_OF_RANGE', 'fd'); + const req = new FSReqWrap(); req.oncomplete = makeStatsCallback(callback); binding.fstat(fd, req); }; @@ -967,6 +1011,10 @@ fs.stat = function(path, callback) { }; fs.fstatSync = function(fd) { + if (typeof fd !== 'number') + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'fd', 'number'); + if (fd < 0 || fd > 0xFFFFFFFF) + throw new errors.RangeError('ERR_OUT_OF_RANGE', 'fd'); binding.fstat(fd); return statsFromValues(); }; @@ -1098,13 +1146,32 @@ fs.unlinkSync = function(path) { }; fs.fchmod = function(fd, mode, callback) { - var req = new FSReqWrap(); + mode = modeNum(mode); + if (typeof fd !== 'number') + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'fd', 'number'); + if (fd < 0 || fd > 0xFFFFFFFF) + throw new errors.RangeError('ERR_OUT_OF_RANGE', 'fd'); + if (typeof mode !== 'number') + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'mode', 'number'); + if (mode < 0 || mode > 0o777) + throw new errors.RangeError('ERR_OUT_OF_RANGE', 'mode'); + + const req = new FSReqWrap(); req.oncomplete = makeCallback(callback); - binding.fchmod(fd, modeNum(mode), req); + binding.fchmod(fd, mode, req); }; fs.fchmodSync = function(fd, mode) { - return binding.fchmod(fd, modeNum(mode)); + mode = modeNum(mode); + if (typeof fd !== 'number') + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'fd', 'number'); + if (fd < 0 || fd > 0xFFFFFFFF) + throw new errors.RangeError('ERR_OUT_OF_RANGE', 'fd'); + if (typeof mode !== 'number') + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'mode', 'number'); + if (mode < 0 || mode > 0o777) + throw new errors.RangeError('ERR_OUT_OF_RANGE', 'mode'); + return binding.fchmod(fd, mode); }; if (constants.O_SYMLINK !== undefined) { diff --git a/src/node_file.cc b/src/node_file.cc index 2b3b007976de14..82baff093e27d2 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -464,10 +464,7 @@ void Access(const FunctionCallbackInfo& args) { void Close(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - if (args.Length() < 1) - return TYPE_ERROR("fd is required"); - if (!args[0]->IsInt32()) - return TYPE_ERROR("fd must be a file descriptor"); + CHECK(args[0]->IsInt32()); int fd = args[0]->Int32Value(); diff --git a/test/parallel/test-fs-close-errors.js b/test/parallel/test-fs-close-errors.js new file mode 100644 index 00000000000000..040f6def447b4d --- /dev/null +++ b/test/parallel/test-fs-close-errors.js @@ -0,0 +1,42 @@ +'use strict'; + +const common = require('../common'); +const fs = require('fs'); + +['', false, null, undefined, {}, []].forEach((i) => { + common.expectsError( + () => fs.close(i), + { + code: 'ERR_INVALID_ARG_TYPE', + type: TypeError, + message: 'The "fd" argument must be of type number' + } + ); + common.expectsError( + () => fs.closeSync(i), + { + code: 'ERR_INVALID_ARG_TYPE', + type: TypeError, + message: 'The "fd" argument must be of type number' + } + ); +}); + +[-1, 0xFFFFFFFF + 1].forEach((i) => { + common.expectsError( + () => fs.close(i), + { + code: 'ERR_OUT_OF_RANGE', + type: RangeError, + message: 'The "fd" argument is out of range' + } + ); + common.expectsError( + () => fs.closeSync(i), + { + code: 'ERR_OUT_OF_RANGE', + type: RangeError, + message: 'The "fd" argument is out of range' + } + ); +}); From 7fe6d1137a810a48a250e1b89dd955cc93adc2ac Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sun, 26 Nov 2017 13:18:54 -0800 Subject: [PATCH 02/10] fs: move type checking on fs.fstat to js --- lib/fs.js | 4 ++-- src/node_file.cc | 5 +---- test/parallel/test-fs-stat.js | 38 +++++++++++++++++++++++++++++++++++ 3 files changed, 41 insertions(+), 6 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 5da17d086d6012..d1e8860e7946d0 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -981,7 +981,7 @@ fs.readdirSync = function(path, options) { }; fs.fstat = function(fd, callback) { - if (typeof fd !== 'number') + if (!Number.isInteger(fd)) throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'fd', 'number'); if (fd < 0 || fd > 0xFFFFFFFF) throw new errors.RangeError('ERR_OUT_OF_RANGE', 'fd'); @@ -1011,7 +1011,7 @@ fs.stat = function(path, callback) { }; fs.fstatSync = function(fd) { - if (typeof fd !== 'number') + if (!Number.isInteger(fd)) throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'fd', 'number'); if (fd < 0 || fd > 0xFFFFFFFF) throw new errors.RangeError('ERR_OUT_OF_RANGE', 'fd'); diff --git a/src/node_file.cc b/src/node_file.cc index 82baff093e27d2..19b9a1163de620 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -634,10 +634,7 @@ static void LStat(const FunctionCallbackInfo& args) { static void FStat(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - if (args.Length() < 1) - return TYPE_ERROR("fd is required"); - if (!args[0]->IsInt32()) - return TYPE_ERROR("fd must be a file descriptor"); + CHECK(args[0]->IsInt32()); int fd = args[0]->Int32Value(); diff --git a/test/parallel/test-fs-stat.js b/test/parallel/test-fs-stat.js index 332a26e9bff2e8..57fccc15869fe2 100644 --- a/test/parallel/test-fs-stat.js +++ b/test/parallel/test-fs-stat.js @@ -130,3 +130,41 @@ fs.stat(__filename, common.mustCall(function(err, s) { assert.strictEqual(typeof parsed[k], 'string', `${k} should be a string`); }); })); + +['', false, null, undefined, {}, []].forEach((i) => { + common.expectsError( + () => fs.fstat(i), + { + code: 'ERR_INVALID_ARG_TYPE', + type: TypeError, + message: 'The "fd" argument must be of type number' + } + ); + common.expectsError( + () => fs.fstatSync(i), + { + code: 'ERR_INVALID_ARG_TYPE', + type: TypeError, + message: 'The "fd" argument must be of type number' + } + ); +}); + +[-1, 0xFFFFFFFF + 1].forEach((i) => { + common.expectsError( + () => fs.fstat(i), + { + code: 'ERR_OUT_OF_RANGE', + type: RangeError, + message: 'The "fd" argument is out of range' + } + ); + common.expectsError( + () => fs.fstatSync(i), + { + code: 'ERR_OUT_OF_RANGE', + type: RangeError, + message: 'The "fd" argument is out of range' + } + ); +}); From 19de31525a9a5a65c2f75261666eef403d317c02 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sun, 26 Nov 2017 13:35:06 -0800 Subject: [PATCH 03/10] fs: move type checking for fs.fdatasync to js --- lib/fs.js | 4 ++-- src/node_file.cc | 5 +---- test/parallel/test-fs-fsync.js | 38 ++++++++++++++++++++++++++++++++++ 3 files changed, 41 insertions(+), 6 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index d1e8860e7946d0..3a7ee7d9d7652c 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -907,7 +907,7 @@ fs.rmdirSync = function(path) { }; fs.fdatasync = function(fd, callback) { - if (typeof fd !== 'number') + if (!Number.isInteger(fd)) throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'fd', 'number'); if (fd < 0 || fd > 0xFFFFFFFF) throw new errors.RangeError('ERR_OUT_OF_RANGE', 'fd'); @@ -917,7 +917,7 @@ fs.fdatasync = function(fd, callback) { }; fs.fdatasyncSync = function(fd) { - if (typeof fd !== 'number') + if (!Number.isInteger(fd)) throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'fd', 'number'); if (fd < 0 || fd > 0xFFFFFFFF) throw new errors.RangeError('ERR_OUT_OF_RANGE', 'fd'); diff --git a/src/node_file.cc b/src/node_file.cc index 19b9a1163de620..926870ab9c3fde 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -792,10 +792,7 @@ static void FTruncate(const FunctionCallbackInfo& args) { static void Fdatasync(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - if (args.Length() < 1) - return TYPE_ERROR("fd is required"); - if (!args[0]->IsInt32()) - return TYPE_ERROR("fd must be a file descriptor"); + CHECK(args[0]->IsInt32()); int fd = args[0]->Int32Value(); diff --git a/test/parallel/test-fs-fsync.js b/test/parallel/test-fs-fsync.js index 3f43dd75450517..1a9d1d97f8fee1 100644 --- a/test/parallel/test-fs-fsync.js +++ b/test/parallel/test-fs-fsync.js @@ -48,3 +48,41 @@ fs.open(fileFixture, '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 number' + } + ); + common.expectsError( + () => fs.fdatasyncSync(i), + { + code: 'ERR_INVALID_ARG_TYPE', + type: TypeError, + message: 'The "fd" argument must be of type number' + } + ); +}); + +[-1, 0xFFFFFFFF + 1].forEach((i) => { + common.expectsError( + () => fs.fdatasync(i), + { + code: 'ERR_OUT_OF_RANGE', + type: RangeError, + message: 'The "fd" argument is out of range' + } + ); + common.expectsError( + () => fs.fdatasyncSync(i), + { + code: 'ERR_OUT_OF_RANGE', + type: RangeError, + message: 'The "fd" argument is out of range' + } + ); +}); From d308dec31006305ed341897710562c616b124370 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sun, 26 Nov 2017 13:38:58 -0800 Subject: [PATCH 04/10] fs: move type checking for fs.sync to js --- lib/fs.js | 4 ++-- src/node_file.cc | 5 +---- test/parallel/test-fs-fsync.js | 20 ++++++++++++++++++-- 3 files changed, 21 insertions(+), 8 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 3a7ee7d9d7652c..1430c04217932d 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -925,7 +925,7 @@ fs.fdatasyncSync = function(fd) { }; fs.fsync = function(fd, callback) { - if (typeof fd !== 'number') + if (!Number.isInteger(fd)) throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'fd', 'number'); if (fd < 0 || fd > 0xFFFFFFFF) throw new errors.RangeError('ERR_OUT_OF_RANGE', 'fd'); @@ -935,7 +935,7 @@ fs.fsync = function(fd, callback) { }; fs.fsyncSync = function(fd) { - if (typeof fd !== 'number') + if (!Number.isInteger(fd)) throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'fd', 'number'); if (fd < 0 || fd > 0xFFFFFFFF) throw new errors.RangeError('ERR_OUT_OF_RANGE', 'fd'); diff --git a/src/node_file.cc b/src/node_file.cc index 926870ab9c3fde..1104b10ad88de5 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -806,10 +806,7 @@ static void Fdatasync(const FunctionCallbackInfo& args) { static void Fsync(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - if (args.Length() < 1) - return TYPE_ERROR("fd is required"); - if (!args[0]->IsInt32()) - return TYPE_ERROR("fd must be a file descriptor"); + CHECK(args[0]->IsInt32()); int fd = args[0]->Int32Value(); diff --git a/test/parallel/test-fs-fsync.js b/test/parallel/test-fs-fsync.js index 1a9d1d97f8fee1..e1ec8ddcc0fe14 100644 --- a/test/parallel/test-fs-fsync.js +++ b/test/parallel/test-fs-fsync.js @@ -66,11 +66,27 @@ fs.open(fileFixture, 'a', 0o777, common.mustCall(function(err, fd) { message: 'The "fd" argument must be of type number' } ); + common.expectsError( + () => fs.fsync(i), + { + code: 'ERR_INVALID_ARG_TYPE', + type: TypeError, + message: 'The "fd" argument must be of type number' + } + ); + common.expectsError( + () => fs.fsyncSync(i), + { + code: 'ERR_INVALID_ARG_TYPE', + type: TypeError, + message: 'The "fd" argument must be of type number' + } + ); }); [-1, 0xFFFFFFFF + 1].forEach((i) => { common.expectsError( - () => fs.fdatasync(i), + () => fs.fsync(i), { code: 'ERR_OUT_OF_RANGE', type: RangeError, @@ -78,7 +94,7 @@ fs.open(fileFixture, 'a', 0o777, common.mustCall(function(err, fd) { } ); common.expectsError( - () => fs.fdatasyncSync(i), + () => fs.fsyncSync(i), { code: 'ERR_OUT_OF_RANGE', type: RangeError, From ca84f5c964a9884a352261e66c9c6700265dc1a8 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sun, 26 Nov 2017 14:06:50 -0800 Subject: [PATCH 05/10] fs: move type checking for fs.ftruncate to js --- lib/fs.js | 8 ++-- src/node_file.cc | 19 ++-------- test/parallel/test-fs-truncate.js | 61 +++++++++++++++++++++++++++++++ 3 files changed, 68 insertions(+), 20 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 1430c04217932d..6e86e1124ffa0d 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -864,11 +864,11 @@ fs.ftruncate = function(fd, len, callback) { } else if (len === undefined) { len = 0; } - if (typeof fd !== 'number') + if (!Number.isInteger(fd)) throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'fd', 'number'); if (fd < 0 || fd > 0xFFFFFFFF) throw new errors.RangeError('ERR_OUT_OF_RANGE', 'fd'); - if (typeof len !== 'number') + if (!Number.isInteger(len)) throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'len', 'number'); len = Math.max(0, len); const req = new FSReqWrap(); @@ -880,11 +880,11 @@ fs.ftruncateSync = function(fd, len) { if (len === undefined) { len = 0; } - if (typeof fd !== 'number') + if (!Number.isInteger(fd)) throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'fd', 'number'); if (fd < 0 || fd > 0xFFFFFFFF) throw new errors.RangeError('ERR_OUT_OF_RANGE', 'fd'); - if (typeof len !== 'number') + if (!Number.isInteger(len)) throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'len', 'number'); len = Math.max(0, len); return binding.ftruncate(fd, len); diff --git a/src/node_file.cc b/src/node_file.cc index 1104b10ad88de5..b25b629bf6ed44 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -763,24 +763,11 @@ static void Rename(const FunctionCallbackInfo& args) { static void FTruncate(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - if (args.Length() < 2) - return TYPE_ERROR("fd and length are required"); - if (!args[0]->IsInt32()) - return TYPE_ERROR("fd must be a file descriptor"); + CHECK(args[0]->IsInt32()); + CHECK(args[1]->IsNumber()); int fd = args[0]->Int32Value(); - - // FIXME(bnoordhuis) It's questionable to reject non-ints here but still - // allow implicit coercion from null or undefined to zero. Probably best - // handled in lib/fs.js. - Local len_v(args[1]); - if (!len_v->IsUndefined() && - !len_v->IsNull() && - !IsInt64(len_v->NumberValue())) { - return env->ThrowTypeError("Not an integer"); - } - - const int64_t len = len_v->IntegerValue(); + const int64_t len = args[1]->IntegerValue(); if (args[2]->IsObject()) { ASYNC_CALL(ftruncate, args[2], UTF8, fd, len) diff --git a/test/parallel/test-fs-truncate.js b/test/parallel/test-fs-truncate.js index 4119d53c4f8d95..7690ec7c24d60e 100644 --- a/test/parallel/test-fs-truncate.js +++ b/test/parallel/test-fs-truncate.js @@ -180,8 +180,69 @@ function testFtruncate(cb) { fs.writeFileSync(file5, 'Hi'); const fd = fs.openSync(file5, 'r+'); process.on('exit', () => fs.closeSync(fd)); + + ['', false, null, {}, []].forEach((i) => { + common.expectsError( + () => fs.ftruncate(fd, i), + { + code: 'ERR_INVALID_ARG_TYPE', + type: TypeError, + message: 'The "len" argument must be of type number' + } + ); + }); + fs.ftruncate(fd, undefined, common.mustCall(function(err) { assert.ifError(err); assert(fs.readFileSync(file5).equals(Buffer.from(''))); })); } + +{ + const file6 = path.resolve(tmp, 'truncate-file-6.txt'); + fs.writeFileSync(file6, 'Hi'); + const fd = fs.openSync(file6, 'r+'); + process.on('exit', () => fs.closeSync(fd)); + fs.ftruncate(fd, -1, common.mustCall(function(err) { + assert.ifError(err); + assert(fs.readFileSync(file6).equals(Buffer.from(''))); + })); +} + +['', false, null, undefined, {}, []].forEach((i) => { + common.expectsError( + () => fs.ftruncate(i), + { + code: 'ERR_INVALID_ARG_TYPE', + type: TypeError, + message: 'The "fd" argument must be of type number' + } + ); + common.expectsError( + () => fs.ftruncateSync(i), + { + code: 'ERR_INVALID_ARG_TYPE', + type: TypeError, + message: 'The "fd" argument must be of type number' + } + ); +}); + +[-1, 0xFFFFFFFF + 1].forEach((i) => { + common.expectsError( + () => fs.ftruncate(i), + { + code: 'ERR_OUT_OF_RANGE', + type: RangeError, + message: 'The "fd" argument is out of range' + } + ); + common.expectsError( + () => fs.ftruncateSync(i), + { + code: 'ERR_OUT_OF_RANGE', + type: RangeError, + message: 'The "fd" argument is out of range' + } + ); +}); From 8b03b46977018d75e713e449997df3adb955c0f9 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sun, 26 Nov 2017 14:27:15 -0800 Subject: [PATCH 06/10] fs: move type checking for fs.fchmod to js --- lib/fs.js | 8 +++--- src/node_file.cc | 8 ++---- test/parallel/test-fs-chmod.js | 46 ++++++++++++++++++++++++++++++++++ 3 files changed, 52 insertions(+), 10 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 6e86e1124ffa0d..8c6793f38b2114 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -1147,11 +1147,11 @@ fs.unlinkSync = function(path) { fs.fchmod = function(fd, mode, callback) { mode = modeNum(mode); - if (typeof fd !== 'number') + if (!Number.isInteger(fd)) throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'fd', 'number'); if (fd < 0 || fd > 0xFFFFFFFF) throw new errors.RangeError('ERR_OUT_OF_RANGE', 'fd'); - if (typeof mode !== 'number') + if (!Number.isInteger(mode)) throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'mode', 'number'); if (mode < 0 || mode > 0o777) throw new errors.RangeError('ERR_OUT_OF_RANGE', 'mode'); @@ -1163,11 +1163,11 @@ fs.fchmod = function(fd, mode, callback) { fs.fchmodSync = function(fd, mode) { mode = modeNum(mode); - if (typeof fd !== 'number') + if (!Number.isInteger(fd)) throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'fd', 'number'); if (fd < 0 || fd > 0xFFFFFFFF) throw new errors.RangeError('ERR_OUT_OF_RANGE', 'fd'); - if (typeof mode !== 'number') + if (!Number.isInteger(mode)) throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'mode', 'number'); if (mode < 0 || mode > 0o777) throw new errors.RangeError('ERR_OUT_OF_RANGE', 'mode'); diff --git a/src/node_file.cc b/src/node_file.cc index b25b629bf6ed44..c02562d76ce8a5 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -1254,12 +1254,8 @@ static void Chmod(const FunctionCallbackInfo& args) { static void FChmod(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - if (args.Length() < 2) - return TYPE_ERROR("fd and mode are required"); - if (!args[0]->IsInt32()) - return TYPE_ERROR("fd must be a file descriptor"); - if (!args[1]->IsInt32()) - return TYPE_ERROR("mode must be an integer"); + CHECK(args[0]->IsInt32()); + CHECK(args[1]->IsInt32()); int fd = args[0]->Int32Value(); int mode = static_cast(args[1]->Int32Value()); diff --git a/test/parallel/test-fs-chmod.js b/test/parallel/test-fs-chmod.js index 7d4b7a10dbd9b4..9eae75c3c71bcd 100644 --- a/test/parallel/test-fs-chmod.js +++ b/test/parallel/test-fs-chmod.js @@ -108,6 +108,15 @@ fs.open(file2, 'w', common.mustCall((err, fd) => { assert.strictEqual(mode_async, fs.fstatSync(fd).mode & 0o777); } + common.expectsError( + () => fs.fchmod(fd, {}), + { + code: 'ERR_INVALID_ARG_TYPE', + type: TypeError, + message: 'The "mode" argument must be of type number' + } + ); + fs.fchmodSync(fd, mode_sync); if (common.isWindows) { assert.ok((fs.fstatSync(fd).mode & 0o777) & mode_sync); @@ -136,6 +145,43 @@ if (fs.lchmod) { })); } +['', false, null, undefined, {}, []].forEach((i) => { + common.expectsError( + () => fs.fchmod(i, 0o000), + { + code: 'ERR_INVALID_ARG_TYPE', + type: TypeError, + message: 'The "fd" argument must be of type number' + } + ); + common.expectsError( + () => fs.fchmodSync(i, 0o000), + { + code: 'ERR_INVALID_ARG_TYPE', + type: TypeError, + message: 'The "fd" argument must be of type number' + } + ); +}); + +[-1, 0xFFFFFFFF + 1].forEach((i) => { + common.expectsError( + () => fs.fchmod(i, 0o000), + { + code: 'ERR_OUT_OF_RANGE', + type: RangeError, + message: 'The "fd" argument is out of range' + } + ); + common.expectsError( + () => fs.fchmodSync(i, 0o000), + { + code: 'ERR_OUT_OF_RANGE', + type: RangeError, + message: 'The "fd" argument is out of range' + } + ); +}); process.on('exit', function() { assert.strictEqual(0, openCount); From 37e9cb56d4002269dfd72e9f826aaa209f484412 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sun, 26 Nov 2017 16:53:33 -0800 Subject: [PATCH 07/10] fs: move type checking for fs.fchown to js --- lib/fs.js | 28 +++++++- src/node_file.cc | 16 +---- test/parallel/test-fs-fchown.js | 110 ++++++++++++++++++++++++++++++++ 3 files changed, 140 insertions(+), 14 deletions(-) create mode 100644 test/parallel/test-fs-fchown.js diff --git a/lib/fs.js b/lib/fs.js index 8c6793f38b2114..09e5b457ada9f3 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -1249,12 +1249,38 @@ if (constants.O_SYMLINK !== undefined) { } fs.fchown = function(fd, uid, gid, callback) { - var req = new FSReqWrap(); + if (!Number.isInteger(fd)) + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'fd', 'number'); + if (fd < 0 || fd > 0xFFFFFFFF) + throw new errors.RangeError('ERR_OUT_OF_RANGE', 'fd'); + if (!Number.isInteger(uid)) + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'uid', 'number'); + if (uid < 0) + throw new errors.RangeError('ERR_OUT_OF_RANGE', 'uid'); + if (!Number.isInteger(gid)) + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'gid', 'number'); + if (gid < 0) + throw new errors.RangeError('ERR_OUT_OF_RANGE', 'gid'); + + const req = new FSReqWrap(); req.oncomplete = makeCallback(callback); binding.fchown(fd, uid, gid, req); }; fs.fchownSync = function(fd, uid, gid) { + if (!Number.isInteger(fd)) + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'fd', 'number'); + if (fd < 0 || fd > 0xFFFFFFFF) + throw new errors.RangeError('ERR_OUT_OF_RANGE', 'fd'); + if (!Number.isInteger(uid)) + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'uid', 'number'); + if (uid < 0) + throw new errors.RangeError('ERR_OUT_OF_RANGE', 'uid'); + if (!Number.isInteger(gid)) + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'gid', 'number'); + if (gid < 0) + throw new errors.RangeError('ERR_OUT_OF_RANGE', 'gid'); + return binding.fchown(fd, uid, gid); }; diff --git a/src/node_file.cc b/src/node_file.cc index c02562d76ce8a5..c85206fee19700 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -1306,19 +1306,9 @@ static void Chown(const FunctionCallbackInfo& args) { static void FChown(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - int len = args.Length(); - if (len < 1) - return TYPE_ERROR("fd required"); - if (len < 2) - return TYPE_ERROR("uid required"); - if (len < 3) - return TYPE_ERROR("gid required"); - if (!args[0]->IsInt32()) - return TYPE_ERROR("fd must be an int"); - if (!args[1]->IsUint32()) - return TYPE_ERROR("uid must be an unsigned int"); - if (!args[2]->IsUint32()) - return TYPE_ERROR("gid must be an unsigned int"); + CHECK(args[0]->IsInt32()); + CHECK(args[1]->IsUint32()); + CHECK(args[2]->IsUint32()); int fd = args[0]->Int32Value(); uv_uid_t uid = static_cast(args[1]->Uint32Value()); diff --git a/test/parallel/test-fs-fchown.js b/test/parallel/test-fs-fchown.js new file mode 100644 index 00000000000000..2e75c6b909bfbc --- /dev/null +++ b/test/parallel/test-fs-fchown.js @@ -0,0 +1,110 @@ +'use strict'; + +const common = require('../common'); +const fs = require('fs'); + +['', false, null, undefined, {}, []].forEach((i) => { + common.expectsError( + () => fs.fchown(i), + { + code: 'ERR_INVALID_ARG_TYPE', + type: TypeError, + message: 'The "fd" argument must be of type number' + } + ); + common.expectsError( + () => fs.fchownSync(i), + { + code: 'ERR_INVALID_ARG_TYPE', + type: TypeError, + message: 'The "fd" argument must be of type number' + } + ); + + common.expectsError( + () => fs.fchown(1, i), + { + code: 'ERR_INVALID_ARG_TYPE', + type: TypeError, + message: 'The "uid" argument must be of type number' + } + ); + common.expectsError( + () => fs.fchownSync(1, i), + { + code: 'ERR_INVALID_ARG_TYPE', + type: TypeError, + message: 'The "uid" argument must be of type number' + } + ); + + common.expectsError( + () => fs.fchown(1, 1, i), + { + code: 'ERR_INVALID_ARG_TYPE', + type: TypeError, + message: 'The "gid" argument must be of type number' + } + ); + common.expectsError( + () => fs.fchownSync(1, 1, i), + { + code: 'ERR_INVALID_ARG_TYPE', + type: TypeError, + message: 'The "gid" argument must be of type number' + } + ); +}); + +[-1, 0xFFFFFFFF + 1].forEach((i) => { + common.expectsError( + () => fs.fchown(i), + { + code: 'ERR_OUT_OF_RANGE', + type: RangeError, + message: 'The "fd" argument is out of range' + } + ); + common.expectsError( + () => fs.fchownSync(i), + { + code: 'ERR_OUT_OF_RANGE', + type: RangeError, + message: 'The "fd" argument is out of range' + } + ); +}); + +common.expectsError( + () => fs.fchown(1, -1), + { + code: 'ERR_OUT_OF_RANGE', + type: RangeError, + message: 'The "uid" argument is out of range' + } +); +common.expectsError( + () => fs.fchownSync(1, -1), + { + code: 'ERR_OUT_OF_RANGE', + type: RangeError, + message: 'The "uid" argument is out of range' + } +); + +common.expectsError( + () => fs.fchown(1, 1, -1), + { + code: 'ERR_OUT_OF_RANGE', + type: RangeError, + message: 'The "gid" argument is out of range' + } +); +common.expectsError( + () => fs.fchownSync(1, 1, -1), + { + code: 'ERR_OUT_OF_RANGE', + type: RangeError, + message: 'The "gid" argument is out of range' + } +); From 867d2759e08de102dcee10d7480bc674e4dd28ef Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sun, 26 Nov 2017 17:05:26 -0800 Subject: [PATCH 08/10] fs: move type checking in fs.futimes to js --- lib/fs.js | 28 +++++++++++++++++---------- src/node_file.cc | 16 +++------------- test/parallel/test-fs-utimes.js | 34 ++++++++++++++++++++------------- 3 files changed, 42 insertions(+), 36 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 09e5b457ada9f3..8f06de6cf327ec 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -1301,7 +1301,7 @@ fs.chownSync = function(path, uid, gid) { }; // converts Date or number to a fractional UNIX timestamp -function toUnixTimestamp(time) { +function toUnixTimestamp(time, name = 'time') { // eslint-disable-next-line eqeqeq if (typeof time === 'string' && +time == time) { return +time; @@ -1316,10 +1316,10 @@ function toUnixTimestamp(time) { // convert to 123.456 UNIX timestamp return time.getTime() / 1000; } - throw new errors.Error('ERR_INVALID_ARG_TYPE', - 'time', - ['Date', 'Time in seconds'], - time); + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', + name, + ['Date', 'Time in seconds'], + time); } // exported for unit tests, not for public consumption @@ -1347,16 +1347,24 @@ fs.utimesSync = function(path, atime, mtime) { }; fs.futimes = function(fd, atime, mtime, callback) { - atime = toUnixTimestamp(atime); - mtime = toUnixTimestamp(mtime); - var req = new FSReqWrap(); + if (!Number.isInteger(fd)) + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'fd', 'number'); + if (fd < 0 || fd > 0xFFFFFFFF) + throw new errors.RangeError('ERR_OUT_OF_RANGE', 'fd'); + atime = toUnixTimestamp(atime, 'atime'); + mtime = toUnixTimestamp(mtime, 'mtime'); + const req = new FSReqWrap(); req.oncomplete = makeCallback(callback); binding.futimes(fd, atime, mtime, req); }; fs.futimesSync = function(fd, atime, mtime) { - atime = toUnixTimestamp(atime); - mtime = toUnixTimestamp(mtime); + if (!Number.isInteger(fd)) + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'fd', 'number'); + if (fd < 0 || fd > 0xFFFFFFFF) + throw new errors.RangeError('ERR_OUT_OF_RANGE', 'fd'); + atime = toUnixTimestamp(atime, 'atime'); + mtime = toUnixTimestamp(mtime, 'mtime'); binding.futimes(fd, atime, mtime); }; diff --git a/src/node_file.cc b/src/node_file.cc index c85206fee19700..cd913b88882e7a 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -1353,19 +1353,9 @@ static void UTimes(const FunctionCallbackInfo& args) { static void FUTimes(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - int len = args.Length(); - if (len < 1) - return TYPE_ERROR("fd required"); - if (len < 2) - return TYPE_ERROR("atime required"); - if (len < 3) - return TYPE_ERROR("mtime required"); - if (!args[0]->IsInt32()) - return TYPE_ERROR("fd must be an int"); - if (!args[1]->IsNumber()) - return TYPE_ERROR("atime must be a number"); - if (!args[2]->IsNumber()) - return TYPE_ERROR("mtime must be a number"); + CHECK(args[0]->IsInt32()); + CHECK(args[1]->IsNumber()); + CHECK(args[2]->IsNumber()); const int fd = args[0]->Int32Value(); const double atime = static_cast(args[1]->NumberValue()); diff --git a/test/parallel/test-fs-utimes.js b/test/parallel/test-fs-utimes.js index 9bcf6039cd60bc..c9455486836c9c 100644 --- a/test/parallel/test-fs-utimes.js +++ b/test/parallel/test-fs-utimes.js @@ -97,12 +97,14 @@ function testIt(atime, mtime, callback) { tests_run++; err = undefined; - try { - fs.futimesSync(-1, atime, mtime); - } catch (ex) { - err = ex; - } - expect_errno('futimesSync', -1, err, 'EBADF'); + common.expectsError( + () => fs.futimesSync(-1, atime, mtime), + { + code: 'ERR_OUT_OF_RANGE', + type: RangeError, + message: 'The "fd" argument is out of range' + } + ); tests_run++; } @@ -125,11 +127,17 @@ function testIt(atime, mtime, callback) { fs.futimes(fd, atime, mtime, common.mustCall(function(err) { expect_ok('futimes', fd, err, atime, mtime); - fs.futimes(-1, atime, mtime, common.mustCall(function(err) { - expect_errno('futimes', -1, err, 'EBADF'); - syncTests(); - callback(); - })); + common.expectsError( + () => fs.futimes(-1, atime, mtime, common.mustNotCall()), + { + code: 'ERR_OUT_OF_RANGE', + type: RangeError, + message: 'The "fd" argument is out of range' + } + ); + + syncTests(); + tests_run++; })); tests_run++; @@ -142,7 +150,7 @@ function testIt(atime, mtime, callback) { const stats = fs.statSync(common.tmpDir); // run tests -const runTest = common.mustCall(testIt, 6); +const runTest = common.mustCall(testIt, 1); runTest(new Date('1982-09-10 13:37'), new Date('1982-09-10 13:37'), function() { runTest(new Date(), new Date(), function() { @@ -163,7 +171,7 @@ runTest(new Date('1982-09-10 13:37'), new Date('1982-09-10 13:37'), function() { }); process.on('exit', function() { - assert.strictEqual(tests_ok, tests_run); + assert.strictEqual(tests_ok, tests_run - 2); }); From 458ab4458eabcf21c2abd9ec12daad9544c32e4a Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sun, 26 Nov 2017 20:51:01 -0800 Subject: [PATCH 09/10] fs: move type checking for fs.read to js --- lib/fs.js | 42 +++++++++++++++++++++++++++++- src/node_file.cc | 15 +++-------- test/parallel/test-fs-read-type.js | 27 +++++++++++-------- 3 files changed, 61 insertions(+), 23 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 8f06de6cf327ec..a076ebc370fd71 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -691,18 +691,38 @@ fs.openSync = function(path, flags, mode) { }; fs.read = function(fd, buffer, offset, length, position, callback) { + if (!Number.isInteger(fd)) + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'fd', 'number'); + if (fd < 0 || fd > 0xFFFFFFFF) + throw new errors.RangeError('ERR_OUT_OF_RANGE', 'fd'); + if (!isUint8Array(buffer)) + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'buffer', + ['Buffer', 'Uint8Array']); + + offset |= 0; + length |= 0; + if (length === 0) { return process.nextTick(function() { callback && callback(null, 0, buffer); }); } + if (offset < 0 || offset >= buffer.length) + throw new errors.RangeError('ERR_OUT_OF_RANGE', 'offset'); + + if (length < 0 || offset + length > buffer.length) + throw new errors.RangeError('ERR_OUT_OF_RANGE', 'length'); + + if (!Number.isInteger(position)) + position = -1; + function wrapper(err, bytesRead) { // Retain a reference to buffer so that it can't be GC'ed too soon. callback && callback(err, bytesRead || 0, buffer); } - var req = new FSReqWrap(); + const req = new FSReqWrap(); req.oncomplete = wrapper; binding.read(fd, buffer, offset, length, position, req); @@ -712,10 +732,30 @@ Object.defineProperty(fs.read, internalUtil.customPromisifyArgs, { value: ['bytesRead', 'buffer'], enumerable: false }); fs.readSync = function(fd, buffer, offset, length, position) { + if (!Number.isInteger(fd)) + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'fd', 'number'); + if (fd < 0 || fd > 0xFFFFFFFF) + throw new errors.RangeError('ERR_OUT_OF_RANGE', 'fd'); + if (!isUint8Array(buffer)) + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'buffer', + ['Buffer', 'Uint8Array']); + + offset |= 0; + length |= 0; + if (length === 0) { return 0; } + if (offset < 0 || offset >= buffer.length) + throw new errors.RangeError('ERR_OUT_OF_RANGE', 'offset'); + + if (length < 0 || offset + length > buffer.length) + throw new errors.RangeError('ERR_OUT_OF_RANGE', 'length'); + + if (!Number.isInteger(position)) + position = -1; + return binding.read(fd, buffer, offset, length, position); }; diff --git a/src/node_file.cc b/src/node_file.cc index cd913b88882e7a..ffb07080afe782 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -1178,12 +1178,8 @@ static void WriteString(const FunctionCallbackInfo& args) { static void Read(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - if (args.Length() < 2) - return TYPE_ERROR("fd and buffer are required"); - if (!args[0]->IsInt32()) - return TYPE_ERROR("fd must be a file descriptor"); - if (!Buffer::HasInstance(args[1])) - return TYPE_ERROR("Second argument needs to be a buffer"); + CHECK(args[0]->IsInt32()); + CHECK(Buffer::HasInstance(args[1])); int fd = args[0]->Int32Value(); @@ -1199,13 +1195,10 @@ static void Read(const FunctionCallbackInfo& args) { size_t buffer_length = Buffer::Length(buffer_obj); size_t off = args[2]->Int32Value(); - if (off >= buffer_length) { - return env->ThrowError("Offset is out of bounds"); - } + CHECK_LT(off, buffer_length); len = args[3]->Int32Value(); - if (!Buffer::IsWithinBounds(off, len, buffer_length)) - return env->ThrowRangeError("Length extends beyond buffer"); + CHECK(Buffer::IsWithinBounds(off, len, buffer_length)); pos = GET_OFFSET(args[4]); diff --git a/test/parallel/test-fs-read-type.js b/test/parallel/test-fs-read-type.js index 0014affa1f7dde..cbbfe4824c1a0e 100644 --- a/test/parallel/test-fs-read-type.js +++ b/test/parallel/test-fs-read-type.js @@ -1,6 +1,5 @@ 'use strict'; const common = require('../common'); -const assert = require('assert'); const fs = require('fs'); const fixtures = require('../common/fixtures'); @@ -9,14 +8,20 @@ const fd = fs.openSync(filepath, 'r'); const expected = 'xyz\n'; // Error must be thrown with string -assert.throws(() => { - fs.read(fd, - expected.length, - 0, - 'utf-8', - common.mustNotCall()); -}, /^TypeError: Second argument needs to be a buffer$/); +common.expectsError( + () => fs.read(fd, expected.length, 0, 'utf-8', common.mustNotCall()), + { + code: 'ERR_INVALID_ARG_TYPE', + type: TypeError, + message: 'The "buffer" argument must be one of type Buffer or Uint8Array' + } +); -assert.throws(() => { - fs.readSync(fd, expected.length, 0, 'utf-8'); -}, /^TypeError: Second argument needs to be a buffer$/); +common.expectsError( + () => fs.readSync(fd, expected.length, 0, 'utf-8'), + { + code: 'ERR_INVALID_ARG_TYPE', + type: TypeError, + message: 'The "buffer" argument must be one of type Buffer or Uint8Array' + } +); From 4f749173f247ee0a06842ab49c27526b468ede10 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sun, 26 Nov 2017 21:03:49 -0800 Subject: [PATCH 10/10] fs: remove unnecessary throw on fs.mkdtemp The type is already checked in JS. Change to a CHECK --- src/node_file.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/node_file.cc b/src/node_file.cc index ffb07080afe782..5c50227d9a52d4 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -1367,8 +1367,7 @@ static void Mkdtemp(const FunctionCallbackInfo& args) { CHECK_GE(args.Length(), 2); BufferValue tmpl(env->isolate(), args[0]); - if (*tmpl == nullptr) - return TYPE_ERROR("template must be a string or Buffer"); + CHECK_NE(*tmpl, nullptr); const enum encoding encoding = ParseEncoding(env->isolate(), args[1], UTF8);