fix(crypto): run WebCrypto digest + async randomBytes on a macrotask like Node#6430
Conversation
…like Node
Node runs `crypto.subtle.digest` and the callback form of
`crypto.randomBytes(size, cb)` on the libuv threadpool, so `await`ing them
observably yields a macrotask — the promise/callback settles on a later
event-loop iteration, never synchronously. Perry computed the bytes eagerly and
resolved synchronously (+0 hops), so an `await`ing caller continued a full
event-loop iteration ahead of Node.
That timing is observable. Auth.js v5 hashes its CSRF token with
`subtle.digest`, so a Next.js Server Component's `await auth()` finished a
macrotask early under Perry, which collapsed React's Flight (RSC) streaming
waves and renumbered the serialized rows of the response versus Node.
Fix: both now schedule their settlement through `setImmediate` (the timer/
callback queue) instead of resolving inline. The bytes are still produced
eagerly; only the callback/promise dispatch is deferred, so values are
unchanged. `subtle.digest` re-arms once (Node's threadpool digest yields ~2
setImmediate ticks); `randomBytes` defers one tick. The deferred Promise/Buffer
survive GC via the timer root scanner (`scan_timer_roots`).
Verified: digest and randomBytes values are byte-identical to Node
(`sha256("hello")` etc.); a synchronous `createHash(...).digest()` still crosses
zero macrotasks; the promise/stream microtask-hop harnesses are unaffected.
Added test-files/test_gap_webcrypto_async_threadpool.ts pinning the observable
contract (async crypto crosses a macrotask, sync hash does not, values unchanged).
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesCryptographic async timing
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant js_crypto_random_bytes_async
participant js_webcrypto_digest
participant EventLoop
participant PromiseOrCallback
Caller->>js_crypto_random_bytes_async: request random bytes
js_crypto_random_bytes_async->>EventLoop: schedule callback with err and value
EventLoop->>PromiseOrCallback: invoke randomBytes callback
Caller->>js_webcrypto_digest: request digest
js_webcrypto_digest->>EventLoop: schedule digest settlement closure
EventLoop->>PromiseOrCallback: resolve digest Promise
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test-files/test_gap_webcrypto_async_threadpool.ts (1)
1-11: 📐 Maintainability & Code Quality | 🔵 TrivialUse the pinned Node runtime for this gap test. The run here used v24.15.0, so it doesn’t match the .node-version oracle (26.5.0).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test-files/test_gap_webcrypto_async_threadpool.ts` around lines 1 - 11, Update the gap test’s Node runtime configuration to use the pinned version from the project’s .node-version oracle, 26.5.0, instead of the currently used v24.15.0. Keep the existing crypto timing assertions and test logic unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perry-stdlib/src/crypto/random.rs`:
- Around line 91-99: Update js_crypto_random_bytes_async to validate that
callback is callable before extracting or queueing it through
js_set_immediate_callback_args. Preserve the existing error/value argument
construction for valid callbacks, and avoid forwarding non-function callback
bits that could be miscast as a ClosureHeader.
In `@crates/perry-stdlib/src/webcrypto/digest.rs`:
- Around line 49-66: Root the allocated Uint8Array and Promise in the digest
flow before performing subsequent heap allocations. In the code around
webcrypto_digest_settle, alloc_uint8array_from_slice, js_promise_new, and
js_closure_alloc, create the runtime’s equivalent RuntimeHandleScope immediately
after allocation and register both buf and promise with it so they remain live
while the closure and captures are created.
---
Nitpick comments:
In `@test-files/test_gap_webcrypto_async_threadpool.ts`:
- Around line 1-11: Update the gap test’s Node runtime configuration to use the
pinned version from the project’s .node-version oracle, 26.5.0, instead of the
currently used v24.15.0. Keep the existing crypto timing assertions and test
logic unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 49de3376-b556-4836-948d-3d0a41c84f4d
📒 Files selected for processing (3)
crates/perry-stdlib/src/crypto/random.rscrates/perry-stdlib/src/webcrypto/digest.rstest-files/test_gap_webcrypto_async_threadpool.ts
| // The digest bytes are computed eagerly, but the Promise settles on the | ||
| // next event-loop iteration (setImmediate) — Node runs the hash on the | ||
| // threadpool, so `await subtle.digest(...)` observably yields a macrotask. | ||
| let buf = alloc_uint8array_from_slice(&digest); | ||
| if buf.is_null() { | ||
| return reject_with_dom_exception("OperationError", "The operation failed"); | ||
| } | ||
| let value = f64::from_bits(JSValue::pointer(buf as *const u8).bits()); | ||
| let promise = perry_runtime::promise::js_promise_new(); | ||
| let promise_val = f64::from_bits(JSValue::pointer(promise as *const u8).bits()); | ||
| let cl = | ||
| perry_runtime::closure::js_closure_alloc(webcrypto_digest_settle as *const u8, 3); | ||
| perry_runtime::closure::js_closure_set_capture_ptr(cl, 0, promise_val.to_bits() as i64); | ||
| perry_runtime::closure::js_closure_set_capture_ptr(cl, 1, value.to_bits() as i64); | ||
| // Remaining macrotask hops (Node's threadpool digest = 2 setImmediate ticks). | ||
| perry_runtime::closure::js_closure_set_capture_ptr(cl, 2, 2); | ||
| perry_runtime::timer::js_set_immediate_callback(cl as i64); | ||
| promise |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
Root heap allocations to prevent use-after-free.
The raw pointers buf and promise are left unrooted during the JS heap allocations for js_promise_new and js_closure_alloc. If a garbage collection occurs during these allocations, the unrooted objects may be swept, resulting in the closure capturing dangling pointers.
Please secure these pointers using a RuntimeHandleScope or the runtime's equivalent rooting mechanism immediately after allocation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-stdlib/src/webcrypto/digest.rs` around lines 49 - 66, Root the
allocated Uint8Array and Promise in the digest flow before performing subsequent
heap allocations. In the code around webcrypto_digest_settle,
alloc_uint8array_from_slice, js_promise_new, and js_closure_alloc, create the
runtime’s equivalent RuntimeHandleScope immediately after allocation and
register both buf and promise with it so they remain live while the closure and
captures are created.
Use JSValue::is_pointer() + js_nanbox_get_pointer for the callback closure instead of a raw `bits & mask + >= 0x1000` floor check, so the addr-class ratchet doesn't gain a handle-floor site. Behavior unchanged (randomBytes callback still deferred one setImmediate tick; verified +1 hop, values correct).
The comment said "only a genuine closure is a schedulable callback" but the
guard tested `is_pointer()`, which also accepts a non-function object/array
(`randomBytes(n, {})`). Match the guard to its stated intent and to every other
node-style-callback site (`is_closure_ptr` / `is_callable_value`): validate the
CLOSURE_MAGIC at the source instead of relying on the timer's downstream check.
Not a crash fix — `js_closure_call2` already routes through
`get_valid_func_ptr`, which range-checks the address and rejects a non-magic
tag, so a mis-typed callback was already a safe no-op (verified: the pre-change
`is_pointer` build survives `randomBytes(16, {})` / `[..]` / `42`). This just
rejects it one layer earlier and keeps the guard honest. Node throws
ERR_INVALID_ARG_TYPE for these; matching that (vs the current silent no-op,
which predates this PR) is a separate parity item across all crypto async
callbacks.
|
Addressed CodeRabbit's callback-validation finding in The finding is real in the narrow sense — the guard was But it does not actually crash, because the timer's dispatch already self-defends. So no miscast-and-crash — the object pointer reads a valid offset, finds a non-magic tag, and no-ops. (This differs from an uninitialized-read bug like #6429; here the read target is always a mapped, valid allocation.) The change switches the guard to
Node throws |
…ck guard The address-classification ratchet flagged `cb_ptr >= 0x10000` as a bare band literal. It is redundant anyway: `is_closure_ptr` opens with `if is_handle_band(ptr) return false` and bounds the address before the CLOSURE_MAGIC probe, so it already rejects the whole handle band and any non-heap value. Call it directly.
1 participant
Summary
Node runs
crypto.subtle.digestand the callback form ofcrypto.randomBytes(size, cb)on the libuv threadpool, soawaiting them observably yields a macrotask — the promise/callback settles on a later event-loop iteration, never synchronously. Perry computed the bytes eagerly and resolved synchronously (+0 hops), so anawaiting caller continued a full event-loop iteration ahead of Node.Why it's observable
Auth.js v5 hashes its CSRF token with
subtle.digest. In a Next.js Server Component,session = await auth()therefore finished a macrotask early under Perry, which collapsed React's Flight (RSC) streaming waves and renumbered the serialized rows of the streamed response versus Node. On a real Next.js 16 app this took the landing route from 5 to 7__next_fhydration pushes — i.e. it was the difference between a byte-divergent and (all but the last few bytes) byte-identical RSC payload.Fix
Both now schedule their settlement through
setImmediateinstead of resolving inline:crypto.subtle.digestreturns a pending promise resolved from a native closure scheduled on the timer/immediate queue; it re-arms once, since Node's threadpool digest yields ~2setImmediateticks.crypto.randomBytes(size, cb)defers its(err, buf)callback by onesetImmediatetick.The bytes are still produced eagerly — only the callback/promise dispatch is deferred, so values are unchanged. The deferred Promise/Buffer survive GC via the timer root scanner (
scan_timer_roots).Testing
sha256("hello")=2cf24dba…, correct lengths).createHash(...).digest()still crosses zero macrotasks.test-files/test_gap_webcrypto_async_threadpool.tspinning the observable contract: async crypto crosses a macrotask, a sync hash does not, and values are unchanged. The exact hop count is threadpool/context-dependent in Node, so the test asserts the async contract rather than a fixed count.Summary by CodeRabbit
crypto.randomBytes()callback delivery andcrypto.subtle.digest()Promise settling to better match Node-style macrotask timing (completion is now deferred).crypto.subtle.digest()now rejects with anOperationError.