Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fs: make ReadStream throw error on NaN #19732

Closed
wants to merge 8 commits into from
Closed

fs: make ReadStream throw error on NaN #19732

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
473dd3d
fs: make ReadStream throw error on NaN
ryzokuken Apr 1, 2018
83f478e
fs, test: add tests for #19732
ryzokuken Apr 1, 2018
a4077ea
Resolve nits and fix failures (hopefully)
ryzokuken Apr 2, 2018
1ff2f51
Remove redundant condition
ryzokuken Apr 2, 2018
23cc8c0
Add an else block
ryzokuken Apr 2, 2018
0ae768d
Address concerns
ryzokuken Apr 2, 2018
163f911
Improve errors and tests
ryzokuken Apr 2, 2018
c49c543
Incorporate required changes
ryzokuken Apr 3, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 19 additions & 9 deletions lib/fs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2009,12 +2009,20 @@ function ReadStream(path, options) {

if (this.start !== undefined) {
if (typeof this.start !== 'number') {
throw new ERR_INVALID_ARG_TYPE('start', 'number', this.start);
throw new ERR_INVALID_ARG_TYPE('options.start', 'number', options.start);
}
if (!Number.isInteger(this.start)) {
throw new ERR_OUT_OF_RANGE('options.start', 'integer', options.start);
}
if (this.end === undefined) {
this.end = Infinity;
} else if (typeof this.end !== 'number') {
throw new ERR_INVALID_ARG_TYPE('end', 'number', 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) {
Expand All @@ -2023,13 +2031,15 @@ 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')
} 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).
this.end = Infinity;
} 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')
this.open();
Expand Down
29 changes: 23 additions & 6 deletions test/parallel/test-fs-read-stream-throw-type-error.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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');
Expand All @@ -10,18 +14,31 @@ 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 }, 'ERR_OUT_OF_RANGE');
createReadStreamErr(example, { end: NaN }, 'ERR_OUT_OF_RANGE');
createReadStreamErr(example, { start: NaN, end: NaN }, 'ERR_OUT_OF_RANGE');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please still add tests for Infinity? :-) That is independent of making a breaking change. Setting start to Infinity will still throw right now and setting end to infinity should not throw.


// 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');