crypto: avoid reusing randomInt() cache bytes

When an asynchronous randomInt() call finds the cache empty, it queues
itself and starts randomFill() on the shared cache buffer. A synchronous
call made before that job completes refills the same buffer with
randomFillSync() and returns values from offset 0. The completion
callback then reset the offset to 0, so the queued call and later calls
returned the bytes that synchronous calls had already used. The
threadpool job and randomFillSync() could also write the buffer at the
same time.

Fill a separate buffer asynchronously and copy it into the cache once
the job is done.

Fixes: #66595
Refs: #35110
Signed-off-by: Jiawen Geng <technicalcute@gmail.com>
Assisted-by: claude:opus-5.5
PR-URL: #66597
Reviewed-By: Filip Skokan <panva.ip@gmail.com>

Author: gengjiawen

lib/internal/crypto/random.js

@@ -25,6 +25,7 @@ const {
   TypedArrayPrototypeGetByteLength,
   TypedArrayPrototypeGetLength,
   TypedArrayPrototypeGetSymbolToStringTag,
+  TypedArrayPrototypeSet,
   Uint8Array,
 } = primordials;
 
@@ -233,6 +234,8 @@ const RAND_MAX = 0xFFFF_FFFF_FFFF;
 // divisible by 6 because each attempt to obtain a random int uses 6 bytes.
 const randomCache = new FastBuffer(6 * 1024);
 let randomCacheOffset = randomCache.length;
+// Asynchronous refills write into this buffer, never into randomCache.
+let asyncRandomCache;
 let asyncCacheFillInProgress = false;
 const asyncCachePendingTasks = [];
 
@@ -313,13 +316,19 @@ function asyncRefillRandomIntCache() {
     return;
 
   asyncCacheFillInProgress = true;
-  randomFill(randomCache, (err) => {
+  // Synchronous calls may refill and read randomCache while this job runs.
+  // Filling a separate buffer and copying it in afterwards ensures that
+  // resetting the offset never hands out bytes they have already used.
+  asyncRandomCache ??= new FastBuffer(randomCache.length);
+  randomFill(asyncRandomCache, (err) => {
     asyncCacheFillInProgress = false;
 
     const tasks = asyncCachePendingTasks;
     const errorReceiver = err && ArrayPrototypeShift(tasks);
-    if (!err)
+    if (!err) {
+      TypedArrayPrototypeSet(randomCache, asyncRandomCache);
       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

test/parallel/test-crypto-randomint-cache.js

@@ -0,0 +1,43 @@
+// Flags: --expose-internals
+'use strict';
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+
+// Synchronous randomInt() calls made while an asynchronous cache refill is
+// pending must not cause the same random bytes to be returned twice.
+
+const assert = require('assert');
+const { randomInt } = require('crypto');
+const { internalBinding } = require('internal/test/binding');
+const { RandomBytesJob } = internalBinding('crypto');
+
+// With this range, every 6-byte draw except 0xffffffffffff is returned
+// unchanged, so a repeated value means repeated cache bytes.
+const max = 2 ** 48 - 1;
+const values = [];
+
+// The refill job calls ondone on the main thread after the threadpool has
+// written its buffer. Make the synchronous calls at that point, before the
+// refill callback in randomInt() runs.
+const { run } = RandomBytesJob.prototype;
+RandomBytesJob.prototype.run = common.mustCall(function() {
+  RandomBytesJob.prototype.run = run;
+  const { ondone } = this;
+  this.ondone = common.mustCall((...args) => {
+    for (let i = 0; i < 3; i++)
+      values.push(randomInt(max));
+    ondone(...args);
+  });
+  run.call(this);
+});
+
+// This is the first randomInt() call in the process, so the cache is empty.
+// The call is queued and an asynchronous refill starts.
+randomInt(max, common.mustSucceed((n) => {
+  values.push(n);
+  for (let i = 0; i < 3; i++)
+    values.push(randomInt(max));
+  assert.strictEqual(new Set(values).size, values.length,
+                     `duplicate values: ${values}`);
+}));