镜像站点 · 本页由第三方 GitHub 只读镜像提供,非 GitHub 官方站点,不接受任何登录或凭据输入。前往 github.com
Skip to content

crypto: avoid reusing randomInt() cache bytes - #66597

Open
gengjiawen wants to merge 1 commit into
nodejs:mainfrom
gengjiawen:crypto-randomint-cache-reuse
Open

gengjiawen wants to merge 1 commit into
nodejs:mainfrom
gengjiawen:crypto-randomint-cache-reuse

Conversation

@gengjiawen

Copy link
Copy Markdown
Member

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

The asynchronous refill now fills a separate buffer, allocated on first use and reused after that. The callback copies it into the cache before resetting the offset. Synchronous calls and the threadpool never touch the same memory, and resetting the offset only hands out bytes nobody has used. The synchronous path is unchanged. The asynchronous path does one extra 6 KiB copy per refill (every 1024 values).

The new test makes the first randomInt() call in a process asynchronous, waits for the refill job to finish, makes synchronous calls, and checks that the async value and the values after the callback differ from the synchronous ones. Without the fix it failed in every run I tried.

Fixes: #66595
Refs: #35110

AI disclosure: written with the help of Claude (claude:opus-5.5). I checked the bug against the source and the latest nightly, and reviewed the fix and the test.

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: nodejs#66595
Refs: nodejs#35110
Signed-off-by: Jiawen Geng <technicalcute@gmail.com>
Assisted-by: claude:opus-5.5
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/crypto

@nodejs-github-bot nodejs-github-bot added crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. labels Oct 8, 2026
@codecov

codecov Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.46%. Comparing base (c3189d4) to head (59e0b84).
⚠️ Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #66597      +/-   ##
==========================================
+ Coverage   90.45%   90.46%   +0.01%     
==========================================
  Files         791      791              
  Lines      276562   276571       +9     
  Branches    53111    53118       +7     
==========================================
+ Hits       250154   250191      +37     
+ Misses      16801    16782      -19     
+ Partials     9607     9598       -9     
Files with missing lines Coverage Δ
lib/internal/crypto/random.js 96.45% <100.00%> (+0.04%) ⬆️

... and 24 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

crypto.randomInt() can return the same value to a sync and an async caller

2 participants