crypto: drop generateKeyPair ctx's KeyObject ref before invoking the callback

[robobun]

What

crypto.generateKeyPair's async completion (KeyPairJobCtx::runFromJS) kept m_keyObj (a RefPtr<KeyObjectData>) across the user callback. The Rust ExternCtx Drop that deletes the C++ ctx runs only after then() returns, so a callback that never returns (process.exit()) strands the ctx's ref. lastChanceToFinalize destroys the JSPublicKeyObject/JSPrivateKeyObject wrappers during VM teardown, but the stranded ctx ref keeps KeyObjectData (and the EVP_PKEY it owns) alive past the leak check.

This also routes the BoringSSL memory hooks through bun_alloc::default_alloc instead of hard-coding mimalloc. The ASAN build config already drops BORINGSSL_REQUIRE_MEMORY_HOOKS so BoringSSL falls back to libc ("Off under ASAN so BoringSSL allocs stay on the intercepted libc heap"), but on ELF the weak OPENSSL_memory_alloc/free/get_size symbols still resolved to our definitions, which called mi_malloc directly and kept every OPENSSL_malloc allocation invisible to LeakSanitizer on Linux. Routing through default_alloc makes them libc under cfg(bun_asan) everywhere. The crate-root bun_alloc::usable_size (a mi_usable_size wrapper whose only callers were these hooks) is deleted.

Reproduction

With the hook change alone:

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:419
  #7 Bun::KeyPairJobCtx::runTask src/jsc/bindings/node/crypto/CryptoGenKeyPair.cpp:23
  #8 Bun__RsaKeyPairJobCtx__runTask src/jsc/bindings/node/crypto/CryptoGenRsaKeyPair.cpp:26

from test/js/node/async_hooks/async-context/async-context-crypto-generateKeyPair.js, which calls process.exit(0) inside the callback. First seen as the only [new] failure on the x64-asan lane of build 86653 for #36598, which applies the same hook change for its own LSan visibility.

Cause

Ownership chain at the time the callback runs:

stack Box<AnyTaskJob>  (libc under ASAN)
  -> ExternCtx.ctx: *mut RsaKeyPairJobCtx  (TZone/FastMalloc, not LSan-tracked)
       -> m_keyObj.m_data: KeyObjectData*  (TZone/FastMalloc, not LSan-tracked)
            -> EVPKeyPointer.pkey_: EVP_PKEY*  (OPENSSL_malloc, now libc)

LSan scans the libc-backed AnyTaskJob box and the stack but can't follow through the FastMalloc blocks, so the EVP_PKEY is reported as a direct leak. The two JS key wrappers hold their own RefPtr<KeyObjectData> but live in JSC GC cells (also not scanned as a root).

Fix

runFromJS now assigns m_keyObj = {} once the JS values have been produced (and on the two export-failure paths before the error callback). With the ctx's ref gone, either ~VM -> lastChanceToFinalize destroying the JS wrappers brings the refcount to zero (KeyObject output), or the refcount is already zero (string/buffer output), and EVP_PKEY_free runs before LSan's atexit check.

SignJobCtx and DhJobCtx get the same reset for their RefPtr<KeyObjectData> fields (m_keyData, m_privateKey/m_publicKey). runTask has already produced the result and the ctx has no further use for the ref, so the reset is free.

Scope

Tested each of the other async crypto job contexts under BUN_DESTRUCT_VM_ON_EXIT=1 + detect_leaks=1 with process.exit(0) in the callback. With only the OPENSSL_memory_alloc hook applied and main's C++:

path	result
generateKeyPair (KeyObject output)	leaks 1480b / 35 allocs
generateKeyPair (PEM + passphrase output)	leaks 1480b / 35 allocs
sign with PEM-string key	no leak
diffieHellman	no leak
generatePrime	no leak
checkPrime	no leak

Only generateKeyPair trips LSan: its EVP_PKEY* sits behind two FastMalloc indirections (ctx -> KeyObjectData* -> EVP_PKEY*) and runFromJS never loads the raw pointer onto the stack. The others hold their OPENSSL_malloc pointer either directly in the ctx or load it onto the runFromJS frame (for memcpy into the result buffer), so LSan's conservative stack scan finds it. The SignJobCtx/DhJobCtx resets are kept for consistency; HkdfJobCtx::m_key holds a symmetric Vector<uint8_t> (FastMalloc, not OPENSSL_malloc) and is left alone, as are GeneratePrimeJobCtx/CheckPrimeJobCtx and the KeyPairJobCtx passphrase, none of which are observable.

Verification

New isASAN-gated tests in crypto.key-objects.test.ts spawn a child with BUN_DESTRUCT_VM_ON_EXIT=1 and detect_leaks=1, generate an RSA key pair (once with KeyObject output, once with encrypted PEM output), and call process.exit(0) from the callback.

• hook change without the m_keyObj reset: both cases fail with the 35-allocation LSan report above
• both changes: both pass, child exits 0
• crypto.test.ts (369 tests), crypto-rsa.test.js, crypto.key-objects.test.ts, the node test-crypto-keygen-* parallel suite, test-crypto-sign-verify.js, test-crypto-dh-stateless.js, and all async-context-crypto-* fixtures pass leak-clean

Fail-before note

The automated fail-before check stashes all of src/ and rebuilds. With the hook reverted to mimalloc, LSan cannot see the EVP_PKEY allocation at all, so the new tests pass trivially on main. The leak only becomes observable once OPENSSL_memory_alloc lands on libc, which is itself one of the src/ changes in this diff; the fail-before/pass-after demonstration above (stashing only CryptoGenKeyPair.cpp) is the closest mechanical proof available.

---

no test proof · iteration 2 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/crypto/crypto.key-objects.test.ts

[coderabbitai]

Walkthrough

BoringSSL hooks now use default allocator APIs. Node crypto jobs release key references before JavaScript callback re-entry, including error paths. ASAN tests cover callback-triggered process exits for generated key objects and encrypted PEM outputs.

Changes

Memory cleanup

Layer / File(s)	Summary
Default allocator integration src/bun_alloc/lib.rs, src/boringssl/lib.rs	The BoringSSL hooks use default_alloc::malloc, usable_size, and free. The root-level mimalloc size helper was removed.
Crypto callback reference release src/jsc/bindings/node/crypto/CryptoDhJob.cpp, src/jsc/bindings/node/crypto/CryptoGenKeyPair.cpp, src/jsc/bindings/node/crypto/CryptoSignJob.cpp	Crypto jobs clear stored key references before success and exception callbacks.
ASAN callback regression tests test/js/node/crypto/crypto.key-objects.test.ts	ASAN subprocess tests cover process.exit() during key-generation callbacks and check outputs, exit status, and LeakSanitizer diagnostics.

Suggested reviewers: jarred-sumner, cirospaciari

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly identifies the primary fix: releasing the generateKeyPair context's KeyObject reference before the callback.
Description check	✅ Passed	The description explains the problem, fix, scope, and verification results, although its headings differ from the repository template.

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

Status: reproduced locally (1480b / 35 allocs via EVP_PKEY_keygen), fix verified leak-clean under BUN_DESTRUCT_VM_ON_EXIT=1 + detect_leaks=1. Swept the sibling async crypto job contexts; only generateKeyPair observably leaks (scope table in the PR body). Deleted the now-dead crate-root bun_alloc::usable_size. All review threads resolved.

CI build 87518: crypto.key-objects.test.ts and AsyncLocalStorage-tracking.test.ts pass on every lane including x64-asan. The only [new] failure is bun-upgrade.test.ts on windows-aarch64 ("Canary builds are not available for this platform yet"), unrelated to this diff and reported separately. Everything else is [flaky] or agent-creation infra. Ready for review.

Note on the automated fail-before check: it stashes all of src/, which reverts the OPENSSL_memory_alloc hook to mimalloc, and with BoringSSL allocations invisible to LSan the new tests pass trivially on main. Stashing only CryptoGenKeyPair.cpp (keeping the hook) reproduces the failure and the fix resolves it; details in the PR body.

[claude]

Beyond the inline nit, I checked that default_alloc::{malloc,free,usable_size} are byte-identical to the previous direct mi_* calls under !cfg(bun_asan), so the BoringSSL hook change is a release no-op; and that lifting the #[cfg(any(debug_assertions, bun_asan))] gate on default_alloc::usable_size still compiles on Windows (the catch-all arm falls through to mi_usable_size).

Extended reasoning...

The inline nit (sibling SignJobCtx/DhJobCtx sharing the same hold-ref-across-callback pattern) is the only finding; it's an at-exit LSan report under a debug-only config with no CI coverage today, so it doesn't block. The two things I independently verified beyond it: (1) default_alloc::malloc/free/usable_size reduce to mi_malloc/mi_free/mi_usable_size when bun_asan is off, so release behavior of the OPENSSL memory hooks is unchanged; (2) removing the debug_assertions|bun_asan cfg gate on default_alloc::usable_size is safe on all check-all targets because the non-ASAN catch-all arm is mi_usable_size. Deferring to a human because the BoringSSL hook is process-global crypto/TLS allocator infrastructure and there's an open nit to address or acknowledge.

[robobun]

^{Updated 11:05 PM PT - Aug 1st, 2026}

❌ @robobun, your commit fffaa6b has 2 failures in Build #87518 (All Failures):

• test/cli/install/bun-upgrade.test.ts - code 1 on 🪟 11 aarch64
• Failed to create agent for 🪟 2019 x64 - test-bun
• Failed to create agent for 🪟 x64 - verify-baseline
• Failed to create agent for 🪟 2019 x64 - test-bun

---

🧪   To try this PR locally:

bunx bun-pr 36657

That installs a local version of the PR into your bun-36657 executable, so you can run:

bun-36657 --bun

[claude]

Both prior findings are addressed in fffaa6b — the orphaned crate-root bun_alloc::usable_size is deleted (and named in the description), and the Scope section now states the intentional exclusions (GeneratePrimeJobCtx/CheckPrimeJobCtx/the KeyPairJobCtx passphrase) with the empirical no-leak table. No new issues found. Deferring only because the OPENSSL_memory_* hook rewire touches the allocator every TLS/crypto path shares — worth one human glance even though non-ASAN builds reduce to the same mi_* calls after inlining.

Checked: default_alloc::{malloc,free,usable_size} are all pub and the non-ASAN cfg arm falls through to mi_usable_size, so release behavior is unchanged; the RefPtr resets in KeyPairJobCtx/SignJobCtx/DhJobCtx all sit after the last read of the cleared field; the new tests follow harness conventions (bunEnv spread, test.concurrent, pipes drained via Promise.all, isASAN-gated).

Extended reasoning...

Overview

Six files: (1) src/boringssl/lib.rs rewires OPENSSL_memory_alloc/free/get_size from hard-coded mi_* to bun_alloc::default_alloc::*; (2) src/bun_alloc/lib.rs promotes default_alloc::usable_size from #[cfg(any(debug_assertions, bun_asan))] pub(crate) to unconditional pub, and deletes the now-orphaned crate-root usable_size wrapper; (3–5) three async crypto job contexts (KeyPairJobCtx, SignJobCtx, DhJobCtx) clear their RefPtr<KeyObjectData> fields before invoking the user callback; (6) two ASAN-gated leak regression tests spawning a child that calls process.exit(0) inside the generateKeyPair callback under BUN_DESTRUCT_VM_ON_EXIT=1 + detect_leaks=1.

What changed since the last review

Commit fffaa6b addressed both open findings: the dead crate-root usable_size is removed and named in the description, and the PR body's Scope section now enumerates the sibling contexts I flagged with an empirical leak/no-leak table and explicitly states why GeneratePrimeJobCtx/CheckPrimeJobCtx/the passphrase are left alone (not LSan-observable — the raw pointer lands on the runFromJS frame or in the ctx directly, so the conservative stack scan finds it). The second new test exercises the encrypted-PEM path, covering the passphrase-carrying m_privateKeyEncoding.

Security risks

None identified. No cryptographic logic, key-material handling, or validation changes — only earlier release of refcounts the ctx destructor would drop anyway, plus allocator routing that is a no-op in release builds (default_alloc on the non-ASAN arm is mi_malloc/mi_free/mi_usable_size). The rewire matches the convention documented in src/CLAUDE.md ("route through bun_alloc::default_alloc rather than mi_* directly").

Level of scrutiny

Medium-high. The ref resets are trivially correct (each field is last read before the reset). The allocator hook is the piece that warrants a human look: it sits under every OPENSSL_malloc in the process (all TLS handshakes, every node:crypto op). I verified the non-ASAN path is behavior-identical, but a maintainer should confirm they're comfortable with default_alloc::usable_size losing its #[cfg] guard and becoming a stable public entry point.

Other factors

All prior review threads (mine, the comment-cop bot's) are resolved. The fail-before caveat is honestly documented in the PR body. Test coverage is appropriate for an LSan-visibility fix — gated on isASAN, concurrent, 30s timeout justified by the symbolizer note.

[robobun]

Main now hits this exact leak: #36598 landed the same memory hook change (6b6fb1a), which made the stranded EVP_PKEY visible to LeakSanitizer, and test/js/node/async_hooks/AsyncLocalStorage-tracking.test.ts now fails on every Linux x64-asan run. The async-context-crypto-generateKeyPair.js fixture calls process.exit(0) inside the generateKeyPair callback, producing the signature documented above: direct leak of 24b in run (src/runtime/node/node_crypto_binding.rs:85:21) +34 more, 1480 bytes in 35 allocations. Examples: build 89023 and build 89031.

Notes for landing this:

• The branch now conflicts on src/boringssl/lib.rs and src/bun_alloc/lib.rs because main already contains that half of the diff via fetch: cache client TLS sessions for resumption #36598. Resolving those two files to main's side and keeping the CryptoGenKeyPair.cpp / CryptoDhJob.cpp / CryptoSignJob.cpp resets plus the tests is the whole rebase.
• After the rebase the fail-before caveat in the PR body no longer applies: the new tests fail on current main as-is, since the hook change is already there.
• The debian x64-asan lane on this PR ran green with both halves applied, and current main (hook half only) is red, so the two halves of the proof are already on record.

[robobun]

Superseded by #36986, which makes the job plumbing free the ctx before invoking the callback for all 11 job types instead of resetting individual ctx fields. The failing main test (AsyncLocalStorage-tracking on x64-asan) is covered there with the same fail-before signature.