Skip to content

Commit

Permalink
crypto: migrate crypto.randomBytes to internal/errors
Browse files Browse the repository at this point in the history
PR-URL: #16454
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
  • Loading branch information
jasnell committed Oct 26, 2017
1 parent 76b8803 commit 0a03e35
Show file tree
Hide file tree
Showing 3 changed files with 36 additions and 15 deletions.
11 changes: 9 additions & 2 deletions lib/internal/crypto/random.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
const errors = require('internal/errors');
const { isArrayBufferView } = require('internal/util/types');
const {
randomBytes,
randomBytes: _randomBytes,
randomFill: _randomFill
} = process.binding('crypto');

Expand All @@ -24,7 +24,7 @@ function assertOffset(offset, length) {
}
}

function assertSize(size, offset, length) {
function assertSize(size, offset = 0, length = Infinity) {
if (typeof size !== 'number' || size !== size) {
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'size', 'number');
}
Expand All @@ -38,6 +38,13 @@ function assertSize(size, offset, length) {
}
}

function randomBytes(size, cb) {
assertSize(size);
if (cb !== undefined && typeof cb !== 'function')
throw new errors.TypeError('ERR_INVALID_CALLBACK');
return _randomBytes(size, cb);
}

function randomFillSync(buf, offset = 0, size) {
if (!isArrayBufferView(buf)) {
throw new errors.TypeError('ERR_INVALID_ARG_TYPE',
Expand Down
7 changes: 1 addition & 6 deletions src/node_crypto.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5526,13 +5526,8 @@ void RandomBytesProcessSync(Environment* env,
void RandomBytes(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);

if (!args[0]->IsNumber() || args[0].As<v8::Number>()->Value() < 0) {
return env->ThrowTypeError("size must be a number >= 0");
}

const int64_t size = args[0]->IntegerValue();
if (size > Buffer::kMaxLength)
return env->ThrowTypeError("size must be a uint32");
CHECK(size <= Buffer::kMaxLength);

Local<Object> obj = env->randombytes_constructor_template()->
NewInstance(env->context()).ToLocalChecked();
Expand Down
33 changes: 26 additions & 7 deletions test/parallel/test-crypto-random.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,26 @@ crypto.DEFAULT_ENCODING = 'buffer';
// bump, we register a lot of exit listeners
process.setMaxListeners(256);

const expectedErrorRegexp = /^TypeError: size must be a number >= 0$/;
[crypto.randomBytes, crypto.pseudoRandomBytes].forEach(function(f) {
[-1, undefined, null, false, true, {}, []].forEach(function(value) {
assert.throws(function() { f(value); }, expectedErrorRegexp);
assert.throws(function() { f(value, common.mustNotCall()); },
expectedErrorRegexp);

common.expectsError(
() => f(value),
{
code: 'ERR_INVALID_ARG_TYPE',
type: TypeError,
message: /^The "size" argument must be of type (number|uint32)$/
}
);

common.expectsError(
() => f(value, common.mustNotCall()),
{
code: 'ERR_INVALID_ARG_TYPE',
type: TypeError,
message: /^The "size" argument must be of type (number|uint32)$/
}
);
});

[0, 1, 2, 4, 16, 256, 1024, 101.2].forEach(function(len) {
Expand Down Expand Up @@ -464,9 +478,14 @@ const expectedErrorRegexp = /^TypeError: size must be a number >= 0$/;

// #5126, "FATAL ERROR: v8::Object::SetIndexedPropertiesToExternalArrayData()
// length exceeds max acceptable value"
assert.throws(function() {
crypto.randomBytes((-1 >>> 0) + 1);
}, /^TypeError: size must be a uint32$/);
common.expectsError(
() => crypto.randomBytes((-1 >>> 0) + 1),
{
code: 'ERR_INVALID_ARG_TYPE',
type: TypeError,
message: 'The "size" argument must be of type uint32'
}
);

[1, true, NaN, null, undefined, {}, []].forEach((i) => {
common.expectsError(
Expand Down

5 comments on commit 0a03e35

@Trott
Copy link
Member

@Trott Trott commented on 0a03e35 Oct 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First line longer than 50 chars.

@jasnell If this is an intentional violation of that rule, perhaps we should consider a PR to change that particular requirement to a guideline rather than a rule.

@jasnell
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I've been strongly considering proposing we relax that a bit. I understand why it exists but it's extremely difficult to work with at times

@Trott
Copy link
Member

@Trott Trott commented on 0a03e35 Oct 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I've been strongly considering proposing we relax that a bit. I understand why it exists but it's extremely difficult to work with at times

In the spirit of "leaders should be held to at least the same standard as everyone else, if not a higher standard", I think we're obligated to at least open a PR to suggest such a change if we're not going to comply with it fastidiously ourselves.

@apapirovski
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Trott see #16523 :)

@Trott
Copy link
Member

@Trott Trott commented on 0a03e35 Oct 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, my comment was merely 13 hours too late. :-D

Please sign in to comment.