From 473dd3d03fe4d08445518f9ab8e2b78892c22e27 Mon Sep 17 00:00:00 2001 From: Ujjwal Sharma Date: Sun, 1 Apr 2018 23:11:54 +0530 Subject: [PATCH 1/8] fs: make ReadStream throw error on NaN This test makes fs.ReadStream throw an error when either start or end is NaN. Fixes: https://github.com/nodejs/node/issues/19715 --- lib/fs.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 2de5f8dd6e205b..91196015980a4e 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -2008,12 +2008,12 @@ function ReadStream(path, options) { this.closed = false; if (this.start !== undefined) { - if (typeof this.start !== 'number') { + if (typeof this.start !== 'number' || Number.isNaN(this.start)) { throw new ERR_INVALID_ARG_TYPE('start', 'number', this.start); } if (this.end === undefined) { this.end = Infinity; - } else if (typeof this.end !== 'number') { + } else if (typeof this.end !== 'number' || Number.isNaN(this.end)) { throw new ERR_INVALID_ARG_TYPE('end', 'number', this.end); } @@ -2028,7 +2028,7 @@ function ReadStream(path, options) { // Backwards compatibility: Make sure `end` is a number regardless of `start`. // TODO(addaleax): Make the above typecheck not depend on `start` instead. // (That is a semver-major change). - if (typeof this.end !== 'number') + if (typeof this.end !== 'number' || Number.isNaN(this.end)) this.end = Infinity; if (typeof this.fd !== 'number') From 83f478e0dbc9c91ace685dc0225fad8c0ac4fdd2 Mon Sep 17 00:00:00 2001 From: Ujjwal Sharma Date: Mon, 2 Apr 2018 00:06:12 +0530 Subject: [PATCH 2/8] fs, test: add tests for #19732 Add regression tests for https://github.com/nodejs/node/pull/19732, making sure appropraite TypeErrors are thrown for NaN values. Refs: https://github.com/nodejs/node/pull/19732 --- .../parallel/test-fs-read-stream-throw-type-error.js | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/test/parallel/test-fs-read-stream-throw-type-error.js b/test/parallel/test-fs-read-stream-throw-type-error.js index c2b0d5452c55e3..8728547745db16 100644 --- a/test/parallel/test-fs-read-stream-throw-type-error.js +++ b/test/parallel/test-fs-read-stream-throw-type-error.js @@ -1,6 +1,10 @@ 'use strict'; const common = require('../common'); const fixtures = require('../common/fixtures'); + +// This test ensures that fs.createReadStream throws a TypeError when invalid +// arguments are passed to it. + const fs = require('fs'); const example = fixtures.path('x.txt'); @@ -18,10 +22,16 @@ const createReadStreamErr = (path, opt) => { { code: 'ERR_INVALID_ARG_TYPE', type: TypeError - }); + } + ); }; createReadStreamErr(example, 123); createReadStreamErr(example, 0); createReadStreamErr(example, true); createReadStreamErr(example, false); + +// Should also throw on NaN (for https://github.com/nodejs/node/pull/19732) +createReadStreamErr(example, { start: NaN }); +createReadStreamErr(example, { end: NaN }); +createReadStreamErr(example, { start: NaN, end: NaN }); From a4077ea66afb6a0aa3ec4f7ed5a63b2ea6d09749 Mon Sep 17 00:00:00 2001 From: Ujjwal Sharma Date: Mon, 2 Apr 2018 10:02:29 +0530 Subject: [PATCH 3/8] Resolve nits and fix failures (hopefully) --- lib/fs.js | 12 +++++++----- .../parallel/test-fs-read-stream-throw-type-error.js | 3 +-- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 91196015980a4e..ff52cec95280d3 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -2008,13 +2008,13 @@ function ReadStream(path, options) { this.closed = false; if (this.start !== undefined) { - if (typeof this.start !== 'number' || Number.isNaN(this.start)) { - throw new ERR_INVALID_ARG_TYPE('start', 'number', this.start); + if (typeof this.start !== 'number' || !Number.isInteger(this.start)) { + throw new ERR_INVALID_ARG_TYPE('start', 'integer', this.start); } if (this.end === undefined) { this.end = Infinity; - } else if (typeof this.end !== 'number' || Number.isNaN(this.end)) { - throw new ERR_INVALID_ARG_TYPE('end', 'number', this.end); + } else if (typeof this.end !== 'number' || !Number.isInteger(this.end)) { + throw new ERR_INVALID_ARG_TYPE('end', 'integer', this.end); } if (this.start > this.end) { @@ -2028,8 +2028,10 @@ function ReadStream(path, options) { // Backwards compatibility: Make sure `end` is a number regardless of `start`. // TODO(addaleax): Make the above typecheck not depend on `start` instead. // (That is a semver-major change). - if (typeof this.end !== 'number' || Number.isNaN(this.end)) + if (typeof this.end !== 'number') this.end = Infinity; + else if (!Number.isInteger(this.end)) + throw new ERR_INVALID_ARG_TYPE('end', 'integer', this.end); if (typeof this.fd !== 'number') this.open(); diff --git a/test/parallel/test-fs-read-stream-throw-type-error.js b/test/parallel/test-fs-read-stream-throw-type-error.js index 8728547745db16..8d559f13879691 100644 --- a/test/parallel/test-fs-read-stream-throw-type-error.js +++ b/test/parallel/test-fs-read-stream-throw-type-error.js @@ -22,8 +22,7 @@ const createReadStreamErr = (path, opt) => { { code: 'ERR_INVALID_ARG_TYPE', type: TypeError - } - ); + }); }; createReadStreamErr(example, 123); From 1ff2f515721940d1c41b3c20a9d1ee0d96b028ce Mon Sep 17 00:00:00 2001 From: Ujjwal Sharma Date: Mon, 2 Apr 2018 14:47:24 +0530 Subject: [PATCH 4/8] Remove redundant condition --- lib/fs.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index ff52cec95280d3..7aeff6c70ef1ef 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -2008,12 +2008,12 @@ function ReadStream(path, options) { this.closed = false; if (this.start !== undefined) { - if (typeof this.start !== 'number' || !Number.isInteger(this.start)) { + if (!Number.isInteger(this.start)) { throw new ERR_INVALID_ARG_TYPE('start', 'integer', this.start); } if (this.end === undefined) { this.end = Infinity; - } else if (typeof this.end !== 'number' || !Number.isInteger(this.end)) { + } else if (!Number.isInteger(this.end)) { throw new ERR_INVALID_ARG_TYPE('end', 'integer', this.end); } From 23cc8c0200958ac11c8223ef32854f8a31cda472 Mon Sep 17 00:00:00 2001 From: Ujjwal Sharma Date: Mon, 2 Apr 2018 16:25:07 +0530 Subject: [PATCH 5/8] Add an else block --- lib/fs.js | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 7aeff6c70ef1ef..9031d659be2632 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -2023,24 +2023,25 @@ function ReadStream(path, options) { } this.pos = this.start; - } - - // Backwards compatibility: Make sure `end` is a number regardless of `start`. - // TODO(addaleax): Make the above typecheck not depend on `start` instead. - // (That is a semver-major change). - if (typeof this.end !== 'number') - this.end = Infinity; - else if (!Number.isInteger(this.end)) - throw new ERR_INVALID_ARG_TYPE('end', 'integer', this.end); + } else { + // Backwards compatibility: Make sure `end` is a number regardless of + // `start`. + // TODO(addaleax): Make the above typecheck not depend on `start` instead. + // (That is a semver-major change). + if (typeof this.end !== 'number') + this.end = Infinity; + else if (!Number.isInteger(this.end)) + throw new ERR_INVALID_ARG_TYPE('end', 'integer', this.end); - if (typeof this.fd !== 'number') - this.open(); + if (typeof this.fd !== 'number') + this.open(); - this.on('end', function() { - if (this.autoClose) { - this.destroy(); - } - }); + this.on('end', function() { + if (this.autoClose) { + this.destroy(); + } + }); + } } fs.FileReadStream = fs.ReadStream; // support the legacy name From 0ae768d39027449d00e297aff296bfa866a87906 Mon Sep 17 00:00:00 2001 From: Ujjwal Sharma Date: Mon, 2 Apr 2018 22:30:05 +0530 Subject: [PATCH 6/8] Address concerns --- lib/fs.js | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 9031d659be2632..95d499e2e3d134 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -2008,8 +2008,11 @@ function ReadStream(path, options) { this.closed = false; if (this.start !== undefined) { + if (typeof this.start !== 'number') { + throw new ERR_INVALID_ARG_TYPE('options.start', 'number', options.start); + } if (!Number.isInteger(this.start)) { - throw new ERR_INVALID_ARG_TYPE('start', 'integer', this.start); + throw new ERR_OUT_OF_RANGE('options.start', 'an integer', options.start); } if (this.end === undefined) { this.end = Infinity; @@ -2023,25 +2026,24 @@ function ReadStream(path, options) { } this.pos = this.start; - } else { + } else if (typeof this.end !== 'number') { // Backwards compatibility: Make sure `end` is a number regardless of // `start`. // TODO(addaleax): Make the above typecheck not depend on `start` instead. // (That is a semver-major change). - if (typeof this.end !== 'number') - this.end = Infinity; - else if (!Number.isInteger(this.end)) - throw new ERR_INVALID_ARG_TYPE('end', 'integer', this.end); + this.end = Infinity; + } else if (!Number.isInteger(this.end)) { + throw new ERR_INVALID_ARG_TYPE('end', 'integer', this.end); + } - if (typeof this.fd !== 'number') - this.open(); + if (typeof this.fd !== 'number') + this.open(); - this.on('end', function() { - if (this.autoClose) { - this.destroy(); - } - }); - } + this.on('end', function() { + if (this.autoClose) { + this.destroy(); + } + }); } fs.FileReadStream = fs.ReadStream; // support the legacy name From 163f91128c1ef0952a2ef9e4fb4339b97bfbc6a4 Mon Sep 17 00:00:00 2001 From: Ujjwal Sharma Date: Mon, 2 Apr 2018 22:38:27 +0530 Subject: [PATCH 7/8] Improve errors and tests --- lib/fs.js | 2 +- .../test-fs-read-stream-throw-type-error.js | 18 +++++++++--------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 95d499e2e3d134..2aa255a19d2bcd 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -2033,7 +2033,7 @@ function ReadStream(path, options) { // (That is a semver-major change). this.end = Infinity; } else if (!Number.isInteger(this.end)) { - throw new ERR_INVALID_ARG_TYPE('end', 'integer', this.end); + throw new ERR_OUT_OF_RANGE('options.end', 'an integer', options.end); } if (typeof this.fd !== 'number') diff --git a/test/parallel/test-fs-read-stream-throw-type-error.js b/test/parallel/test-fs-read-stream-throw-type-error.js index 8d559f13879691..9b40fcb3e0e76d 100644 --- a/test/parallel/test-fs-read-stream-throw-type-error.js +++ b/test/parallel/test-fs-read-stream-throw-type-error.js @@ -14,23 +14,23 @@ fs.createReadStream(example, null); fs.createReadStream(example, 'utf8'); fs.createReadStream(example, { encoding: 'utf8' }); -const createReadStreamErr = (path, opt) => { +const createReadStreamErr = (path, opt, errorCode) => { common.expectsError( () => { fs.createReadStream(path, opt); }, { - code: 'ERR_INVALID_ARG_TYPE', + code: errorCode, type: TypeError }); }; -createReadStreamErr(example, 123); -createReadStreamErr(example, 0); -createReadStreamErr(example, true); -createReadStreamErr(example, false); +createReadStreamErr(example, 123, 'ERR_INVALID_ARG_TYPE'); +createReadStreamErr(example, 0, 'ERR_INVALID_ARG_TYPE'); +createReadStreamErr(example, true, 'ERR_INVALID_ARG_TYPE'); +createReadStreamErr(example, false, 'ERR_INVALID_ARG_TYPE'); // Should also throw on NaN (for https://github.com/nodejs/node/pull/19732) -createReadStreamErr(example, { start: NaN }); -createReadStreamErr(example, { end: NaN }); -createReadStreamErr(example, { start: NaN, end: NaN }); +createReadStreamErr(example, { start: NaN }, 'ERR_OUT_OF_RANGE'); +createReadStreamErr(example, { end: NaN }, 'ERR_OUT_OF_RANGE'); +createReadStreamErr(example, { start: NaN, end: NaN }, 'ERR_OUT_OF_RANGE'); From c49c543ae53269188c8c631ef1f26d3c5cbabd6f Mon Sep 17 00:00:00 2001 From: Ujjwal Sharma Date: Tue, 3 Apr 2018 14:48:49 +0530 Subject: [PATCH 8/8] Incorporate required changes --- lib/fs.js | 15 ++++++++++----- .../test-fs-read-stream-throw-type-error.js | 8 ++++++++ 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 2aa255a19d2bcd..489f7c0fd1a6a7 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -2012,12 +2012,17 @@ function ReadStream(path, options) { throw new ERR_INVALID_ARG_TYPE('options.start', 'number', options.start); } if (!Number.isInteger(this.start)) { - throw new ERR_OUT_OF_RANGE('options.start', 'an integer', options.start); + throw new ERR_OUT_OF_RANGE('options.start', 'integer', options.start); } if (this.end === undefined) { this.end = Infinity; - } else if (!Number.isInteger(this.end)) { - throw new ERR_INVALID_ARG_TYPE('end', 'integer', this.end); + } else { + if (typeof this.end !== 'number') { + throw new ERR_INVALID_ARG_TYPE('options.end', 'number', this.end); + } + if (!(Number.isInteger(this.end) || this.end === Infinity)) { + throw new ERR_OUT_OF_RANGE('options.end', 'integer/infinity', this.end); + } } if (this.start > this.end) { @@ -2032,8 +2037,8 @@ function ReadStream(path, options) { // TODO(addaleax): Make the above typecheck not depend on `start` instead. // (That is a semver-major change). this.end = Infinity; - } else if (!Number.isInteger(this.end)) { - throw new ERR_OUT_OF_RANGE('options.end', 'an integer', options.end); + } else if (!(Number.isInteger(this.end) || this.end === Infinity)) { + throw new ERR_OUT_OF_RANGE('options.end', 'integer/infinity', this.end); } if (typeof this.fd !== 'number') diff --git a/test/parallel/test-fs-read-stream-throw-type-error.js b/test/parallel/test-fs-read-stream-throw-type-error.js index 9b40fcb3e0e76d..b444e48a9d4792 100644 --- a/test/parallel/test-fs-read-stream-throw-type-error.js +++ b/test/parallel/test-fs-read-stream-throw-type-error.js @@ -34,3 +34,11 @@ createReadStreamErr(example, false, 'ERR_INVALID_ARG_TYPE'); createReadStreamErr(example, { start: NaN }, 'ERR_OUT_OF_RANGE'); createReadStreamErr(example, { end: NaN }, 'ERR_OUT_OF_RANGE'); createReadStreamErr(example, { start: NaN, end: NaN }, 'ERR_OUT_OF_RANGE'); + +// Should also throw on non-integer Numbers and non-numbers +createReadStreamErr(example, { start: 'a' }, 'ERR_INVALID_ARG_TYPE'); +createReadStreamErr(example, { start: 0.1 }, 'ERR_OUT_OF_RANGE'); +createReadStreamErr(example, { start: 0, end: 'a' }, 'ERR_INVALID_ARG_TYPE'); +createReadStreamErr(example, { start: 0, end: 0.1 }, 'ERR_OUT_OF_RANGE'); +createReadStreamErr(example, { start: 1, end: 0 }, 'ERR_OUT_OF_RANGE'); +createReadStreamErr(example, { end: 0.1 }, 'ERR_OUT_OF_RANGE');