crypto: accept decimal Number in randomBytes #15130

benjamingr · 2017-09-01T13:21:29Z

This change adds the ability to pass a double into randomBytes based on feedback from #15118 which aligns the behavior with other core methods like setTimeout

Marking as semver-major out of caution but generally I think this is patch.

No docs were changed because this actually aligns with the documented behavior better.

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

Affected core subsystem(s)

crypto

evanlucas · 2017-09-01T14:27:33Z

src/node_crypto.cc

@@ -5616,13 +5616,13 @@ void RandomBytesProcessSync(Environment* env,
 void RandomBytes(const FunctionCallbackInfo<Value>& args) {
  Environment* env = Environment::GetCurrent(args);

-  if (!args[0]->IsUint32()) {
+  if (!args[0]->IsNumber() || args[0]->NumberValue() < 0) {


I wonder if the args[0]->NumberValue() has a performance impact here? Could you not use args[0].As<Number>?

@evanlucas did you accidentally add a not in? Would you like me to change it to as<Number>?

for the record NumberValue does if (obj->IsNumber()) return Just(obj->Number());

No, I thought it had come up a few times that .As() is faster than *Value(), but could be wrong. I haven't done any performance testing on the difference though. Being that this is used for things like uuid generation, checking which is faster would probably be a good thing though.

NumberValue() has a fast path for things that are already numbers, so it won’t be a huge difference. In any case, not compared to the work of actually creating these numbers.

The overload of NumberValue() that doesn’t take a context argument is deprecated, by the way; you should use the one that does. (The reason being that NumberValue can actually execute arbitrary code via @@toPromitive if the input isn’t already a number, but in this case that won’t happen).

I personally would really prefer args[0].As<Number> since you’re already checking that args[0] is a Number instance.

evanlucas · 2017-09-01T14:28:10Z

src/node_crypto.cc

-  if (size < 0 || size > Buffer::kMaxLength)
-    return env->ThrowRangeError("size is not a valid Smi");
+  if (size > Buffer::kMaxLength)
+    return env->ThrowTypeError("size must be a uint32");


I'm +1 on this change, but wouldn't feel comfortable with it not being semver-major since it changes an error message

Wait, why is the RangeError changed to a TypeError?

Good question: A range error made no sense there - and the code path emitting it didn't happen in reasonable cases anyway. For example if you passed -1.1 you'd get the TypeError in the path above - so not changing this to a TypeError would break a lot more.

Hmm, I am not sure I totally buy the argument of breaking less code here. Since we are changing the error messages here, why not make everything Semantically Correct(r)?

-1.1: RangeError: size must be a uint32

2⁴⁰: RangeError: size must be a uint32

'hello': TypeError: size must be a number

I'm not saying that a TypeError is more correct here, I'll try to explain what I mean by breakage:

Before both -1.1 and 2⁴⁰ were TypeErrors since ->IsUint32 returns false and this change would make them both RangeErrors.

A RangeError would only happen if the number was a Uint32 and it's smaller than Buffer::kMaxLength which would only happen if the number was bigger than 2147483647 which is never. Only on 32 bit architecture buffers are max sized at 1073741823 which means RangeErrors would only happen on 32 bit and only when the number passed was between 1073741823 and 2147483647 - which I guess is very rare.

I'm fine with breaking this in a semver-major, but I'd rather do that in a different PR titled "make invalid numbers throw RangeError in randomBytes so it can be specifically examined for that change.

That's fair 👍

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>

jasnell

LGTM with green CI

benjamingr · 2017-09-01T17:47:42Z

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

macOS failures unrelated, tests pass on a macOS vm locally.

benjamingr · 2017-09-04T07:12:18Z

Landed in 484bfa2 - thanks for all the reviews

This change adds the ability to pass a double into randomBytes. PR-URL: #15130 Fixes: #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>

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>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. labels Sep 1, 2017

benjamingr added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 1, 2017

benjamingr changed the title ~~crypto: accept Number with decimal point in randomBytes~~ crypto: accept decimal Number in randomBytes Sep 1, 2017

benjamingr force-pushed the randomBytes-double branch from 089a33a to fc02c9f Compare September 1, 2017 13:24

benjamingr mentioned this pull request Sep 1, 2017

crypto.randomBytes shows a confusing error #15118

Closed

evanlucas reviewed Sep 1, 2017

View reviewed changes

sam-github approved these changes Sep 1, 2017

View reviewed changes

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>

benjamingr force-pushed the randomBytes-double branch from fc02c9f to 59c11c6 Compare September 1, 2017 15:54

jasnell approved these changes Sep 1, 2017

View reviewed changes

evanlucas approved these changes Sep 1, 2017

View reviewed changes

addaleax approved these changes Sep 1, 2017

View reviewed changes

joyeecheung approved these changes Sep 2, 2017

View reviewed changes

BridgeAR approved these changes Sep 2, 2017

View reviewed changes

lpinca approved these changes Sep 2, 2017

View reviewed changes

tniessen approved these changes Sep 2, 2017

View reviewed changes

TimothyGu approved these changes Sep 3, 2017

View reviewed changes

benjamingr closed this Sep 4, 2017

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

crypto: accept decimal Number in randomBytes #15130

crypto: accept decimal Number in randomBytes #15130

benjamingr commented Sep 1, 2017 •

edited

Loading

evanlucas Sep 1, 2017

benjamingr Sep 1, 2017

evanlucas Sep 1, 2017

addaleax Sep 1, 2017 •

edited

Loading

evanlucas Sep 1, 2017

TimothyGu Sep 2, 2017

benjamingr Sep 2, 2017

TimothyGu Sep 2, 2017

benjamingr Sep 2, 2017 •

edited

Loading

TimothyGu Sep 3, 2017

jasnell left a comment

benjamingr commented Sep 1, 2017 •

edited

Loading

benjamingr commented Sep 4, 2017

crypto: accept decimal Number in randomBytes #15130

crypto: accept decimal Number in randomBytes #15130

Conversation

benjamingr commented Sep 1, 2017 • edited Loading

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax Sep 1, 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

benjamingr Sep 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell left a comment

Choose a reason for hiding this comment

benjamingr commented Sep 1, 2017 • edited Loading

benjamingr commented Sep 4, 2017

benjamingr commented Sep 1, 2017 •

edited

Loading

addaleax Sep 1, 2017 •

edited

Loading

benjamingr Sep 2, 2017 •

edited

Loading

benjamingr commented Sep 1, 2017 •

edited

Loading