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

crypto: add buffering to randomInt #35110

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions benchmark/crypto/randomInt.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
'use strict';

const common = require('../common.js');
const { randomInt } = require('crypto');

const bench = common.createBenchmark(main, {
mode: ['sync', 'async-sequential', 'async-parallel'],
min: [-(2 ** 47) + 1, -10_000, -100],
max: [100, 10_000, 2 ** 47],
n: [1e3, 1e5]
});

function main({ mode, min, max, n }) {
if (mode === 'sync') {
bench.start();
for (let i = 0; i < n; i++)
randomInt(min, max);
bench.end(n);
} else if (mode === 'async-sequential') {
bench.start();
(function next(i) {
if (i === n)
return bench.end(n);
randomInt(min, max, () => {
next(i + 1);
});
})(0);
} else {
bench.start();
let done = 0;
for (let i = 0; i < n; i++) {
randomInt(min, max, () => {
if (++done === n)
bench.end(n);
});
}
}
}
87 changes: 62 additions & 25 deletions lib/internal/crypto/random.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

const {
Array,
ArrayPrototypeForEach,
ArrayPrototypePush,
ArrayPrototypeShift,
ArrayPrototypeSplice,
BigInt,
FunctionPrototypeBind,
FunctionPrototypeCall,
Expand Down Expand Up @@ -186,6 +190,13 @@ function randomFill(buf, offset, size, callback) {
// e.g.: Buffer.from("ff".repeat(6), "hex").readUIntBE(0, 6);
const RAND_MAX = 0xFFFF_FFFF_FFFF;

// Cache random data to use in randomInt. The cache size must be evenly
// divisible by 6 because each attempt to obtain a random int uses 6 bytes.
const randomCache = new FastBuffer(6 * 1024);
Copy link
Member

Choose a reason for hiding this comment

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

Consider using secureBuffer here to integrate with secure heap (see randomUUID's random cache and #36779).

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. We could do that, but UUIDs and random ints always result in insecure memory allocations anyway that mostly destroy any security guarantees. JavaScript is terrible for secure memory management.

let randomCacheOffset = randomCache.length;
let asyncCacheFillInProgress = false;
const asyncCachePendingTasks = [];
Copy link
Member

Choose a reason for hiding this comment

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

using an array for this is usually not performing as expected for high numbers. Using a linked list solves this problem.

Copy link
Member

Choose a reason for hiding this comment

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

we have fixed-queue which also might be appropriate?


// Generates an integer in [min, max) range where min is inclusive and max is
// exclusive.
function randomInt(min, max, callback) {
Copy link
Member

Choose a reason for hiding this comment

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

An idea for potential enhancement: consider adding options arg with disableEntropyCache setting to allow disabling the random cache. This way will make the API consistent with randomUUID (see #36729).

Expand Down Expand Up @@ -230,33 +241,59 @@ function randomInt(min, max, callback) {
// than or equal to 0 and less than randLimit.
const randLimit = RAND_MAX - (RAND_MAX % range);

if (isSync) {
// Sync API
while (true) {
const x = randomBytes(6).readUIntBE(0, 6);
if (x >= randLimit) {
// Try again.
continue;
}
return (x % range) + min;
// If we don't have a callback, or if there is still data in the cache, we can
// do this synchronously, which is super fast.
while (isSync || (randomCacheOffset < randomCache.length)) {
if (randomCacheOffset === randomCache.length) {
// This might block the thread for a bit, but we are in sync mode.
randomFillSync(randomCache);
randomCacheOffset = 0;
}

const x = randomCache.readUIntBE(randomCacheOffset, 6);
randomCacheOffset += 6;

if (x < randLimit) {
const n = (x % range) + min;
if (isSync) return n;
process.nextTick(callback, undefined, n);
return;
}
} else {
// Async API
const pickAttempt = () => {
randomBytes(6, (err, bytes) => {
if (err) return callback(err);
const x = bytes.readUIntBE(0, 6);
if (x >= randLimit) {
// Try again.
return pickAttempt();
}
const n = (x % range) + min;
callback(null, n);
});
};

pickAttempt();
}

// At this point, we are in async mode with no data in the cache. We cannot
// simply refill the cache, because another async call to randomInt might
// already be doing that. Instead, queue this call for when the cache has
// been refilled.
ArrayPrototypePush(asyncCachePendingTasks, { min, max, callback });
asyncRefillRandomIntCache();
}

function asyncRefillRandomIntCache() {
if (asyncCacheFillInProgress)
return;

asyncCacheFillInProgress = true;
randomFill(randomCache, (err) => {
asyncCacheFillInProgress = false;

const tasks = asyncCachePendingTasks;
const errorReceiver = err && ArrayPrototypeShift(tasks);
if (!err)
randomCacheOffset = 0;

// Restart all pending tasks. If an error occurred, we only notify a single
// callback (errorReceiver) about it. This way, every async call to
// randomInt has a chance of being successful, and it avoids complex
// exception handling here.
ArrayPrototypeForEach(ArrayPrototypeSplice(tasks, 0), (task) => {
randomInt(task.min, task.max, task.callback);
});

// This is the only call that might throw, and is therefore done at the end.
if (errorReceiver)
errorReceiver.callback(err);
});
}


Expand Down