fs: make ReadStream throw error on NaN #19732

ryzokuken · 2018-04-01T17:46:04Z

This test makes fs.ReadStream throw an error when either start or
end is NaN.

Fixes: #19715

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

/cc @lpinca

ryzokuken · 2018-04-01T17:46:42Z

@lpinca should I also add a regression test for it?

BridgeAR · 2018-04-01T17:51:45Z

@ryzokuken yes please. Any code change should ideally come with a test.

addaleax · 2018-04-01T18:03:19Z

lib/fs.js

      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)) {


Can you add this for the other typeof this.end !== 'number' check below as well? The one that is supposed to be merged with this one? :)

@addaleax Sure! On it.

@addaleax done.

This test makes fs.ReadStream throw an error when either start or end is NaN. Fixes: nodejs#19715

ryzokuken · 2018-04-01T18:15:45Z

@BridgeAR I'd love to add the test, but because this would be my first bugfix and thus the first test I'd be writing from scratch, I'd need some help with it. For starters, does it need to be a parallel test or a sequential one? Or perhaps it's neither?

addaleax · 2018-04-01T18:17:31Z

https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md :)

parallel should work fine, I think.

Add regression tests for nodejs#19732, making sure appropraite TypeErrors are thrown for NaN values. Refs: nodejs#19732

ryzokuken · 2018-04-01T18:39:07Z

@addaleax there was an existing test, so I added assertions to that instead. Let me know if that doesn't work, and I'd be happy to make a separate test file.

addaleax

LGTM, thanks!

In particular, I think this is semver-patch because the issue shows that this would never actually work to begin with, and break creation of other ReadStream instances.

ryzokuken · 2018-04-01T18:58:47Z

@addaleax Thanks. That's a relief. semver-major sounded scary.

lpinca · 2018-04-01T19:24:47Z

test/parallel/test-fs-read-stream-throw-type-error.js

@@ -18,10 +22,16 @@ const createReadStreamErr = (path, opt) => {
    {
      code: 'ERR_INVALID_ARG_TYPE',
      type: TypeError
-    });


Nit: unnecessary change.

lpinca · 2018-04-01T19:30:36Z

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)) {


As suggested by @anliting we can probably use just Number.isInteger().

lpinca · 2018-04-01T19:52:08Z

CI: https://ci.nodejs.org/job/node-test-pull-request/13975/

anliting · 2018-04-01T20:46:12Z

test/parallel/test-fs-read-stream-throw-type-error.js

 };

 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 });


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.

@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

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 })

@anliting That would take a different branch, though?

@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

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.

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

anliting · 2018-04-01T21:24:01Z

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


Would throw new ERR_INVALID_ARG_TYPE('start', 'integer', this.start); be more appropriate? Since typeof NaN=='number'.

ryzokuken · 2018-04-02T04:24:35Z

@anliting @addaleax thanks for figuring this out for me, pushing the required changes in a minute.

ryzokuken · 2018-04-02T04:36:15Z

looks better now?

lpinca · 2018-04-02T05:12:53Z

lib/fs.js

@@ -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)) {


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

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

ryzokuken · 2018-04-02T09:21:48Z

@lpinca @anliting done, take a look.

anliting · 2018-04-02T09:48:26Z

@ryzokuken
LGTM. Besides, I think neither ERR_INVALID_ARG_TYPE nor ERR_OUT_OF_RANGE fits this situation, maybe we can just choose ERR_OUT_OF_RANGE.

anliting · 2018-04-02T10:17:56Z

Oops, I think I found another problem. It seems to throw when we pass {start:0} as options.

Change

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

    if (this.start > this.end) {
      const errVal = `{start: ${this.start}, end: ${this.end}}`;
      throw new ERR_OUT_OF_RANGE('start', '<= "end"', errVal);
    }

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

to

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

    if (this.start > this.end) {
      const errVal = `{start: ${this.start}, end: ${this.end}}`;
      throw new ERR_OUT_OF_RANGE('start', '<= "end"', errVal);
    }

    this.pos = this.start;
  }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);
  }

.

ryzokuken · 2018-04-02T10:55:30Z

@anliting better now?

anliting · 2018-04-02T10:57:17Z

Better ^^

anliting · 2018-04-07T00:20:38Z

@ryzokuken

Yes, that understanding is right. What I like more would be "until end bytes and reads end-start bytes".

If you define the behavior as "read from start to end", end<start isn't valid. But if you define it as I said (#19732 (comment)), there isn't a problem (like [0,1,2,3].slice(3,1).length==0), user can just code the start and end independently.

anliting · 2018-04-07T00:23:13Z

@ryzokuken

I think the both behaviors make a sense of its own; so I have no idea to choose.

ryzokuken · 2018-04-07T00:28:06Z

Hmm, so I'd consult either @addaleax or someone else from the TSC regarding that particular behavior. Regarding fractions, Anna didn't want to throw on them either, because while it might not make a lot of sense, a lot of people do intricate math to calculate the values of both these parameters, and they might not get an integer all the time.

anliting · 2018-04-07T17:22:03Z

@ryzokuken

"Exception" means those situations which we cannot handle; "exception handling" means we throw them back and hope if someone else handles.

If we don't throw on fractions, we will have to handle them. For example:

options.start=Math.ceil(options.start)
options.end=Math.floor(options.end)

or

options.start=Math.floor(options.start)
options.end=Math.floor(options.end)

If we current have no idea how to handle it, we can throw on them first, and wait for the idea comes. However, that makes another semver-major.

ryzokuken · 2018-04-07T18:15:33Z

Again, I'd defer to @addaleax regarding that.

addaleax · 2018-04-07T21:07:12Z

I think what’s currently happening is that both values are being rounded down? That sounds okay, especially because we have this very weird behaviour where end is inclusive, contrary to all conventions in JS and similar languages. (It’s not a harmful thing, just odd.) Rounding up for end also seems okay, or actually rounding the values to the nearest integer (like, when start is 12.99999998 for some reason).

I mean, honestly, I don’t think it’d be the end of the world if we threw on non-integer numbers, and my opinion isn’t always gold either. I’m just thinking, JS is a language where the math just happens to be defined in terms of floats, so it’s probably better to be on the safe side and implement something that handles that somewhat gracefully.

ryzokuken · 2018-04-07T21:18:59Z

As long as we are "handling" fractions internally in a predictable manner, I think @anliting's very helpful suggestion is satisfied as well. I mean, we only need to throw where we aren't in a position to handle it ourselves, right? Here we can.

My suggestion: let's make this behavior of handling fractional arguments well-documented though, so that anyone doing complex math that might return a fraction (our target audience for this, basically), are well-informed and can reason about this behavior rationally.

@addaleax so, should we just round down on both arguments? Or maybe round down on start and up on end? Rounding to the nearest integer doesn't sound like something reliable, predictable to even reasonable IMO. What are your thoughts on this?

addaleax · 2018-04-07T21:24:49Z

Rounding to the nearest integer doesn't sound like something reliable, predictable to even reasonable IMO. What are your thoughts on this?

When your math gives you 12.9999998, which should have turned out to 13 if it weren’t for floating-point inaccuracies, and we round down to 12, then all your offsets into the returned buffers are wrong.

Honestly, I don’t think many people would be affected by this either way, so it’s probably more important to pick some consistent behaviour than determining which exactly it should be?

ryzokuken · 2018-04-07T21:32:00Z

When your math gives you 12.9999998, which should have turned out to 13 if it weren’t for floating-point inaccuracies, and we round down to 12, then all your offsets into the returned buffers are wrong.

Agreed. That makes sense.

it’s probably more important to pick some consistent behaviour than determining which exactly it should be?

You're right, but we'll still need to pick one anyway. Let me know whichever seems more reasonable to you.

anliting · 2018-04-08T03:41:37Z

After reading your conversation, I would vote for "Math.round for both start and end". In contrary of floating-point inaccuracies, it is rare that the user provides numbers which are far away from integers and it still doesn't want to round it itself.

ryzokuken · 2018-04-08T07:36:16Z

Updated table:

So, for the course of this pull request:

Type	Current Behavior	Proposed Behavior
`undefined`	(0, `Infinity`)	❓
Anything other than `Number`	`ERR_INVALID_ARG_TYPE`	`ERR_INVALID_ARG_TYPE`
`NaN`	`ERR_INVALID_ARG_TYPE`	`ERR_INVALID_ARG_TYPE`
Negative `Number`s	✘	`ERR_OUT_OF_RANGE`
Fractional `Number`s	✘	`Math.round`
Integers	Work Perfectly	Still work perfectly
Invalid Arguments	`ERR_OUT_OF_RANGE`	`ERR_OUT_OF_RANGE`

@addaleax @anliting I think we all agree on this now, right?

anliting · 2018-04-08T09:27:09Z

@ryzokuken

I prefer ERR_OUT_OF_RANGE on NaN (#19732 (comment)), and I guess it will still be (0, Infinity) on undefined.

ryzokuken · 2018-04-08T09:40:07Z

I prefer ERR_OUT_OF_RANGE on NaN

Let me guess, because it's not the wrong type (still a Number), but the wrong value of the correct type?

That does make a lot of sense.

ryzokuken · 2018-04-08T17:24:28Z

So, finally:

Type	Current Behavior	Proposed Behavior
`undefined`	(0, `Infinity`)	(0, `Infinity`)
Anything other than `Number`	`ERR_INVALID_ARG_TYPE`	`ERR_INVALID_ARG_TYPE`
`NaN`	`ERR_INVALID_ARG_TYPE`	`ERR_OUT_OF_RANGE`
Negative `Number`s	✘	`ERR_OUT_OF_RANGE`
Fractional `Number`s	✘	`Math.round`
Integers	Work Perfectly	Still work perfectly
Invalid Arguments	`ERR_OUT_OF_RANGE`	`ERR_OUT_OF_RANGE`

@addaleax @anliting this looks good? I've been thinking of making a new PR so that others don't get bogged down by the conversation in here.

addaleax · 2018-04-08T19:13:44Z

@ryzokuken Yes, I think that seems fine, assuming that “Invalid Arguments” means end < start?

ryzokuken · 2018-04-09T01:06:14Z

@addaleax they do. Great. Making another PR for this today.

anliting · 2018-04-09T01:34:14Z

@ryzokuken

LGTM. ^^

Improve handling of erratic arguments in fs.ReadStream Refs: nodejs#19732

ryzokuken · 2018-04-23T20:48:56Z

Closing in favor of the other two PRs.

Improve handling of erratic arguments in fs.ReadStream Refs: #19732 PR-URL: #19898 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ron Korving <ron@ronkorving.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Apr 1, 2018

addaleax reviewed Apr 1, 2018

View reviewed changes

fs: make ReadStream throw error on NaN

473dd3d

This test makes fs.ReadStream throw an error when either start or end is NaN. Fixes: nodejs#19715

ryzokuken force-pushed the fix-19715 branch from 49a9ade to 473dd3d Compare April 1, 2018 18:13

fs, test: add tests for nodejs#19732

83f478e

Add regression tests for nodejs#19732, making sure appropraite TypeErrors are thrown for NaN values. Refs: nodejs#19732

addaleax approved these changes Apr 1, 2018

View reviewed changes

lpinca approved these changes Apr 1, 2018

View reviewed changes

lpinca reviewed Apr 1, 2018

View reviewed changes

anliting reviewed Apr 1, 2018

View reviewed changes

Resolve nits and fix failures (hopefully)

a4077ea

ryzokuken force-pushed the fix-19715 branch from 318414b to a4077ea Compare April 2, 2018 04:34

lpinca reviewed Apr 2, 2018

View reviewed changes

Remove redundant condition

1ff2f51

anliting mentioned this pull request Apr 2, 2018

fs.createReadStream doesn't throw with options={start:NaN,end:NaN} #19715

Closed

Add an else block

23cc8c0

ryzokuken mentioned this pull request Apr 9, 2018

fs: improve argument handling for ReadStream #19898

Closed

4 tasks

ryzokuken added a commit to ryzokuken/node that referenced this pull request Apr 16, 2018

fs: improve argument handling for ReadStream

e507f93

Improve handling of erratic arguments in fs.ReadStream Refs: nodejs#19732

ryzokuken closed this Apr 23, 2018

fs: make ReadStream throw error on NaN #19732

fs: make ReadStream throw error on NaN #19732

Conversation

ryzokuken commented Apr 1, 2018

Checklist

ryzokuken commented Apr 1, 2018

BridgeAR commented Apr 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryzokuken commented Apr 1, 2018

addaleax commented Apr 1, 2018

ryzokuken commented Apr 1, 2018

addaleax left a comment

Choose a reason for hiding this comment

ryzokuken commented Apr 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpinca commented Apr 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anliting Apr 1, 2018 • edited Loading

Choose a reason for hiding this comment

anliting Apr 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryzokuken commented Apr 2, 2018

ryzokuken commented Apr 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryzokuken commented Apr 2, 2018

anliting commented Apr 2, 2018 • edited Loading

anliting commented Apr 2, 2018

ryzokuken commented Apr 2, 2018

anliting commented Apr 2, 2018

anliting commented Apr 7, 2018 • edited Loading

anliting commented Apr 7, 2018

ryzokuken commented Apr 7, 2018

anliting commented Apr 7, 2018 • edited Loading

ryzokuken commented Apr 7, 2018

addaleax commented Apr 7, 2018

ryzokuken commented Apr 7, 2018

addaleax commented Apr 7, 2018

ryzokuken commented Apr 7, 2018

anliting commented Apr 8, 2018

ryzokuken commented Apr 8, 2018 • edited Loading

anliting commented Apr 8, 2018

ryzokuken commented Apr 8, 2018

ryzokuken commented Apr 8, 2018

addaleax commented Apr 8, 2018

ryzokuken commented Apr 9, 2018

anliting commented Apr 9, 2018

ryzokuken commented Apr 23, 2018

anliting Apr 1, 2018 •

edited

Loading

anliting Apr 1, 2018 •

edited

Loading

anliting commented Apr 2, 2018 •

edited

Loading

anliting commented Apr 7, 2018 •

edited

Loading

anliting commented Apr 7, 2018 •

edited

Loading

ryzokuken commented Apr 8, 2018 •

edited

Loading