node:crypto: free the async job ctx before invoking the completion callback - #36986
Conversation
…llback The 11 extern crypto job ctxs (generateKeyPair x5, sign, diffieHellman, hkdf, generatePrime, checkPrime, generateKey) invoked the JS callback from inside runFromJS while the C++ ctx was still alive; the ctx was only freed after then() returned. A callback that never returns (process.exit()) stranded everything the ctx still owned, e.g. the generated EVP_PKEY, which LeakSanitizer reports now that OPENSSL_malloc is on the libc heap under ASAN (6b6fb1a). Restructure the FFI contract so the C++ side never sees the callback: Bun__<Name>Ctx__runFromJS(ctx, global, callback) becomes Bun__<Name>Ctx__takeCallbackArgs(ctx, global, args[3]) -> argc. The AnyTaskJob plumbing produces the arguments, frees the ctx, then invokes the callback through the same event loop run_callback the C++ side called before. The ctx cannot own anything across user JS by construction, with unchanged invocation timing and semantics.
|
Updated 4:24 PM PT - Aug 5th, 2026
✅ @robobun, your commit 31bff9ba0e23ce336113f8e786a1512d7cb100ec passed in 🧪 To try this PR locally: bunx bun-pr 36986That installs a local version of the PR into your bun-36986 --bun |
|
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: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAsynchronous Node crypto jobs now return callback arguments through native bridges. The runtime extracts them, cleans up native contexts before JavaScript execution, and releases queued jobs during shutdown. Tests cover error propagation and ASAN process-exit behavior. ChangesCrypto callback argument extraction
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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
🤖 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 `@src/jsc/bindings/node/crypto/CryptoDhJob.cpp`:
- Around line 49-60: Update the success path in the CryptoDhJob callback after
WebCore::createBuffer to check scope.exception() immediately, and if an
exception is pending, encode and return it through the callback contract as done
in CryptoGenKeyPair.cpp. Only assign args[1] and return the successful
two-argument result when buffer creation completed without an exception.
In `@src/runtime/node/node_crypto_binding.rs`:
- Around line 117-127: The Err branch in the callback dispatch match must still
invoke the removed callback when argument extraction fails. Update the
ctx_take_callback_args handling to convert err via global.take_exception(err),
then call run_callback with callback, global, and the converted exception as the
first argument instead of reporting it as unhandled; preserve the existing
successful argc path.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4ea7ccf8-f817-4ca1-8c62-184e2996b956
📒 Files selected for processing (19)
src/jsc/bindings/node/crypto/CryptoDhJob.cppsrc/jsc/bindings/node/crypto/CryptoDhJob.hsrc/jsc/bindings/node/crypto/CryptoGenDhKeyPair.cppsrc/jsc/bindings/node/crypto/CryptoGenDsaKeyPair.cppsrc/jsc/bindings/node/crypto/CryptoGenEcKeyPair.cppsrc/jsc/bindings/node/crypto/CryptoGenKeyPair.cppsrc/jsc/bindings/node/crypto/CryptoGenKeyPair.hsrc/jsc/bindings/node/crypto/CryptoGenNidKeyPair.cppsrc/jsc/bindings/node/crypto/CryptoGenRsaKeyPair.cppsrc/jsc/bindings/node/crypto/CryptoHkdf.cppsrc/jsc/bindings/node/crypto/CryptoHkdf.hsrc/jsc/bindings/node/crypto/CryptoKeygen.cppsrc/jsc/bindings/node/crypto/CryptoKeygen.hsrc/jsc/bindings/node/crypto/CryptoPrimes.cppsrc/jsc/bindings/node/crypto/CryptoPrimes.hsrc/jsc/bindings/node/crypto/CryptoSignJob.cppsrc/jsc/bindings/node/crypto/CryptoSignJob.hsrc/runtime/node/node_crypto_binding.rstest/js/node/crypto/crypto.key-objects.test.ts
The exception check that used to happen inside the nested runCallbackN call now happens on the Rust side after the C++ ThrowScope destructs, so throw-sites in argument production (WebCore::createBuffer via createUninitialized, error construction) tripped BUN_JSC_validateExceptionChecks on the asan lane. Check the scope immediately after each call that can throw; a pending exception still skips the callback and is reported as unhandled, as before.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The crypto takeCallbackArgs rewiring removed the last C++ callers; runCallback2 stays for the webview backends.
There was a problem hiding this comment.
The dead-code deletion from my earlier note is done in 5c94f8b — thanks. No further findings, but this rewires the FFI contract and free/invoke ordering for all 11 extern crypto job types across the C++/Rust boundary, so it's worth a human pass.
Reviewed: the &mut [JSValue; 3] ↔ EncodedJSValue* ABI (thin pointer, matches); GC rooting of the produced args between deinit_ctx and run_callback (on the then stack frame, covered by JSC's conservative scan); the Drop null-guard vs. then's early free (no double-free); and that each takeCallbackArgs body preserves the pre-refactor arity and exception-pending-skips-callback behavior via RETURN_IF_EXCEPTION(scope, 0) + from_js_host_call_generic.
Extended reasoning...
Overview
This PR fixes an ASAN-visible leak in the async node:crypto jobs (generateKeyPair, sign/verify, diffieHellman, hkdf, generatePrime/checkPrime, generateKey) by restructuring the FFI contract: instead of C++ invoking the JS callback directly via Bun__EventLoop__runCallbackN while the job ctx is still alive, C++ now writes the callback arguments into a caller-provided EncodedJSValue[3] and returns argc. The Rust extern_crypto_job! plumbing then frees the ctx before calling event_loop.run_callback, so a callback that never returns (process.exit()) no longer strands the ctx's OpenSSL allocations. 21 files: 11 C++ ctx bodies + headers, the Rust macro in node_crypto_binding.rs, deletion of the now-dead runCallback1/runCallback3 exports in event_loop.rs and headers-handwritten.h, and new ASAN-gated leak tests.
Security risks
The change is in node:crypto but is a resource-lifetime refactor, not a change to key generation, signing, or verification logic. The produced JS values (KeyObjects, Buffers, booleans) are identical; only when the native ctx is freed moves. No new user-controlled input paths, no validation weakened. The one subtle correctness surface is GC: the args array holds unrooted JSValues across the ctx_deinit FFI call. JSC's conservative stack scanner covers the Rust then() frame, and ctx_deinit does not allocate on the JS heap or trigger GC (it's delete this on a TZONE-allocated C++ struct), so this is sound — but it's exactly the kind of invariant a human reviewer familiar with Bun's JSC integration should sign off on.
Level of scrutiny
High. This is an FFI contract change applied uniformly across 11 job types, with a lifecycle reordering (free-before-user-JS) that the whole PR exists to establish. Each C++ body was hand-rewritten from runFromJS to takeCallbackArgs, and each needed its own RETURN_IF_EXCEPTION placement to satisfy validateExceptionChecks now that the scope check no longer happens inside a nested runCallbackN. The PR description is unusually thorough (traces the exact leak, explains why #36598 made it visible, enumerates behavior-preservation claims, lists verification runs), and the follow-up commits (9ba4326, 5c94f8b) show the author responding correctly to review — but the breadth and the memory-safety stakes put this outside auto-approval.
Other factors
My earlier inline note (delete the newly-dead runCallback1/3 exports) was addressed in 5c94f8b and named in the description. CodeRabbit's two findings were discussed and either withdrawn or addressed via the RETURN_IF_EXCEPTION commit; the comment-cop flags on the Rust comments were dismissed with a reasonable justification (they document the FFI contract, not a workaround). The new tests are properly ASAN-gated, spawn per case with test.concurrent, drain both pipes, and assert the combined {stdout, stderr, exitCode} object per REVIEW.md conventions. The SignJobCtx trailing return 0 after an exhaustive switch is dead but harmless (compiler placation). I checked the &mut [JSValue; 3] Rust parameter against the C++ EncodedJSValue* signature — a reference to a fixed-size array is a thin pointer on the C ABI, so they match.
… callbacks Two issues found reviewing the completion rewiring: Shutdown release: a completion task enqueued but not yet dispatched when process.exit() runs (exit racing the work pool) was re-queued by release_queued_tasks_for_shutdown, stranding the ctx's OpenSSL memory behind LSan-invisible allocations: the same 1480-byte signature the callback-path fix closed. AnyTaskJob now carries an erased release entry and the shutdown release claims the tag, freeing the job without running its completion. A completion posted after the final drain (the work pool cannot be joined without blocking exit) remains quarantined in no-validate-leaksan.txt, now with an accurate comment. Error value: the caught-export-exception paths encoded the JSC::Exception cell itself, so the callback's err argument was not the thrown Error (not instanceof Error, no code property). Use Exception::value(), matching node, which surfaces ERR_CRYPTO_JWK_UNSUPPORTED_CURVE for JWK export of an unsupported curve. Same fix in GeneratePrimeJobCtx.
… array
Restore the runTask/runFromJS naming (work-pool half / JS-thread half)
and replace the EncodedJSValue* out-array plus count with a
JSCallbackArgs value type. Constructors are the only producers, so
bodies read as return { err } or return { jsNull(), publicKey,
privateKey }, and the Rust side consumes it as a slice. The extern
shims copy it through a typed out-pointer since C linkage cannot
return a class type.
There was a problem hiding this comment.
No bugs found, but this deserves human review: it restructures the FFI completion path for all 11 async crypto job types, adds a mirrored C++/Rust ABI struct (JSCallbackArgs), a second type-erased entry on AnyTaskJob with hand-computed offset dispatch, and a new shutdown-release path — memory-safety-critical native code across 25 files.
What was reviewed:
JSCallbackArgsC++/Rust layout mirroring (3×EncodedJSValue + u32) and the.min(argv.len())clamp onargc.release_erasedoffset math (.add(1)onfn(*mut ())) against theoffset_of!assertion; drop path frees ctx + callback viaExternCtx::Drop.- GC safety of the stack-held
JsCallbackArgsbetweenctx_run_from_jsandrun_callback(onlyctx_deinitruns in between; no JS allocation). - Exception-scope discipline (
RETURN_IF_EXCEPTIONafter each throwing production call) and that the pending-exception path still skips the callback viafrom_js_host_call_genericreturningErr.
Extended reasoning...
Overview
This PR restructures how the 11 C++-backed async node:crypto jobs (generateKeyPair×5, sign/verify, diffieHellman, hkdf, generatePrime, checkPrime, generateKey) deliver their completion. Previously each C++ runFromJS invoked the JS callback directly via Bun__EventLoop__runCallbackN while the native ctx was still alive; a callback that never returned (process.exit()) stranded the ctx's owned resources (EVP_PKEY, KeyObjectData, BIGNUMs), which #36598 made ASAN-visible. The fix moves callback invocation to the Rust extern_crypto_job! plumbing: C++ runFromJS now returns a JSCallbackArgs value (new header, mirrored as a #[repr(C)] Rust struct) through an out-pointer, Rust frees the ctx, then invokes run_callback. A second change adds a type-erased release_erased entry to AnyTaskJob so __bun_release_task_at_shutdown can free queued-but-undispatched completions at VM shutdown. A drive-by fix corrects the caught-exception paths (generateKeyPair, generatePrime) to pass Exception::value() (the Error) rather than the Exception cell, matching Node. Dead Bun__EventLoop__runCallback1/3 exports are deleted (my earlier comment, addressed in 5c94f8b).
Security risks
None identified. This is an internal lifecycle/ordering refactor; no new user-controlled input parsing, no changes to key generation or validation logic, no TLS/auth surface. The ABI mirroring is the risk surface — a layout mismatch would be memory corruption, not a security bypass.
Level of scrutiny
High. This is native memory-safety code at the Rust↔C++ FFI boundary with:
- A hand-mirrored ABI struct (
JSCallbackArgs↔JsCallbackArgs) whose layout must match exactly across compilers. - A second type-erased fn-pointer slot on
AnyTaskJob<C>reached by raw pointer arithmetic (.cast::<fn(*mut ())>().add(1)), guarded only by aconst _offset assertion. - A new shutdown-release path that frees jobs whose
Droptouches JSC handles (StrongOptional::deinit) while the VM is tearing down. - GC-lifetime reasoning that stack-held
JSValues survive acrossctx_deinit(correct — conservative stack scan covers them andctx_deinitdoesn't allocate JS — but subtle). - A behavior change (Exception cell →
->value()) piggybacked on the refactor.
REVIEW.md's memory-safety section is directly on point ("pair every acquisition with its release at the acquisition site", "reference counts provably balanced on every terminal path", "never let a pointer or slice outlive the memory it points into"). I checked each of these and found no issues, but the combination of FFI ABI, type erasure, and shutdown lifecycle across 25 files is exactly the kind of change where a maintainer familiar with AnyTaskJob's invariants and the VM shutdown sequence should sign off.
Other factors
- All prior review threads (CodeRabbit, comment-cop, my dead-code note) are resolved; the author addressed them with follow-up commits (9ba4326, 5c94f8b, a78c019, c6a6990).
- Test coverage is thorough: an
isASAN-gated leak suite covering every job type + the shutdown-queue path, and a Node-parity test for theException::value()fix. The PR description documents that the leak tests fail on the unfixed build with the exact CI signature. - The
SignJobCtx::runFromJStrailingreturn {};after the exhaustive switch is unreachable (both cases return) — compiler placation, not a latent zero-arg-callback path. - The
no-validate-leaksan.txtentry fortest-crypto-op-during-process-exit.jsis retained with an accurate comment explaining the unfixable work-pool race sliver.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/node/node_crypto_binding.rs (1)
129-148: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPass callback-argument extraction errors to the completion callback.
try_swap()has already removedcallback. TheErrbranch reports the exception as unhandled and never invokes that callback. Promisified crypto operations can remain pending.Convert
errwithglobal.take_exception(err). Then callrun_callbackwith the converted error as argument zero.Proposed fix
- Err(err) => global.report_active_exception_as_unhandled(err), + Err(err) => { + let error = global.take_exception(err); + global.bun_vm().event_loop_mut().run_callback( + callback, + global, + JSValue::UNDEFINED, + &[error], + ); + }As per coding guidelines, “Never swallow failures or signal success after failure; propagate … errors explicitly.”
🤖 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 `@src/runtime/node/node_crypto_binding.rs` around lines 129 - 148, Update the Err branch after ctx deinitialization in the callback flow to convert err with global.take_exception(err), then invoke run_callback using the removed callback and the converted error as argument zero instead of reporting the exception as unhandled. Preserve the existing successful callback path and ensure the completion callback is always signaled on callback-argument extraction failure.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.
Outside diff comments:
In `@src/runtime/node/node_crypto_binding.rs`:
- Around line 129-148: Update the Err branch after ctx deinitialization in the
callback flow to convert err with global.take_exception(err), then invoke
run_callback using the removed callback and the converted error as argument zero
instead of reporting the exception as unhandled. Preserve the existing
successful callback path and ensure the completion callback is always signaled
on callback-argument extraction failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f8115bf0-15ca-4709-b0d0-3b57a4b516c6
📒 Files selected for processing (23)
src/jsc/any_task_job.rssrc/jsc/bindings/node/crypto/CryptoDhJob.cppsrc/jsc/bindings/node/crypto/CryptoDhJob.hsrc/jsc/bindings/node/crypto/CryptoGenDhKeyPair.cppsrc/jsc/bindings/node/crypto/CryptoGenDsaKeyPair.cppsrc/jsc/bindings/node/crypto/CryptoGenEcKeyPair.cppsrc/jsc/bindings/node/crypto/CryptoGenKeyPair.cppsrc/jsc/bindings/node/crypto/CryptoGenKeyPair.hsrc/jsc/bindings/node/crypto/CryptoGenNidKeyPair.cppsrc/jsc/bindings/node/crypto/CryptoGenRsaKeyPair.cppsrc/jsc/bindings/node/crypto/CryptoHkdf.cppsrc/jsc/bindings/node/crypto/CryptoHkdf.hsrc/jsc/bindings/node/crypto/CryptoKeygen.cppsrc/jsc/bindings/node/crypto/CryptoKeygen.hsrc/jsc/bindings/node/crypto/CryptoPrimes.cppsrc/jsc/bindings/node/crypto/CryptoPrimes.hsrc/jsc/bindings/node/crypto/CryptoSignJob.cppsrc/jsc/bindings/node/crypto/CryptoSignJob.hsrc/jsc/bindings/node/crypto/JSCallbackArgs.hsrc/jsc/event_loop.rssrc/runtime/dispatch.rssrc/runtime/node/node_crypto_binding.rstest/js/node/crypto/crypto.key-objects.test.ts
|
Re the repeated suggestion to invoke the callback from the Err branch in node_crypto_binding.rs: declining for the same reason as the earlier resolved thread. That branch only fires for exceptions the C++ body left pending, and those never reached the callback before this PR either (Bun__EventLoop__runCallbackN opened with a has_exception gate, so the invoke was a no-op and the same unhandled report fired). Exceptions the bodies catch are still delivered as the err argument through the normal path. Invoking the callback with a pending-exception error would be new behavior on OOM-class paths, and would diverge from node, which surfaces result-conversion failures as uncaught exceptions rather than callback errors. |
There was a problem hiding this comment.
I didn't find any bugs, and both of my earlier nits (dead runCallback1/3 exports; generatePrime in the leak matrix) have been addressed. Deferring to a human because this is a 25-file refactor of memory-safety-critical FFI plumbing — the free-before-user-JS ordering, the new erased shutdown-release path in AnyTaskJob, and the cross-language JSCallbackArgs layout contract all warrant a maintainer's eyes.
What was reviewed:
- Rust↔C++
JsCallbackArgslayout parity (#[repr(C)][JSValue;3]+u32vsEncodedJSValue[3]+uint32_t) and the.min(argv.len())slice bound. - GC safety of the produced
JSValues living on thethen()stack frame afterctx_deinitand beforerun_callback— they're stack-rooted for the conservative scan. release_erased's.add(1)fn-pointer read is guarded by theoffset_of!const-assert; the newtask_tag::AnyTaskJobshutdown arm affects allAnyTaskJobusers (pbkdf2/scrypt/random too), whoseDropimpls look safe to run at that point.RETURN_IF_EXCEPTIONafter every throwing production call in the rewrittenrunFromJSbodies; theException::value()fix has a dedicated test.
Extended reasoning...
Overview
This PR restructures the completion path for the 11 extern C++ crypto async jobs (generateKeyPair ×5, sign/verify, diffieHellman, hkdf, generatePrime, checkPrime, generateKey) so the native ctx is freed before the JS completion callback runs. The C++ runFromJS bodies no longer receive or invoke the callback; instead they return a small by-value JSCallbackArgs struct through an out-pointer, and the Rust extern_crypto_job! macro's then() body does runFromJS → deinit ctx → run_callback. It also adds an erased release entry to AnyTaskJob so queued-but-undispatched completions are freed at VM shutdown, deletes the now-dead Bun__EventLoop__runCallback1/3 exports, and fixes a pre-existing bug where the caught-export-exception paths passed the JSC::Exception cell (not the thrown Error) to the callback. 25 files: 4 Rust, 14 C++ (headers + bodies), 1 new C++ header, tests, and no-validate-leaksan.txt.
Security risks
None identified. This is internal resource-lifecycle plumbing; no user-input parsing, auth, or crypto-primitive changes. The Exception::value() change surfaces a more useful error object (with .code) to userland, matching Node.
Level of scrutiny
High. This is exactly the memory-safety category REVIEW.md calls out most heavily: cross-language FFI ownership, ctx lifecycle across work-pool → JS-thread handoff, unsafe Rust with hand-verified layout invariants (the offset_of! const-asserts and .cast::<fn(*mut ())>().add(1) read), GC-rooting reasoning for JSValues held between ctx free and callback invocation, and a new shutdown-release path that applies to every AnyTaskJob user (not only crypto). The change is well-reasoned and thoroughly tested (ASAN-gated leak suite covering all 7 ctx classes + a shutdown-race case + a node-parity behavior test), but the surface area and the class of bug it targets (UAF/leak on non-returning callbacks) mean a maintainer should sign off on the design and the unsafe-block invariants.
Other factors
- Both of my prior inline nits were addressed (5c94f8b deleted the dead
runCallback1/3exports; 31bff9b addedgeneratePrimeto the leak matrix). - The
SignJobCtx::runFromJSfall-throughreturn {}after the exhaustiveswitchis unreachable (both enum armsreturn); it exists to satisfy the compiler now that the function has a non-void return type — harmless. - The
dispatch.rsshutdown arm now freesAnyTaskJobentries instead of re-queuing them; I checked that the Rust-nativeCryptoJobctxs (Scrypt,random::JobCtx, PBKDF2'sCallbackCtx) all haveDrop/deinitpaths that don't require the completion to have run, so this broader change looks safe — but it's a behavior change beyond the crypto extern jobs and worth a second look. - CodeRabbit's finding was withdrawn after discussion; the comment-cop bot flags were dismissed as documenting FFI contracts, not workarounds.
…llback (oven-sh#36986) ## What `test/js/node/async_hooks/AsyncLocalStorage-tracking.test.ts` (the crypto-generateKeyPair fixture) fails on every Linux x64-asan run since oven-sh#36598 landed (builds [89023](https://buildkite.com/bun/bun/builds/89023), [89031](https://buildkite.com/bun/bun/builds/89031)): ``` direct leak of 24b in run (src/runtime/node/node_crypto_binding.rs:85:21) +34 more SUMMARY: AddressSanitizer: 1480 byte(s) leaked in 35 allocation(s). #6 EVP_PKEY_keygen vendor/boringssl/crypto/evp/evp_ctx.cc #7 Bun::KeyPairJobCtx::runTask src/jsc/bindings/node/crypto/CryptoGenKeyPair.cpp:23 #8 Bun__RsaKeyPairJobCtx__runTask src/jsc/bindings/node/crypto/CryptoGenRsaKeyPair.cpp:26 ``` ## Cause The 11 extern crypto job ctxs (generateKeyPair x5, sign/verify, diffieHellman, hkdf, generatePrime, checkPrime, generateKey) completed by invoking the JS callback from inside C++ `runFromJS` while the ctx was still alive; the ctx was freed only after `then()` returned. A callback that never returns (the fixture calls `process.exit(0)` inside it) stranded everything the ctx still owned: the generated `EVP_PKEY`, `KeyObjectData` refs, `BIGNUM`s. The leak is pre-existing; oven-sh#36598 made it observable by routing `OPENSSL_malloc` through libc under ASAN. Whether LSan reported the other job types too was codegen luck (their pointers happened to be reachable by the conservative stack scan); `generateKeyPair`'s `EVP_PKEY` sits behind two FastMalloc indirections and was reported deterministically. ## Fix Make it structurally impossible for a job ctx to hold native resources across user JS: the native side never sees the callback. - `runFromJS` keeps its name (the JS-thread half, paired with the work-pool half `runTask`) but no longer receives the callback. It returns `JSCallbackArgs`, a small by-value type whose constructors are the only producers, so bodies read `return { err };` or `return { jsNull(), publicKey, privateKey };`. The extern "C" shims copy it through a typed out-pointer (C linkage cannot return a class type); the Rust side consumes it as a slice. - The Rust `extern_crypto_job!` plumbing does, in order: run `runFromJS` to produce the arguments, free the ctx (`ctx_deinit`), invoke the callback. The invariant lives in one place and applies to every job type. - Shutdown release: a completion task enqueued but not yet dispatched when `process.exit()` runs (exit racing the work pool) used to be re-queued at shutdown, stranding the ctx the same way. `AnyTaskJob` now carries an erased release entry and the shutdown release frees the job without running its completion. A completion posted after the final drain is not recoverable without joining the work pool (which would block exit); `test-crypto-op-during-process-exit.js` stays in `no-validate-leaksan.txt` for that sliver, now with an accurate comment. - The caught-export-exception paths encoded the `JSC::Exception` cell itself, so the callback's err argument was not the thrown Error (not `instanceof Error`, no `code`). They now use `Exception::value()`, matching node: JWK export of an unsupported curve surfaces `ERR_CRYPTO_JWK_UNSUPPORTED_CURVE`. No behavior change otherwise: - `Bun__EventLoop__runCallback{1,2,3}` were Rust's `EventLoop::run_callback` exported to C++. The plumbing now calls `run_callback` directly: same enter/exit bracketing, same pending-exception gate, same unhandled-exception reporting, same synchronous timing. This made `runCallback1`/`runCallback3` dead (the crypto bodies were their last callers), so their exports and declarations are deleted; `runCallback2` stays for the webview backends. - Callback arity is preserved per path (observable via `arguments.length`): error paths pass 1 arg, results 2, generateKeyPair success 3. - Exception paths are preserved: a throw out of argument production skips the callback and reports unhandled, as before. Each `runFromJS` checks its `ThrowScope` after every call that can throw (`RETURN_IF_EXCEPTION`), since the check that used to happen inside the nested `runCallbackN` call now happens after the C++ scope destructs; `BUN_JSC_validateExceptionChecks` verifies this on the asan lane. - The produced `JSValue`s live on the `then()` stack frame between production and invocation, which JSC's conservative scan covers; they are JS-heap values, so freeing the ctx first cannot invalidate them. - Perf: same number of FFI crossings, no allocation added. The Rust-native crypto jobs (pbkdf2, scrypt, random) already had the ordering property: they resolve promises or queue the callback via nextTick, so their ctx drops before user JS runs. The synchronous-callback extern jobs were the gap. ## Verification New tests in `crypto.key-objects.test.ts`: - `isASAN`-gated leak suite: children run with `BUN_DESTRUCT_VM_ON_EXIT=1` and `detect_leaks=1` (the asan lane's configuration) and call `process.exit(0)` from the callback of each job type: generateKeyPair (KeyObject and encrypted PEM outputs), sign, diffieHellman, hkdf, checkPrime, generateKey, plus an exit-before-completion-dispatch case (busy-spin so the queued completion is never dispatched). - An export-error test: `generateKeyPair('ec', { namedCurve: 'secp224r1', ...jwk encodings })` asserts the callback err is `instanceof Error` with code `ERR_CRYPTO_JWK_UNSUPPORTED_CURVE` (matches node; fails on main, which passes the Exception cell). Results: - unfixed build (src stashed): both generateKeyPair leak tests fail with the exact CI signature (`Direct leak of 24 byte(s)` in `EVP_PKEY_keygen` via `KeyPairJobCtx::runTask`) - fixed build: all pass, including under `BUN_JSC_validateExceptionChecks=1`, and ec/ed25519 keypair and verify probes run leak-clean as well - `AsyncLocalStorage-tracking.test.ts`: 74 pass, 0 fail (all async-context crypto fixtures, against both bun and node) - `crypto.test.ts` (369), `crypto.key-objects.test.ts` (117), and 37 node parallel files (`test-crypto-keygen*`, `test-crypto-sign-verify`, `test-crypto-hkdf`, `test-crypto-dh-stateless`, `test-crypto-*prime*`) all pass The break landed with oven-sh#36598 (which made the leak visible); oven-sh#36657 proposed clearing individual ctx fields before the callback, and this PR supersedes that approach with the ordering guarantee in the job plumbing instead of per-field resets. <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/async_hooks/AsyncLocalStorage-tracking.test.ts test/js/node/crypto/crypto.key-objects.test.ts <!-- robobun:evidence:end -->
2 participants
What
test/js/node/async_hooks/AsyncLocalStorage-tracking.test.ts(the crypto-generateKeyPair fixture) fails on every Linux x64-asan run since #36598 landed (builds 89023, 89031):Cause
The 11 extern crypto job ctxs (generateKeyPair x5, sign/verify, diffieHellman, hkdf, generatePrime, checkPrime, generateKey) completed by invoking the JS callback from inside C++
runFromJSwhile the ctx was still alive; the ctx was freed only afterthen()returned. A callback that never returns (the fixture callsprocess.exit(0)inside it) stranded everything the ctx still owned: the generatedEVP_PKEY,KeyObjectDatarefs,BIGNUMs.The leak is pre-existing; #36598 made it observable by routing
OPENSSL_mallocthrough libc under ASAN. Whether LSan reported the other job types too was codegen luck (their pointers happened to be reachable by the conservative stack scan);generateKeyPair'sEVP_PKEYsits behind two FastMalloc indirections and was reported deterministically.Fix
Make it structurally impossible for a job ctx to hold native resources across user JS: the native side never sees the callback.
runFromJSkeeps its name (the JS-thread half, paired with the work-pool halfrunTask) but no longer receives the callback. It returnsJSCallbackArgs, a small by-value type whose constructors are the only producers, so bodies readreturn { err };orreturn { jsNull(), publicKey, privateKey };. The extern "C" shims copy it through a typed out-pointer (C linkage cannot return a class type); the Rust side consumes it as a slice.extern_crypto_job!plumbing does, in order: runrunFromJSto produce the arguments, free the ctx (ctx_deinit), invoke the callback. The invariant lives in one place and applies to every job type.process.exit()runs (exit racing the work pool) used to be re-queued at shutdown, stranding the ctx the same way.AnyTaskJobnow carries an erased release entry and the shutdown release frees the job without running its completion. A completion posted after the final drain is not recoverable without joining the work pool (which would block exit);test-crypto-op-during-process-exit.jsstays inno-validate-leaksan.txtfor that sliver, now with an accurate comment.JSC::Exceptioncell itself, so the callback's err argument was not the thrown Error (notinstanceof Error, nocode). They now useException::value(), matching node: JWK export of an unsupported curve surfacesERR_CRYPTO_JWK_UNSUPPORTED_CURVE.No behavior change otherwise:
Bun__EventLoop__runCallback{1,2,3}were Rust'sEventLoop::run_callbackexported to C++. The plumbing now callsrun_callbackdirectly: same enter/exit bracketing, same pending-exception gate, same unhandled-exception reporting, same synchronous timing. This maderunCallback1/runCallback3dead (the crypto bodies were their last callers), so their exports and declarations are deleted;runCallback2stays for the webview backends.arguments.length): error paths pass 1 arg, results 2, generateKeyPair success 3.runFromJSchecks itsThrowScopeafter every call that can throw (RETURN_IF_EXCEPTION), since the check that used to happen inside the nestedrunCallbackNcall now happens after the C++ scope destructs;BUN_JSC_validateExceptionChecksverifies this on the asan lane.JSValues live on thethen()stack frame between production and invocation, which JSC's conservative scan covers; they are JS-heap values, so freeing the ctx first cannot invalidate them.The Rust-native crypto jobs (pbkdf2, scrypt, random) already had the ordering property: they resolve promises or queue the callback via nextTick, so their ctx drops before user JS runs. The synchronous-callback extern jobs were the gap.
Verification
New tests in
crypto.key-objects.test.ts:isASAN-gated leak suite: children run withBUN_DESTRUCT_VM_ON_EXIT=1anddetect_leaks=1(the asan lane's configuration) and callprocess.exit(0)from the callback of each job type: generateKeyPair (KeyObject and encrypted PEM outputs), sign, diffieHellman, hkdf, checkPrime, generateKey, plus an exit-before-completion-dispatch case (busy-spin so the queued completion is never dispatched).generateKeyPair('ec', { namedCurve: 'secp224r1', ...jwk encodings })asserts the callback err isinstanceof Errorwith codeERR_CRYPTO_JWK_UNSUPPORTED_CURVE(matches node; fails on main, which passes the Exception cell).Results:
Direct leak of 24 byte(s)inEVP_PKEY_keygenviaKeyPairJobCtx::runTask)BUN_JSC_validateExceptionChecks=1, and ec/ed25519 keypair and verify probes run leak-clean as wellAsyncLocalStorage-tracking.test.ts: 74 pass, 0 fail (all async-context crypto fixtures, against both bun and node)crypto.test.ts(369),crypto.key-objects.test.ts(117), and 37 node parallel files (test-crypto-keygen*,test-crypto-sign-verify,test-crypto-hkdf,test-crypto-dh-stateless,test-crypto-*prime*) all passThe break landed with #36598 (which made the leak visible); #36657 proposed clearing individual ctx fields before the callback, and this PR supersedes that approach with the ordering guarantee in the job plumbing instead of per-field resets.
no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/async_hooks/AsyncLocalStorage-tracking.test.ts test/js/node/crypto/crypto.key-objects.test.ts