Skip to content

fs: make ReadStream throw error on NaN #19732

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

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 3 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
10 changes: 6 additions & 4 deletions lib/fs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2008,13 +2008,13 @@ function ReadStream(path, options) {
this.closed = false;

if (this.start !== undefined) {
if (typeof this.start !== 'number') {
throw new ERR_INVALID_ARG_TYPE('start', 'number', this.start);
if (typeof this.start !== 'number' || !Number.isInteger(this.start)) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: the condition can be just !Number.isInteger() as that returns true when passing something that is not a number.

Copy link

Choose a reason for hiding this comment

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

Yes: http://www.ecma-international.org/ecma-262/8.0/index.html#sec-number.isinteger

throw new ERR_INVALID_ARG_TYPE('start', 'integer', this.start);
Copy link
Member

Choose a reason for hiding this comment

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

It would be great to use much more detailed errors here. As @anliting pointed out it is not always correct to throw a ERR_INVALID_ARG_TYPE here.

What you can do is to check here what exactly you got that does not fit the expectations as in:

if (typeof this.start !== 'number') {
  throw new ERR_INVALID_ARG_TYPE('options.start', 'number', options.start);
}
throw new ERR_OUT_OF_RANGE('options.start', 'an integer', options.start);

The same applies to all other errors in here. Please also be aware how I changed start to options.start (not 'this.start' as that is not the input).

Copy link

Choose a reason for hiding this comment

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

@BridgeAR Yes, doing things right is more important than having program run faster just a little bit; sorry for suggesting "just choose 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.

Your suggestion is totally fine. It will only be slower in the error case (when `Number.isInteger returns false).

}
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' || !Number.isInteger(this.end)) {
throw new ERR_INVALID_ARG_TYPE('end', 'integer', this.end);
Copy link
Member

Choose a reason for hiding this comment

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

The same as above: distinguishing the type from the rest of the error would be good. You can actually copy the same part and just change what variable it applies to.

Copy link

Choose a reason for hiding this comment

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

For example, change

    if (this.end === undefined) {
      this.end = Infinity;
    } else if (!Number.isInteger(this.end)) {
      throw new ERR_INVALID_ARG_TYPE('end', 'integer', this.end);
    }

to

    if (this.end === undefined) {
      this.end = Infinity;
    }else{
      if(typeof this.end!=='number'){
        throw new ERR_INVALID_ARG_TYPE('end','number',this.end)
      }
      if (!Number.isInteger(this.end)) {
        throw new ERR_OUT_OF_RANGE('end', 'integer', this.end);
      }
    }

.

}

if (this.start > this.end) {
Expand All @@ -2030,6 +2030,8 @@ function ReadStream(path, options) {
// (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')
Copy link
Member

Choose a reason for hiding this comment

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

This should not be included in the else block. The if / else is only about this.start and this.end and nothing else.

Copy link

Choose a reason for hiding this comment

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

Yes, it should not.

this.open();
Expand Down
9 changes: 9 additions & 0 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 @@ -25,3 +29,8 @@ 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 });
Copy link

Choose a reason for hiding this comment

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

I think we can leave only createReadStreamErr(example, { start: NaN }), because if start is not passed, end is not defined (https://github.com/ryzokuken/node/blob/473dd3d03fe4d08445518f9ab8e2b78892c22e27/lib/fs.js#L2010).

And at least we need to delete createReadStreamErr(example, { end: NaN }) to pass node-test-commit-linux.

Copy link
Member

Choose a reason for hiding this comment

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

@anliting If start is not passed but end is, then the value is still set and type-checked: https://github.com/ryzokuken/node/blob/473dd3d03fe4d08445518f9ab8e2b78892c22e27/lib/fs.js#L2031-L2032

Copy link

Choose a reason for hiding this comment

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

If we are going to test the end part, we need have start to pass the range test first: createReadStreamErr(example, { start: 0, end: NaN })

Copy link
Member

Choose a reason for hiding this comment

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

@anliting That would take a different branch, though?

Copy link

@anliting anliting Apr 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@addaleax

Oh, I see that line (which makes end is still set and type-checked) now, my fault.

"If we are going to test the end part ..." means if we are going to test if this line work:
https://github.com/ryzokuken/node/blob/473dd3d03fe4d08445518f9ab8e2b78892c22e27/lib/fs.js#L2016

Copy link

@anliting anliting Apr 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

We might need to change

  if (typeof this.end !== 'number' || Number.isNaN(this.end))
    this.end = Infinity;

to

  if (typeof this.end !== 'number')
    this.end = Infinity;
  else if (Number.isNaN(this.end))
    throw new ERR_INVALID_ARG_TYPE('end', 'integer', this.end);

to pass node-test-commit-linux.

Copy link
Member

Choose a reason for hiding this comment

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

@anliting Ah, right! Yes, we should probably do that.

createReadStreamErr(example, { end: NaN });
createReadStreamErr(example, { start: NaN, end: NaN });
Copy link
Member

Choose a reason for hiding this comment

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

Please add some tests with Infinity as value.

Copy link

@anliting anliting Apr 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@addaleax Should the case {start:0,end:Infinity} throw (according to doc), or not (maybe for backward compatibility)?

Copy link
Member

Choose a reason for hiding this comment

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

@anliting I think Infinity should be a valid value for end, yes… It should o the same as passing in end: undefined (or no end: value at all), but removing support for Infinity as a value is probably too much of a breaking change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly what I had been thinking. Doesn't throwing an error on Infinity make this semver-major?

Copy link

Choose a reason for hiding this comment

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

My appointment on that {start:0,end:Infinity} should throw is that the documentation does not allow passing Infinity as end (doc says end is an integer); no support is being removed, since it is never supported. In this point of view, "not throwing on {start:0,end:Infinity}" is a bug, and the old programs that worked are just lucky.

But passing Infinity as end is not harmful at all; it is even a good feature. Maybe we can change the doc, allowing it from now, making this a semver-minor?

Copy link
Member

Choose a reason for hiding this comment

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

But passing Infinity as end is not harmful at all; it is even a good feature.

👍

Maybe we can change the doc, allowing it from now, making this a semver-minor?

It’s probably a good idea to let the docs reflect this, but it might not even have to be semver-minor, if it’s just documenting existing behaviour. (After all, this is supposed to be a bug fix PR.)

Like, you’re right that technically we’d be adding support for something that is not documented as part of the API, but with Node we have to be conservative about these things and can’t just say “X is unsupported because it’s undocumented”.

Copy link

@anliting anliting Apr 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@addaleax Okay, now I understand your position for this; I'll get used to that. XD

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@addaleax so, should I also make changes to docs in this PR?

Copy link
Member

Choose a reason for hiding this comment

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

@ryzokuken You can do that, or you can do it in another PR, or you can leave it up for somebody else to document this. But preferably one of the first two options :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

option 2 sounds perfect. Let me address all the concerns regarding the code in this PR and I'd make another one for the documentation changes.