stream: fix highWaterMark integer overflow #12593

tniessen · 2017-04-22T19:15:14Z

Fixes integer overflows when supplying values exceeding 2 ^ 31 - 1 for highWaterMark.

Fixes: #12553

Checklist

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

Affected core subsystem(s)

stream

vsemozhetbyt · 2017-04-22T19:19:38Z

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

mscdex · 2017-04-22T19:22:30Z

Instead of using ~~ to cast to integer, why not use something like Math.trunc() that will still convert to integer but allows larger numbers (>32 bits)? That should be a much simpler change.

cjihrig · 2017-04-22T19:23:34Z

test/parallel/test-regress-GH-12593.js

@@ -0,0 +1,16 @@
+'use strict';


Could you add this to an appropriate existing test or give the file a better name. regress-GH-*.js names aren't really helpful for people working on the tests in the future.

I could not find an existing test file matching this issue, so pick a new name?

the convention would be then test-streams-highwatermark.js or similar

cjihrig · 2017-04-22T19:24:19Z

test/parallel/test-regress-GH-12593.js

+
+assert.throws(() => {
+  stream.Readable({
+    highWaterMark: 'foo'


Can you test a wider range of "bad" inputs.

cjihrig · 2017-04-22T19:25:12Z

test/parallel/test-regress-GH-12593.js

+const stream = require('stream');
+
+assert.throws(() => {
+  stream.Readable({


There should be tests for Writable too.

cjihrig · 2017-04-22T19:26:35Z

lib/_stream_readable.js

+    }
+
+    if (~~hwm !== hwm) {
+      throw new Error('highWaterMark exceeds valid range');


This should be a RangeError. It might be helpful to say what the valid range actually is.

cjihrig · 2017-04-22T19:27:19Z

lib/_stream_readable.js

-  this.highWaterMark = (hwm || hwm === 0) ? hwm : defaultHwm;
+  if (hwm != null) {
+    if (typeof hwm !== 'number') {
+      throw new TypeError('highWaterMark must be a number');


I think we're standardizing on putting the offending data's name in double quotes. So "highWaterMark" in this case.

This raises a good question... at some point we will need these errors to work with internals/errors but we need to make sure we are coordinating with the readable-streams folks.

vsemozhetbyt · 2017-04-22T19:28:01Z

test/parallel/test-regress-GH-12593.js

@@ -0,0 +1,16 @@
+'use strict';
+const common = require('../common');


Linter: 'common' is assigned a value but never used. This can be just:

require('../common');

tniessen · 2017-04-23T12:07:51Z

@mscdex These changes are based on the discussion below #12553 which suggested to disallow exceeding values. If it is consensus that larger values than 2GB should be supported, I will adapt the PR accordingly.

mscdex · 2017-04-23T14:35:51Z

@tniessen I could see throwing on negative values, but I think most people would expect larger values to just work (as #12553 shows). Separating out the two changes would allow just the negative value check to be semver-major while the Math.trunc() fix could be backported as a bug fix (IMHO).

tniessen · 2017-04-23T15:02:11Z

@mscdex Should I use this PR to fix the bug using Math.trunc() and another to check against negative values (and invalid types)?

mscdex · 2017-04-23T15:18:08Z

@tniessen It might be good to hear from other @nodejs/collaborators about my proposed solution first.

mcollina · 2017-04-23T16:55:43Z

I'm on vacation untill the end of this week.

This should be reviewed by @nodejs/streams. If someone else from the wg has time, we need to check this deeply as we do not want to break the ecosystem.

Generically, I'm -1 on making a semver-major out of this, so we should stick to throwing on negative values.

mscdex · 2017-04-23T17:34:16Z

@mcollina Throwing where we previously didn't would be semver-major I think. At least I believe that's been the general trend in the past.

mcollina · 2017-04-23T20:26:09Z

I think if highWaterMark is < 0, none of the stream machinery would work. I am without a laptop, so I need to check this. I would classify this as a bug fix, as the original code would fail silently.

I'm -1 on pumping semver-major for this right now, because of the maintenance burden it will take on readable-stream.

tniessen · 2017-05-03T09:43:26Z

Any updates?

mcollina · 2017-05-03T10:18:31Z

Any negative value is interpreted by the stream machinery as a 0, which makes this bug tricky: a user is putting in a value that is extremely high with the intent of "buffer forever", and instead it gets a stream that is always flowing.

I think we should not throw in this case, but instead cast the highWaterMark to 2147483647 if there is an overflow. Also, I would normalize any negative value to 0. Those should not be semver-major changes.

calvinmetcalf · 2017-05-03T12:35:25Z

lib/_stream_readable.js

+      throw new TypeError('highWaterMark must be a number');
+    }
+
+    if (~~hwm !== hwm) {


is this the way we do things other places in node?

It is how the old implementation converted hvm to a 32-bit integer. This is open for discussion, see this comment by @mscdex.

calvinmetcalf · 2017-05-03T12:45:07Z

I think we should not throw in this case, but instead cast the highWaterMark to 2147483647 if there is an overflow. Also, I would normalize any negative value to 0. Those should not be semver-major changes.

I think buffer.kMaxLength is the actual upper bounds for how large hwm could be

mscdex · 2017-05-03T17:28:22Z

I think buffer.kMaxLength is the actual upper bounds for how large hwm could be

Why is that? I would think the only time that particular constant would matter is if a user tried to .read(n) a very large number of bytes that exceeded that constant and there was enough buffered data to actually cover that kind of length. Otherwise such a single, large Buffer shouldn't ever be created.

mcollina · 2017-05-04T07:37:06Z

I would think the only time that particular constant would matter is if a user tried to .read(n) a very large number of bytes that exceeded that constant and there was enough buffered data to actually cover that kind of length. Otherwise such a single, large Buffer shouldn't ever be created.

Currently hightWaterMark is going from 2147483647 to -2147483647.

mscdex · 2017-05-04T07:41:57Z

Currently hightWaterMark is going from 2147483647 to -2147483647.

I don't understand this comment. What does that have to do with potentially using buffer.kMaxLength as the upper bound? Right now it's just coincidence that buffer.kMaxLength and ~~ produce the same max value.

Anyway, I think we should still allow larger values via Math.trunc().

reqshark · 2017-05-04T07:55:55Z

lib/_stream_readable.js

@@ -68,11 +68,19 @@ function ReadableState(options, stream) {
  // the point at which it stops calling _read() to fill the buffer
  // Note: 0 is a valid value, means "don't call _read preemptively ever"
  var hwm = options.highWaterMark;
-  var defaultHwm = this.objectMode ? 16 : 16 * 1024;
-  this.highWaterMark = (hwm || hwm === 0) ? hwm : defaultHwm;
+  if (hwm != null) {


why the loose null check? why not do if (hwm) { // ... ?

I guess because hwm can be 0 (zero)

a zero watermark doesn't make sense, anything less than 1 is nothing getting processed.

also why delete defaultHwm?

a 0 is allowed for comparability reasons

a 0 is allowed for comparability reasons

please embellish, what/when does that comparison happen?

I did not want to change the behavior of accepting 0. I just deleted defaultHwm because I think it is pretty obvious what 16 * 1024 is in the new code, though I can use a variable as well.

tniessen · 2017-05-04T08:22:36Z

Without further discussions of the code, are there any objections to @mscdex suggestion? Otherwise, I am going to split this into two PRs as suggested.

mcollina · 2017-05-05T08:44:34Z

@tniessen go ahead and implement @mscdex suggestion.

Fixes integer overflows when supplying values exceeding 2 ^ 31 - 1 for highWaterMark.

tniessen · 2017-05-10T13:50:18Z

@mscdex @cjihrig @mcollina @calvinmetcalf I force-pushed a commit which only contains the integer overflow fix.

cjihrig

LGTM with one request.

cjihrig · 2017-05-10T14:00:17Z

test/parallel/test-streams-highwatermark.js

+const assert = require('assert');
+const stream = require('stream');
+
+const readable = stream.Readable({ highWaterMark: 2147483648 });


Can you move all of the 2147483648s into a variable, and add a comment on why that specific value was chosen. That should prevent anyone from changing it to a small value in the future.

I think a better value to test is Number.MAX_SAFE_INTEGER.

@mscdex Done.

mcollina

LGTM, dropping this to semver-minor

mcollina · 2017-05-10T15:19:10Z

I'll be landing this tomorrow (11th of May), if no one objects.

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

edit: 11th of May

tniessen · 2017-05-10T15:39:09Z

Thanks @mcollina. I will create a separate PR with the semver-major changes after this gets merged.

mscdex · 2017-05-10T15:52:34Z

I'll be landing this tomorrow (10th of May), if no one objects.

Today is the 10th ;-)

Anyway, here is CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/772/

mcollina · 2017-05-10T16:01:25Z

Updated with the correct date, thanks @mscdex.

mcollina · 2017-05-10T16:45:17Z

I think the citgm failures are recurring, can someone confirm?

tniessen · 2017-05-10T22:16:40Z

@mcollina I don't see any relations between the reported errors and this PR. Are there known problems with citgm?

lucamaraschi

LGTM
@tniessen can you please add the general description of the test as indicated in the general guidelines?

tniessen · 2017-05-11T15:03:06Z

@lucamaraschi Done.

tniessen · 2017-05-13T11:52:09Z

@mscdex @mcollina Thoughts about citgm? Was this tested on master? I built 57a08e2 (which was the last commit on master before I branched) and tried npm test on node-ftp (which failed on citgm). I was able to reproduce the error without my PR.

mscdex · 2017-05-14T13:21:19Z

lib/_stream_readable.js

@@ -72,7 +72,7 @@ function ReadableState(options, stream) {
  this.highWaterMark = (hwm || hwm === 0) ? hwm : defaultHwm;

  // cast to ints.
-  this.highWaterMark = ~~this.highWaterMark;
+  this.highWaterMark = Math.trunc(this.highWaterMark);


Actually, I think we may want to use Math.floor() instead, it seems to be significantly faster (~6x at least on V8 5.8).

@tniessen can you update that?

thefourtheye · 2017-05-15T15:08:20Z

lib/_stream_readable.js

@@ -72,7 +72,7 @@ function ReadableState(options, stream) {
  this.highWaterMark = (hwm || hwm === 0) ? hwm : defaultHwm;

  // cast to ints.
-  this.highWaterMark = ~~this.highWaterMark;
+  this.highWaterMark = Math.floor(this.highWaterMark);


Minor nit: This changes the result when passed a negative floating point number.

@thefourtheye what do you suggest? Can you make an example?
This was just changed at @mscdex suggestion from Math.trunc.

All negative values are interpreted as 0 by the machinery, so nothing should change.

@mcollina Oh thats okay. This is just a nit. I am just making an observation here.

It'll be even less of an issue once we start throwing on invalid values.

This is correct, but not an actual problem. According to @mcollina:

Any negative value is interpreted by the stream machinery as a 0

Therefore, it does not matter whether the value is Math.floor(-233.5) = -234 or ~~(-233.5) = -233, not even in the special case of Math.floor(-0.5) = -1 whilst ~~(-0.5) = 0.

We are planning to throw on negative values anyway.

Edit: Sorry, the last three messages were invisible to me until now, I did not know this had already been discussed with @mcollina and @mscdex.

mcollina · 2017-05-16T12:03:03Z

Landed as 11918c4

Fixes integer overflows when supplying values exceeding MAX_SAFE_INTEGER for highWaterMark. PR-URL: #12593 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luca Maraschi <luca.maraschi@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Adds checks for the type of "highWaterMark" and restricts it to non-negative values. Refs: nodejs#12593

Fixes integer overflows when supplying values exceeding MAX_SAFE_INTEGER for highWaterMark. PR-URL: nodejs#12593 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luca Maraschi <luca.maraschi@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

gibfahn · 2018-01-15T21:32:14Z

Release team opted not to backport to v6.x, as it seems there is a chance it might break things, and we haven't seen any requests for it. If there are reasons to backport let us know.

nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Apr 22, 2017

cjihrig requested changes Apr 22, 2017

View reviewed changes

vsemozhetbyt reviewed Apr 22, 2017

View reviewed changes

mcollina added the semver-major PRs that contain breaking changes and should be released in the next major version. label May 3, 2017

calvinmetcalf reviewed May 3, 2017

View reviewed changes

reqshark reviewed May 4, 2017

View reviewed changes

tniessen changed the title ~~stream: add highWaterMark validity checks~~ stream: fix highWaterMark integer overflow May 10, 2017

stream: fix highWaterMark integer overflow

9344665

Fixes integer overflows when supplying values exceeding 2 ^ 31 - 1 for highWaterMark.

tniessen force-pushed the fix-12553 branch from b206b98 to 9344665 Compare May 10, 2017 13:37

cjihrig approved these changes May 10, 2017

View reviewed changes

Use MAX_SAFE_INTEGER instead of 2^31-1

5e110e1

BridgeAR approved these changes May 10, 2017

View reviewed changes

mcollina approved these changes May 10, 2017

View reviewed changes

mcollina added semver-minor PRs that contain new features and should be released in the next minor version. and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels May 10, 2017

jasnell approved these changes May 10, 2017

View reviewed changes

jasnell approved these changes May 11, 2017

View reviewed changes

lucamaraschi approved these changes May 11, 2017

View reviewed changes

Add test description

94e6081

mscdex reviewed May 14, 2017

View reviewed changes

Use floor instead of trunc

a15dacd

thefourtheye reviewed May 15, 2017

View reviewed changes

mcollina closed this May 16, 2017

mcollina mentioned this pull request May 16, 2017

Int32 overflow when creating a buffer #12553

Closed

tniessen added a commit to tniessen/node that referenced this pull request May 16, 2017

stream: check highWaterMark type and range

b09ff75

Adds checks for the type of "highWaterMark" and restricts it to non-negative values. Refs: nodejs#12593

tniessen mentioned this pull request May 16, 2017

stream: check type and range of highWaterMark #13065

Closed

3 tasks

jasnell mentioned this pull request May 28, 2017

8.0.0 Release Proposal #12220

Closed

gibfahn added the dont-land-on-v6.x label Jan 15, 2018

		@@ -0,0 +1,16 @@
		'use strict';
		const common = require('../common');

stream: fix highWaterMark integer overflow #12593

stream: fix highWaterMark integer overflow #12593

Conversation

tniessen commented Apr 22, 2017 • edited Loading

Checklist

Affected core subsystem(s)

vsemozhetbyt commented Apr 22, 2017

mscdex commented Apr 22, 2017 • edited Loading

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

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

Choose a reason for hiding this comment

tniessen commented Apr 23, 2017

mscdex commented Apr 23, 2017

tniessen commented Apr 23, 2017 • edited Loading

mscdex commented Apr 23, 2017

mcollina commented Apr 23, 2017

mscdex commented Apr 23, 2017 • edited Loading

mcollina commented Apr 23, 2017

tniessen commented May 3, 2017

mcollina commented May 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calvinmetcalf commented May 3, 2017

mscdex commented May 3, 2017 • edited Loading

mcollina commented May 4, 2017

mscdex commented May 4, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tniessen commented May 4, 2017

mcollina commented May 5, 2017

tniessen commented May 10, 2017

cjihrig left a comment

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

mcollina left a comment

Choose a reason for hiding this comment

mcollina commented May 10, 2017 • edited Loading

tniessen commented May 10, 2017 • edited Loading

mscdex commented May 10, 2017

mcollina commented May 10, 2017

mcollina commented May 10, 2017

tniessen commented May 10, 2017

lucamaraschi left a comment

Choose a reason for hiding this comment

tniessen commented May 11, 2017

tniessen commented May 13, 2017

mscdex May 14, 2017 • edited Loading

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

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

tniessen May 15, 2017 • edited Loading

Choose a reason for hiding this comment

mcollina commented May 16, 2017

gibfahn commented Jan 15, 2018

tniessen commented Apr 22, 2017 •

edited

Loading

mscdex commented Apr 22, 2017 •

edited

Loading

tniessen commented Apr 23, 2017 •

edited

Loading

mscdex commented Apr 23, 2017 •

edited

Loading

mscdex commented May 3, 2017 •

edited

Loading

mscdex commented May 4, 2017 •

edited

Loading

mcollina commented May 10, 2017 •

edited

Loading

tniessen commented May 10, 2017 •

edited

Loading

mscdex May 14, 2017 •

edited

Loading

tniessen May 15, 2017 •

edited

Loading