fix(bindings): add missing null checks for invalid `this` in crypto/stream C++ bindings by Jarred-Sumner · Pull Request #28076 · oven-sh/bun

Jarred-Sumner · 2026-03-13T21:40:53Z

Follow-up sweep after #28072 — three more places where jsDynamicCast on thisValue() can return null and the code dereferences it anyway.

Bugs fixed

File	Function	Problem
`JSHmac.cpp`	`jsHmacProtoFuncDigest`	No null check before `hmac->m_finalized`
`JSDiffieHellmanGroup.cpp`	`jsDiffieHellmanGroupGetter_verifyError`	Had `if (!thisObject) { throwVMTypeError(...); }` but no `return` — fell through to `thisObject->getImpl()`
`JSBufferList.cpp`	`JSBufferList_getLength`	Same — `if (!bufferList) throwTypeError(...);` with no `return`, fell through to `bufferList->length()`

Repro (segfaults on current Bun)

const { createHmac } = require("crypto");
const hmac = createHmac("sha256", "key");
const sym = Object.getOwnPropertySymbols(hmac).find(s => s.description === "kHandle");
hmac[sym].digest.call({}); // Segmentation fault at address 0x20

Now throws TypeError [ERR_INVALID_THIS]: Value of "this" must be of type Hmac.

…ings - JSBufferList: add missing early return after throwTypeError in length getter (was falling through to deref null pointer) - JSHmac: add null check in digest before dereferencing hmac->m_finalized - JSDiffieHellmanGroup: use ERR_INVALID_THIS instead of generic TypeError to match Node.js error code

claude · 2026-03-13T21:40:57Z

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review to trigger a review.

robobun · 2026-03-13T21:41:06Z

^{Updated 5:35 PM PT - Mar 13th, 2026}

❌ @Jarred-Sumner, your commit 9ac7b91 has 1 failures in Build #39478 (All Failures):

test/integration/next-pages/test/dev-server.test.ts - code 1 on 🍎 13 aarch64

🧪 To try this PR locally:

bunx bun-pr 28076

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

bun-28076 --bun

coderabbitai · 2026-03-13T21:42:59Z

Walkthrough

Three Node.js crypto binding files now include defensive null-checks and enhanced error handling for invalid this-values. The verifyError getter in DiffieHellmanGroup returns a structured error instead of throwing, JSHmac.digest validates the Hmac object, JSBufferList uses explicit early returns after throws, and a new test file verifies these behaviors don't cause segmentation faults.

Changes

Cohort / File(s)	Summary
Crypto Bindings - Error Handling `src/bun.js/bindings/JSBufferList.cpp`, `src/bun.js/bindings/node/crypto/JSDiffieHellmanGroup.cpp`, `src/bun.js/bindings/node/crypto/JSHmac.cpp`	Enhanced error handling with defensive null-checks for invalid this-values. JSBufferList adds explicit early returns post-throw with unlikely branch hints; DiffieHellmanGroup verifyError now returns INVALID_THIS error instead of throwing; JSHmac.digest adds validation for Hmac object validity.
Crypto Invalid This Tests `test/js/node/crypto/crypto-invalid-this.test.ts`	New unit tests verifying native crypto methods don't segfault when called with invalid this context (null, {}, primitives), confirming ERR_INVALID_THIS is thrown instead. Tests cover Hmac.digest() and DiffieHellmanGroup.verifyError.

🚥 Pre-merge checks | ✅ 2

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main change: adding null checks for invalid `this` in crypto/stream C++ bindings, which is the primary objective of this PR.
Description check	✅ Passed	The description provides detailed bug fixes with a table showing affected files and functions, includes a reproduction case demonstrating the issue, and explains the resulting behavior—covering both required template sections effectively.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

📝 Coding Plan

Generate coding plan for human review comments

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

coderabbitai

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/bun.js/bindings/node/crypto/JSHmac.cpp (1)

117-123: ⚠️ Potential issue | 🔴 Critical

jsHmacProtoFuncUpdate still dereferences null this

Line 118 can return nullptr for invalid receivers, but Line 121 dereferences hmac unconditionally. This leaves a remaining segfault path (native.update.call({})) even after fixing digest.

Suggested fix

     JSValue thisHmac = callFrame->thisValue();
     JSHmac* hmac = jsDynamicCast<JSHmac*>(thisHmac);
+    if (!hmac) [[unlikely]] {
+        return Bun::ERR::INVALID_THIS(scope, globalObject, "Hmac"_s);
+    }
 
     // Check if the HMAC is already finalized
     if (hmac->m_finalized) {
         return Bun::ERR::CRYPTO_HASH_FINALIZED(scope, globalObject);
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/bun.js/bindings/node/crypto/JSHmac.cpp` around lines 117 - 123, The
update method (jsHmacProtoFuncUpdate) can get a null/invalid receiver because
jsDynamicCast<JSHmac*>(callFrame->thisValue()) may return nullptr, but the code
dereferences hmac immediately to check m_finalized; add a null check after
obtaining hmac and return the appropriate crypto/invalid-receiver error (the
same pattern used in digest) before accessing hmac->m_finalized. Locate
jsHmacProtoFuncUpdate, the call to
callFrame->thisValue()/jsDynamicCast<JSHmac*>, and ensure you guard hmac ==
nullptr and return Bun::ERR::CRYPTO_HASH_FINALIZED or the matching
invalid-receiver error path (consistent with existing error helpers) instead of
dereferencing a null pointer.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@test/js/node/crypto/crypto-invalid-this.test.ts`:
- Around line 6-9: The getNativeHandle helper currently assumes the "kHandle"
symbol exists and will produce a confusing runtime error when it doesn't; update
the function (getNativeHandle) to explicitly check the result of
Object.getOwnPropertySymbols(obj).find(s => s.description === "kHandle") and
throw or assert with a clear message (e.g. "kHandle symbol not found on object")
when sym is undefined so failures point directly to handle discovery rather than
later when accessing obj[sym].

---

Outside diff comments:
In `@src/bun.js/bindings/node/crypto/JSHmac.cpp`:
- Around line 117-123: The update method (jsHmacProtoFuncUpdate) can get a
null/invalid receiver because jsDynamicCast<JSHmac*>(callFrame->thisValue()) may
return nullptr, but the code dereferences hmac immediately to check m_finalized;
add a null check after obtaining hmac and return the appropriate
crypto/invalid-receiver error (the same pattern used in digest) before accessing
hmac->m_finalized. Locate jsHmacProtoFuncUpdate, the call to
callFrame->thisValue()/jsDynamicCast<JSHmac*>, and ensure you guard hmac ==
nullptr and return Bun::ERR::CRYPTO_HASH_FINALIZED or the matching
invalid-receiver error path (consistent with existing error helpers) instead of
dereferencing a null pointer.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: da50bb31-f1e6-4e1f-970a-d3dd64c27c18

📥 Commits

Reviewing files that changed from the base of the PR and between 2964b4a and 5596635.

📒 Files selected for processing (4)

src/bun.js/bindings/JSBufferList.cpp
src/bun.js/bindings/node/crypto/JSDiffieHellmanGroup.cpp
src/bun.js/bindings/node/crypto/JSHmac.cpp
test/js/node/crypto/crypto-invalid-this.test.ts

alii · 2026-03-13T21:57:45Z

@robobun adopt

robobun

All gates pass.

Gate 1: CI

Build #39467 — all test jobs green across Linux (x64, aarch64, musl, baseline), macOS (x64, aarch64), Windows (x64, baseline), Alpine, Ubuntu, and ASAN. The only failure is upload-benchmark.mjs which also fails on main (infra flake). windows-11-aarch64-test-bun is stuck pending — this check doesn't exist on main, likely a new/unavailable runner.

Gate 2: Classification

Bug fix — three null dereference segfaults in C++ bindings.

Gate 3: Test proof

Baked binary (main) FAILS ✅: build/debug/bun-debug test crypto-invalid-this.test.ts → UndefinedBehaviorSanitizer: undefined-behavior JSHmac.cpp:177:15 — member access within null pointer of type 'JSHmac'
PR binary PASSES ✅: CI ran the new test on all platforms including ASAN — all passed.
New test file test/js/node/crypto/crypto-invalid-this.test.ts covers Hmac.digest and DiffieHellmanGroup.verifyError with invalid this values ({}, null, 42).

Gate 4: Diff

No TODO/FIXME/HACK. All three fixes are minimal and correct:

JSHmac.cpp: Added missing null check + return ERR::INVALID_THIS
JSDiffieHellmanGroup.cpp: Replaced no-return throwVMTypeError with return ERR::INVALID_THIS
JSBufferList.cpp: Added missing return {} after throw

Fixes match the root cause described in the PR body.

Gate 5: Bot convergence

One coderabbit nitpick (add expect(sym).toBeDefined() in test helper) — resolved as noise: test helper, not production code, failure already clear.

Gate 6: Hygiene

Clean PR body with repro, table of bugs, and correct scope. No scope creep.

…tream C++ bindings (oven-sh#28076) Follow-up sweep after oven-sh#28072 — three more places where `jsDynamicCast` on `thisValue()` can return null and the code dereferences it anyway. ## Bugs fixed | File | Function | Problem | |---|---|---| | `JSHmac.cpp` | `jsHmacProtoFuncDigest` | No null check before `hmac->m_finalized` | | `JSDiffieHellmanGroup.cpp` | `jsDiffieHellmanGroupGetter_verifyError` | Had `if (!thisObject) { throwVMTypeError(...); }` but no `return` — fell through to `thisObject->getImpl()` | | `JSBufferList.cpp` | `JSBufferList_getLength` | Same — `if (!bufferList) throwTypeError(...);` with no `return`, fell through to `bufferList->length()` | ## Repro (segfaults on current Bun) ```js const { createHmac } = require("crypto"); const hmac = createHmac("sha256", "key"); const sym = Object.getOwnPropertySymbols(hmac).find(s => s.description === "kHandle"); hmac[sym].digest.call({}); // Segmentation fault at address 0x20 ``` Now throws `TypeError [ERR_INVALID_THIS]: Value of "this" must be of type Hmac`. --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>

[autofix.ci] apply automated fixes

5596635

coderabbitai Bot reviewed Mar 13, 2026

View reviewed changes

Comment thread test/js/node/crypto/crypto-invalid-this.test.ts

robobun approved these changes Mar 13, 2026

View reviewed changes

Jarred-Sumner merged commit 636c638 into main Mar 13, 2026
59 of 64 checks passed

Jarred-Sumner deleted the claude/fix-invalid-this-crypto-bindings branch March 13, 2026 22:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(bindings): add missing null checks for invalid `this` in crypto/stream C++ bindings#28076

fix(bindings): add missing null checks for invalid `this` in crypto/stream C++ bindings#28076
Jarred-Sumner merged 2 commits into
mainfrom
claude/fix-invalid-this-crypto-bindings

Jarred-Sumner commented Mar 13, 2026

Uh oh!

claude Bot commented Mar 13, 2026

Uh oh!

robobun commented Mar 13, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Mar 13, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

alii commented Mar 13, 2026

Uh oh!

robobun left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

Jarred-Sumner commented Mar 13, 2026

Bugs fixed

Repro (segfaults on current Bun)

Uh oh!

claude Bot commented Mar 13, 2026

Claude Code Review

Uh oh!

robobun commented Mar 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Mar 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alii commented Mar 13, 2026

Uh oh!

robobun left a comment

Choose a reason for hiding this comment

Gate 1: CI

Gate 2: Classification

Gate 3: Test proof

Gate 4: Diff

Gate 5: Bot convergence

Gate 6: Hygiene

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

robobun commented Mar 13, 2026 •

edited

Loading

coderabbitai Bot commented Mar 13, 2026 •

edited

Loading