From 8974df15a973e97a74cf9fb0ccb45c11baa7b54a Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sun, 26 Nov 2017 12:44:20 -0800 Subject: [PATCH 01/14] fs: move type checking for fs.close to js PR-URL: https://github.com/nodejs/node/pull/17334 Reviewed-By: Refael Ackermann Reviewed-By: Timothy Gu Reviewed-By: Jon Moss Reviewed-By: Anna Henningsen Reviewed-By: Joyee Cheung Reviewed-By: Matteo Collina --- 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 ee19e96f3b7..67c96a7524d 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 e0b31ea5a65..1eda2104b78 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 00000000000..040f6def447 --- /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 639096855ed52958a5bf7ccc0a83cee6a4a24166 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sun, 26 Nov 2017 13:18:54 -0800 Subject: [PATCH 02/14] fs: move type checking on fs.fstat to js PR-URL: https://github.com/nodejs/node/pull/17334 Reviewed-By: Refael Ackermann Reviewed-By: Timothy Gu Reviewed-By: Jon Moss Reviewed-By: Anna Henningsen Reviewed-By: Joyee Cheung Reviewed-By: Matteo Collina --- 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 67c96a7524d..ac0d42c76a2 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 1eda2104b78..72995ca1be7 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 332a26e9bff..57fccc15869 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 956f97b875c9d5193395c70b378c8e1bcedca08e Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sun, 26 Nov 2017 13:35:06 -0800 Subject: [PATCH 03/14] fs: move type checking for fs.fdatasync to js PR-URL: https://github.com/nodejs/node/pull/17334 Reviewed-By: Refael Ackermann Reviewed-By: Timothy Gu Reviewed-By: Jon Moss Reviewed-By: Anna Henningsen Reviewed-By: Joyee Cheung Reviewed-By: Matteo Collina --- 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 ac0d42c76a2..70e0d3d98ca 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 72995ca1be7..7acf6296fc9 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 3f43dd75450..1a9d1d97f8f 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 8cb080c4867bb7b8eefbb2abf34bd97c31f647a1 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sun, 26 Nov 2017 13:38:58 -0800 Subject: [PATCH 04/14] fs: move type checking for fs.sync to js PR-URL: https://github.com/nodejs/node/pull/17334 Reviewed-By: Refael Ackermann Reviewed-By: Timothy Gu Reviewed-By: Jon Moss Reviewed-By: Anna Henningsen Reviewed-By: Joyee Cheung Reviewed-By: Matteo Collina --- 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 70e0d3d98ca..9d9ba706127 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 7acf6296fc9..13155bedb10 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 1a9d1d97f8f..e1ec8ddcc0f 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 d453fac33b0769a12b8b926f0d77e1bbd834b397 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sun, 26 Nov 2017 14:06:50 -0800 Subject: [PATCH 05/14] fs: move type checking for fs.ftruncate to js PR-URL: https://github.com/nodejs/node/pull/17334 Reviewed-By: Refael Ackermann Reviewed-By: Timothy Gu Reviewed-By: Jon Moss Reviewed-By: Anna Henningsen Reviewed-By: Joyee Cheung Reviewed-By: Matteo Collina --- 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 9d9ba706127..1a93d0acf2e 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 13155bedb10..c330c95fa67 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 4119d53c4f8..7690ec7c24d 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 0a01aa8e946ee86199af56ff3eac9bae67235ca8 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sun, 26 Nov 2017 14:27:15 -0800 Subject: [PATCH 06/14] fs: move type checking for fs.fchmod to js PR-URL: https://github.com/nodejs/node/pull/17334 Reviewed-By: Refael Ackermann Reviewed-By: Timothy Gu Reviewed-By: Jon Moss Reviewed-By: Anna Henningsen Reviewed-By: Joyee Cheung Reviewed-By: Matteo Collina --- 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 1a93d0acf2e..c105f5fb3aa 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 c330c95fa67..2acac8c8f8b 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 7d4b7a10dbd..9eae75c3c71 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 82eb459e3f5bab86c6aa75eae41cb51da1c6e2f6 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sun, 26 Nov 2017 16:53:33 -0800 Subject: [PATCH 07/14] fs: move type checking for fs.fchown to js PR-URL: https://github.com/nodejs/node/pull/17334 Reviewed-By: Refael Ackermann Reviewed-By: Timothy Gu Reviewed-By: Jon Moss Reviewed-By: Anna Henningsen Reviewed-By: Joyee Cheung Reviewed-By: Matteo Collina --- 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 c105f5fb3aa..f13488ba0e6 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 2acac8c8f8b..0d90df0e954 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 00000000000..2e75c6b909b --- /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 448ec0b5aaa8b07381a865a4b27bb62529639e48 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sun, 26 Nov 2017 17:05:26 -0800 Subject: [PATCH 08/14] fs: move type checking in fs.futimes to js PR-URL: https://github.com/nodejs/node/pull/17334 Reviewed-By: Refael Ackermann Reviewed-By: Timothy Gu Reviewed-By: Jon Moss Reviewed-By: Anna Henningsen Reviewed-By: Joyee Cheung Reviewed-By: Matteo Collina --- 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 f13488ba0e6..b40d726404e 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 0d90df0e954..e8dd7051ca0 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 9bcf6039cd6..c9455486836 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 163869879e2e29a3210dd2ab40eaa19cceb7bf1e Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sun, 26 Nov 2017 20:51:01 -0800 Subject: [PATCH 09/14] fs: move type checking for fs.read to js PR-URL: https://github.com/nodejs/node/pull/17334 Reviewed-By: Refael Ackermann Reviewed-By: Timothy Gu Reviewed-By: Jon Moss Reviewed-By: Anna Henningsen Reviewed-By: Joyee Cheung Reviewed-By: Matteo Collina --- 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 b40d726404e..9554620a040 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 e8dd7051ca0..0bb5b727999 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 0014affa1f7..cbbfe4824c1 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 805dca199aff69edc1b3ead82365182ad0bf651d Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sun, 26 Nov 2017 21:03:49 -0800 Subject: [PATCH 10/14] fs: remove unnecessary throw on fs.mkdtemp The type is already checked in JS. Change to a CHECK PR-URL: https://github.com/nodejs/node/pull/17334 Reviewed-By: Refael Ackermann Reviewed-By: Timothy Gu Reviewed-By: Jon Moss Reviewed-By: Anna Henningsen Reviewed-By: Joyee Cheung Reviewed-By: Matteo Collina --- 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 0bb5b727999..f146e4ddff6 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); From 6c1a04b381fa97a38ae1faf8763cfa82a20a89db Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Wed, 13 Dec 2017 07:56:19 -0800 Subject: [PATCH 11/14] doc: include Daniel Bevenius as a TSC member PR-URL: https://github.com/nodejs/node/pull/17652 Reviewed-By: Colin Ihrig Reviewed-By: Franziska Hinkelmann Reviewed-By: Anna Henningsen Reviewed-By: Daniel Bevenius Reviewed-By: James M Snell Reviewed-By: Gireesh Punathil Reviewed-By: Evan Lucas --- README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/README.md b/README.md index e37a2b70d82..db9696afb1a 100644 --- a/README.md +++ b/README.md @@ -238,6 +238,8 @@ For more information about the governance of the Node.js project, see **Сковорода Никита Андреевич** <chalkerx@gmail.com> (he/him) * [cjihrig](https://github.com/cjihrig) - **Colin Ihrig** <cjihrig@gmail.com> +* [danbev](https://github.com/danbev) - +**Daniel Bevenius** <daniel.bevenius@gmail.com> * [evanlucas](https://github.com/evanlucas) - **Evan Lucas** <evanlucas@me.com> (he/him) * [fhinkel](https://github.com/fhinkel) - From 9f61c7038557de30d1d3618aa94d356c6a64b6cc Mon Sep 17 00:00:00 2001 From: Evan Lucas Date: Wed, 13 Dec 2017 08:17:21 -0600 Subject: [PATCH 12/14] process: fix coverage generation e8a26e783e2e33514d44d8b2725d5f048e1314ce added `process` to the internal module wrapper. This broke the utility used to write coverage information due to a SyntaxError that `process` had already been declared. PR-URL: https://github.com/nodejs/node/pull/17651 Reviewed-By: Colin Ihrig Reviewed-By: Anna Henningsen Reviewed-By: Timothy Gu Reviewed-By: James M Snell Reviewed-By: Jon Moss Reviewed-By: Ruben Bridgewater Reviewed-By: Michael Dawson --- lib/internal/process/write-coverage.js | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/internal/process/write-coverage.js b/lib/internal/process/write-coverage.js index 1657f5b4708..17da7ee609d 100644 --- a/lib/internal/process/write-coverage.js +++ b/lib/internal/process/write-coverage.js @@ -1,5 +1,4 @@ 'use strict'; -const process = require('process'); const path = require('path'); const { mkdirSync, writeFileSync } = require('fs'); From e24ad97832979b945ccb2af6e2cf67caf7524cb1 Mon Sep 17 00:00:00 2001 From: Daniel Bevenius Date: Tue, 12 Dec 2017 07:30:01 +0100 Subject: [PATCH 13/14] test: remove unused disposed_ variable Currently building test_inspector_socket.cc generates the following warning: ../test/cctest/test_inspector_socket.cc:189:8: warning: private field 'disposed_' is not used [-Wunused-private-field] bool disposed_ = false; ^ 1 warning generated. This commit removes this variable. PR-URL: https://github.com/nodejs/node/pull/17628 Reviewed-By: Evan Lucas Reviewed-By: Colin Ihrig Reviewed-By: Jon Moss Reviewed-By: Ruben Bridgewater Reviewed-By: James M Snell Reviewed-By: Luigi Pinca --- test/cctest/test_inspector_socket.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/test/cctest/test_inspector_socket.cc b/test/cctest/test_inspector_socket.cc index 943109b8a59..debbc957379 100644 --- a/test/cctest/test_inspector_socket.cc +++ b/test/cctest/test_inspector_socket.cc @@ -186,7 +186,6 @@ class TestInspectorDelegate : public InspectorSocket::Delegate { void process(inspector_handshake_event event, const std::string& path); - bool disposed_ = false; delegate_fn handshake_delegate_; InspectorSocket::Pointer socket_; std::string ws_key_; From decab712ba14b8ec577025a57b8ab460fd3b8ec5 Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Sat, 25 Nov 2017 13:26:28 -0500 Subject: [PATCH 14/14] events: remove reaches into _events internals Refactor lib & src code to eliminate all deep reaches into the internal _events dictionary object, instead use available APIs and add an extra method to EventEmitter: rawListeners. PR-URL: https://github.com/nodejs/node/pull/17440 Reviewed-By: Anna Henningsen Reviewed-By: Timothy Gu Reviewed-By: Ruben Bridgewater Reviewed-By: Matteo Collina Reviewed-By: James M Snell --- doc/api/events.md | 33 +++++++++++++++++++ lib/events.js | 18 +++++++--- lib/internal/bootstrap_node.js | 1 - lib/vm.js | 13 ++------ src/env.h | 1 - src/node.cc | 6 ---- test/parallel/test-event-emitter-listeners.js | 19 +++++++++++ 7 files changed, 69 insertions(+), 22 deletions(-) diff --git a/doc/api/events.md b/doc/api/events.md index 8eaa8cae102..c935b7de0f7 100644 --- a/doc/api/events.md +++ b/doc/api/events.md @@ -574,6 +574,39 @@ to indicate an unlimited number of listeners. Returns a reference to the `EventEmitter`, so that calls can be chained. +### emitter.rawListeners(eventName) + +- `eventName` {any} + +Returns a copy of the array of listeners for the event named `eventName`, +including any wrappers (such as those created by `.once`). + +```js +const emitter = new EventEmitter(); +emitter.once('log', () => console.log('log once')); + +// Returns a new Array with a function `onceWrapper` which has a property +// `listener` which contains the original listener bound above +const listeners = emitter.rawListeners('log'); +const logFnWrapper = listeners[0]; + +// logs "log once" to the console and does not unbind the `once` event +logFnWrapper.listener(); + +// logs "log once" to the console and removes the listener +logFnWrapper(); + +emitter.on('log', () => console.log('log persistently')); +// will return a new Array with a single function bound by `on` above +const newListeners = emitter.rawListeners('log'); + +// logs "log persistently" twice +newListeners[0](); +emitter.emit('log'); +``` + [`--trace-warnings`]: cli.html#cli_trace_warnings [`EventEmitter.defaultMaxListeners`]: #events_eventemitter_defaultmaxlisteners [`domain`]: domain.html diff --git a/lib/events.js b/lib/events.js index 036758be82a..b9149d2b9b5 100644 --- a/lib/events.js +++ b/lib/events.js @@ -32,6 +32,7 @@ module.exports = EventEmitter; EventEmitter.EventEmitter = EventEmitter; EventEmitter.prototype._events = undefined; +EventEmitter.prototype._eventsCount = 0; EventEmitter.prototype._maxListeners = undefined; // By default EventEmitters will print a warning if more than 10 listeners are @@ -357,8 +358,8 @@ EventEmitter.prototype.removeAllListeners = return this; }; -EventEmitter.prototype.listeners = function listeners(type) { - const events = this._events; +function _listeners(target, type, unwrap) { + const events = target._events; if (events === undefined) return []; @@ -368,9 +369,18 @@ EventEmitter.prototype.listeners = function listeners(type) { return []; if (typeof evlistener === 'function') - return [evlistener.listener || evlistener]; + return unwrap ? [evlistener.listener || evlistener] : [evlistener]; + + return unwrap ? + unwrapListeners(evlistener) : arrayClone(evlistener, evlistener.length); +} + +EventEmitter.prototype.listeners = function listeners(type) { + return _listeners(this, type, true); +}; - return unwrapListeners(evlistener); +EventEmitter.prototype.rawListeners = function rawListeners(type) { + return _listeners(this, type, false); }; EventEmitter.listenerCount = function(emitter, type) { diff --git a/lib/internal/bootstrap_node.js b/lib/internal/bootstrap_node.js index c75578f7692..3f43c23e8d7 100644 --- a/lib/internal/bootstrap_node.js +++ b/lib/internal/bootstrap_node.js @@ -13,7 +13,6 @@ function startup() { const EventEmitter = NativeModule.require('events'); - process._eventsCount = 0; const origProcProto = Object.getPrototypeOf(process); Object.setPrototypeOf(origProcProto, EventEmitter.prototype); diff --git a/lib/vm.js b/lib/vm.js index 2f110b2db2b..82f140923a8 100644 --- a/lib/vm.js +++ b/lib/vm.js @@ -56,7 +56,7 @@ const realRunInThisContext = Script.prototype.runInThisContext; const realRunInContext = Script.prototype.runInContext; Script.prototype.runInThisContext = function(options) { - if (options && options.breakOnSigint && process._events.SIGINT) { + if (options && options.breakOnSigint && process.listenerCount('SIGINT') > 0) { return sigintHandlersWrap(realRunInThisContext, this, [options]); } else { return realRunInThisContext.call(this, options); @@ -64,7 +64,7 @@ Script.prototype.runInThisContext = function(options) { }; Script.prototype.runInContext = function(contextifiedSandbox, options) { - if (options && options.breakOnSigint && process._events.SIGINT) { + if (options && options.breakOnSigint && process.listenerCount('SIGINT') > 0) { return sigintHandlersWrap(realRunInContext, this, [contextifiedSandbox, options]); } else { @@ -95,14 +95,7 @@ function createScript(code, options) { // Remove all SIGINT listeners and re-attach them after the wrapped function // has executed, so that caught SIGINT are handled by the listeners again. function sigintHandlersWrap(fn, thisArg, argsArray) { - // Using the internal list here to make sure `.once()` wrappers are used, - // not the original ones. - let sigintListeners = process._events.SIGINT; - - if (Array.isArray(sigintListeners)) - sigintListeners = sigintListeners.slice(); - else - sigintListeners = [sigintListeners]; + const sigintListeners = process.rawListeners('SIGINT'); process.removeAllListeners('SIGINT'); diff --git a/src/env.h b/src/env.h index ba1558a13d5..408388ba9ec 100644 --- a/src/env.h +++ b/src/env.h @@ -145,7 +145,6 @@ class ModuleWrap; V(env_pairs_string, "envPairs") \ V(errno_string, "errno") \ V(error_string, "error") \ - V(events_string, "_events") \ V(exiting_string, "_exiting") \ V(exit_code_string, "exitCode") \ V(exit_string, "exit") \ diff --git a/src/node.cc b/src/node.cc index 4cdc50e85b5..0027c0cc7b8 100644 --- a/src/node.cc +++ b/src/node.cc @@ -3354,12 +3354,6 @@ void SetupProcessObject(Environment* env, env->SetMethod(process, "_setupNextTick", SetupNextTick); env->SetMethod(process, "_setupPromises", SetupPromises); env->SetMethod(process, "_setupDomainUse", SetupDomainUse); - - // pre-set _events object for faster emit checks - Local events_obj = Object::New(env->isolate()); - CHECK(events_obj->SetPrototype(env->context(), - Null(env->isolate())).FromJust()); - process->Set(env->events_string(), events_obj); } diff --git a/test/parallel/test-event-emitter-listeners.js b/test/parallel/test-event-emitter-listeners.js index 0736e3103e9..9d2892d2e89 100644 --- a/test/parallel/test-event-emitter-listeners.js +++ b/test/parallel/test-event-emitter-listeners.js @@ -82,3 +82,22 @@ function listener2() {} const s = new TestStream(); assert.deepStrictEqual(s.listeners('foo'), []); } + +{ + const ee = new events.EventEmitter(); + ee.on('foo', listener); + const wrappedListener = ee.rawListeners('foo'); + assert.strictEqual(wrappedListener.length, 1); + assert.strictEqual(wrappedListener[0], listener); + assert.notStrictEqual(wrappedListener, ee.rawListeners('foo')); + ee.once('foo', listener); + const wrappedListeners = ee.rawListeners('foo'); + assert.strictEqual(wrappedListeners.length, 2); + assert.strictEqual(wrappedListeners[0], listener); + assert.notStrictEqual(wrappedListeners[1], listener); + assert.strictEqual(wrappedListeners[1].listener, listener); + assert.notStrictEqual(wrappedListeners, ee.rawListeners('foo')); + ee.emit('foo'); + assert.strictEqual(wrappedListeners.length, 2); + assert.strictEqual(wrappedListeners[1].listener, listener); +}