node:crypto: fix null deref when worker.terminate() lands during an EC JWK private key import

[robobun]

Symptom

worker.terminate() while the worker is inside crypto.createPrivateKey({ key, format: "jwk" }) for an EC key kills the whole process. Stock bun (1.4.0 canary, 9008ae7) dies on the first or second worker with:

panic: Segmentation fault at address 0x10

Debug/ASAN builds report the same site directly:

src/jsc/bindings/node/crypto/KeyObject.cpp:1423:35: runtime error: member call on null pointer of type 'JSC::JSArrayBufferView'
    Bun::KeyObject::getKeyObjectHandleFromJwk <- KeyObject::prepareAsymmetricKey <- Bun::jsCreatePrivateKey

Repro (crashes every run on main, usually within the first few workers):

const { Worker, isMainThread, parentPort } = require("worker_threads");
const crypto = require("crypto");
if (isMainThread) {
  let n = 0;
  const again = () => {
    const w = new Worker(__filename);
    w.on("error", () => {});
    w.on("message", () => setTimeout(() => w.terminate(), n % 6));
    w.on("exit", () => (++n < 60 ? again() : console.log("survived", n)));
  };
  again();
} else {
  const jwk = crypto.generateKeyPairSync("ec", { namedCurve: "P-256" }).privateKey.export({ format: "jwk" });
  parentPort.postMessage("busy");
  for (;;) crypto.createPrivateKey({ key: jwk, format: "jwk" });
}

Cause

decodeJwkString() returns null when constructFromEncoding() leaves an exception pending. Every other call in getKeyObjectHandleFromJwk follows it with RETURN_IF_EXCEPTION; the EC private key branch called ->span() on the result of decoding d without checking.

That is reachable in normal use because RETURN_IF_EXCEPTION is also where a worker's termination trap gets serviced (vm.hasExceptionsAfterHandlingTraps()): once terminate() has fired the trap, the next check inside constructFromEncoding throws the TerminationException and the decode returns null. Any terminate that lands between the y decode's check and the d decode, a window that includes setPublicKeyRaw, hits it, which is why the repro fires so quickly.

Fix

Add the missing RETURN_IF_EXCEPTION(scope, {}) after decoding d, matching the x/y/RSA/OKP/AKP branches of the same function. #33752 (EC scalar validation) happens to carry the same line as part of a larger change; this PR is just the crash fix.

The same pattern exists in CryptoHkdf.cpp prepareKey (string key path), which the open #32258 covers; not touched here.

Verification

New test in test/js/node/crypto/crypto.key-objects.test.ts. A self-spawning fixture terminates rounds of four workers spinning on an EC JWK import; d carries leading zero bytes (same scalar, still imports) so the decode dominates each call and the trap lands inside it nearly every time.

• Without the fix: fails 6/6 on a debug ASAN build (UBSan report above, first round each time) and 6/6 on a release binary (Segmentation fault at address 0x10).
• With the fix: passes 6/6 in isolation (about 2.4s each on a debug ASAN build, 2 rounds) and the whole file passes 2/2; release runs 6 rounds in well under a second.

---

no test proof · iteration 1 · 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]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 30 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: f32193b5-c51d-4c69-88af-39dca42a1856

📥 Commits

Reviewing files that changed from the base of the PR and between 54d6d16 and f2f65a4.

📒 Files selected for processing (2)

• src/jsc/bindings/node/crypto/KeyObject.cpp
• test/js/node/crypto/crypto.key-objects.test.ts

---

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

[robobun]

^{Updated 12:59 AM PT - Aug 11th, 2026}

✅ @robobun, your commit f2f65a411a296c44b8966699d2233af4ffcdbb5e passed in Build #92056! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 37443

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

bun-37443 --bun

[robobun]

Status: ready for review, CI running (build 92056).

Reproduced on stock bun with the repro in the description: panic: Segmentation fault at address 0x10 within the first few workers, every run. On a debug ASAN build UBSan names the site (KeyObject.cpp:1423, member call on a null JSArrayBufferView in getKeyObjectHandleFromJwk).

The new case in test/js/node/crypto/crypto.key-objects.test.ts fails 6/6 without the one-line change in KeyObject.cpp (debug ASAN and release alike) and passes with it.

Automated review came back with no requested changes; no open review threads.

[claude]

LGTM — one-line RETURN_IF_EXCEPTION after the EC d decode, matching every sibling decodeJwkString call in the same function.

What was reviewed:

• Confirmed every other decodeJwkString call site in getKeyObjectHandleFromJwk (x/y/OKP/AKP/RSA) already has this check; this was the lone omission.
• Test follows the file's existing terminate()-race pattern: concurrent pipe drains, error/exit wired to reject, slow-scaled rounds, combined {stdout, stderr, exitCode} assertion.
• The one other unguarded decodeJwkString-shaped site (CryptoHkdf.cpp) is called out as intentionally left to #32258.

Extended reasoning...

Overview

Single-line addition of RETURN_IF_EXCEPTION(scope, {}) after decodeJwkString(globalObject, scope, dView, "key.d"_s) in the EC private key branch of KeyObject::getKeyObjectHandleFromJwk (src/jsc/bindings/node/crypto/KeyObject.cpp:1423). Every other decodeJwkString call in this function — xBuf, yBuf, the OKP dataBuf, the AKP pubBuf/privBuf, and all six RSA fields via getDecodedJwkStringBuf — is already followed by exactly this line. The one for EC d was missing, so a TerminationException surfaced by constructFromEncoding returned null and dBuf->span() dereferenced it.

A new regression test in test/js/web/workers/worker-terminate-lifetime.test.ts spins workers on createPrivateKey({ key: jwk, format: "jwk" }) for an EC key and terminates them mid-import. The d scalar is padded with 768KB of leading zeros (which BN_bin2bn discards, so the key still imports) to make the base64 decode dominate wall-clock and hit the window reliably.

Security risks

None. The added line is a standard exception-propagation guard that returns early with an empty KeyObject when an exception (including the sticky worker TerminationException) is already pending. It does not change validation, key material handling, or any success-path behavior. If anything it closes a process-crash DoS reachable from user JS via worker termination.

Level of scrutiny

Low. This is the canonical "missing RETURN_IF_EXCEPTION after a call that can throw" fix that REVIEW.md calls out explicitly. The correct behavior is unambiguous — it must match the immediately adjacent xBuf/yBuf decodes three lines above — and there is no design decision involved.

Other factors

• The PR description addresses the "fix the whole class" rule: the only other unguarded site with this shape (CryptoHkdf.cpp prepareKey) is named and deferred to open PR #32258, which is an acceptable exclusion per the review guidance.
• The test follows the exact conventions of the neighboring terminate() races in the same file: await using proc, Promise.all([stdout.text(), stderr.text(), proc.exited]), combined-object assertion, ready() wires message/error/exit to resolve/reject, and iteration counts scale on the file-local slow flag with the shared timeout.
• Verification in the description shows fail-before (5/5 UBSan, 6/6 SIGSEGV with USE_SYSTEM_BUN=1) and pass-after (5/5), satisfying the "test must fail on system bun" requirement.
• No CODEOWNERS entries cover this path.

[claude]

LGTM — one-line missing RETURN_IF_EXCEPTION after the EC d decode, matching every sibling decodeJwkString call in the same function.

What was reviewed:

• Confirmed all other decodeJwkString calls in getKeyObjectHandleFromJwk (x/y/OKP/AKP/RSA) already carry the check; this was the lone gap.
• Test follows harness conventions (tempDir, concurrent pipe drain, combined-object assert, isDebug||isASAN round scaling) and awaits worker close events rather than sleeping.
• Checked that the padded d (leading zero bytes) still imports as a valid scalar via BignumPointer, so the pre-postMessage import guards against a vacuous test.

Extended reasoning...

Overview

One production line in src/jsc/bindings/node/crypto/KeyObject.cpp: add RETURN_IF_EXCEPTION(scope, {}) immediately after decodeJwkString(..., dView, "key.d"_s) in the EC private-key branch of getKeyObjectHandleFromJwk. Every other decodeJwkString call in this function already carries this guard; the missing one meant dBuf->span() dereferenced null when constructFromEncoding returned empty with an exception (in practice, a worker TerminationException) pending. Plus a regression test in test/js/node/crypto/crypto.key-objects.test.ts (moved from the worker-terminate file per commit f2f65a4).

Security risks

None. The change only adds an early-return-on-pending-exception; no new parsing, validation loosening, or trust boundary change. The padded d in the test is worker-local and never leaves the process.

Level of scrutiny

Low-to-moderate. This is native crypto binding code, but the change is a mechanical pattern-match to the immediately adjacent lines (xBuf/yBuf decodes two statements above use the identical guard). The failure mode is a null-deref crash, and the fix is the canonical JSC exception-check macro at exactly the site the REVIEW.md "Exception checks after every call that can enter JS" rule requires.

Other factors

The test is well-constructed: it widens the race window by padding d with ~768KB of leading zero bytes (which BN_bin2bn drops, so the scalar is unchanged and the import still succeeds), imports once before postMessage so a broken JWK fails loudly rather than silently passing, awaits worker close events, drains pipes concurrently, and scales rounds down on debug/ASAN. The PR description notes it fails 5/5 without the fix and passes 5/5 with it, and fails under USE_SYSTEM_BUN=1. The description's mention of worker-terminate-lifetime.test.ts is stale (the test was moved to crypto.key-objects.test.ts in the tip commit), but the actual placement next to other KeyObject tests is appropriate. The sibling CryptoHkdf.cpp gap is explicitly deferred to open PR #32258, which is a reasonable scope boundary.