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

Jump to bottom

crypto.randomBytes shows a confusing error #15118

Closed
benjamingr opened this issue Aug 31, 2017 · 2 comments
Closed

crypto.randomBytes shows a confusing error #15118

benjamingr opened this issue Aug 31, 2017 · 2 comments
Labels
crypto Issues and PRs related to the crypto subsystem. discuss Issues opened for discussions and feedbacks.

Comments

@benjamingr
Copy link
Member

benjamingr commented Aug 31, 2017 •
edited
Loading

Reproduced on Node.js 8.4 on Mac and 8.2 on Linux.

Currently, we show the following error when crypto.randomBytes is called with a non-integer:

> crypto.randomBytes(19660.79999999999)
TypeError: size must be a number >= 0
    at TypeError (native)

Which is confusing, since it is in fact a number bigger than 0.

To add to the confusion, we ran into this issue myself when using a third party package (myspeed) using another package (noisegen) that did division that should probably have been an integer number but wasn't.

We should either:

  • Fix the error message
  • Fix the API to accept a non-integer. By changing the check to check if a number is passed and coerce it to a UInt32. Personally I'm in favor of that.

Pinging @nodejs/crypto and @mrbar42 who discovered this

Would love it if some people weighed in.
@benjamingr benjamingr added the crypto Issues and PRs related to the crypto subsystem. label Aug 31, 2017
@joyeecheung
Copy link
Member

FWIW:

> Buffer.alloc(19660.79999999999).length
19660

so I think option 2 would be less surprising.

@joyeecheung joyeecheung added the discuss Issues opened for discussions and feedbacks. label Sep 1, 2017
@benjamingr benjamingr mentioned this issue Sep 1, 2017
Closed
3 tasks
@benjamingr
Copy link
Member Author

Went ahead with the truncating behavior in #15130 PTAL 🎉

benjamingr pushed a commit to benjamingr/io.js that referenced this issue Sep 1, 2017
crypto: accept decimal Number in randomBytes
59c11c6 
This change adds the ability to pass a double into randomBytes.

PR-URL: nodejs#15130
Fixes: nodejs#15118
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
addaleax pushed a commit to addaleax/ayo that referenced this issue Sep 5, 2017
@addaleax
crypto: accept decimal Number in randomBytes
75dd355 
This change adds the ability to pass a double into randomBytes.

PR-URL: nodejs/node#15130
Fixes: nodejs/node#15118
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@thepragmaticidealist thepragmaticidealist mentioned this issue Sep 3, 2018
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
crypto Issues and PRs related to the crypto subsystem. discuss Issues opened for discussions and feedbacks.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@benjamingr @joyeecheung