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.

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

Int32 overflow when creating a buffer #12553

Closed
rgrannell1 opened this issue Apr 20, 2017 · 6 comments
Closed

Int32 overflow when creating a buffer #12553

rgrannell1 opened this issue Apr 20, 2017 · 6 comments
Labels
confirmed-bug Issues with confirmed bugs. stream Issues and PRs related to the stream subsystem.

Comments

@rgrannell1
Copy link

I noticed that there's an unhandled overflow error in stream.Readable in work earlier today; when large values are supplied as highWaterMark the value used by the created object wraps around to a negative number. This didn't cause us massive problems, but it is a little difficult to debug (given that most people won't read the contents of _readableState)

console.log(
	require('stream').Readable({
		highWaterMark: 2147483647
	})._readableState.highWaterMark
)

console.log(
	require('stream').Readable({
		highWaterMark: 2147483647 + 1
	})._readableState.highWaterMark
)

which outputs:

2147483647
-2147483648

This should ideally throw an error, or support larger numeric values.

@cjihrig
Copy link
Contributor

cjihrig commented Apr 20, 2017

You may be running into these lines.

> ~~(2147483647)
2147483647
> ~~(2147483647 + 1)
-2147483648

@tniessen
Copy link
Member

@cjihrig Was about to write the same thing, appears to be the problem.

@mscdex mscdex added confirmed-bug Issues with confirmed bugs. stream Issues and PRs related to the stream subsystem. labels Apr 20, 2017
@tniessen
Copy link
Member

This should ideally throw an error, or support larger numeric values.

Thoughts on this? 2^31 is a fairly high value for highWaterMark. Is there any real-world use for bigger values?

@cjihrig
Copy link
Contributor

cjihrig commented Apr 22, 2017

I'd personally be in favor of some input validation, but I'd like to know what @nodejs/streams thinks.

@mcollina
Copy link
Member

I'm definitely +1 for input validation. That large value is really a bug, because you are essentially disabling backpressure, and you are exposing yourself to out-of-memory errors.

@mcollina
Copy link
Member

Fixed in #12593.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants