bun:test: fix crash when constructing a mock function via Reflect.construct#31368
bun:test: fix crash when constructing a mock function via Reflect.construct#31368robobun wants to merge 1 commit into
Conversation
JSMockFunction used its call handler as its construct handler, so constructing a mock returned whatever the implementation produced. A native [[Construct]] must return an object, so Reflect.construct(jest.fn(), []) failed an assertion in debug builds and new (jest.fn())() evaluated to undefined. Add a dedicated construct handler that follows ordinary constructor semantics: create this from new.target's prototype, invoke the mock with it, and return the implementation's result only when it is an object, otherwise the new instance.
|
Warning Review limit reached
Your plan includes 5 reviews of capacity. Refill in 26 minutes and 3 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 2:47 PM PT - May 24th, 2026
❌ @robobun, your commit 1b07a4c has 1 failures in
🧪 To try this PR locally: bunx bun-pr 31368That installs a local version of the PR into your bun-31368 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any issues — the construct handler mirrors the established callFrame->setThisValue() pattern from NapiClass.cpp and has solid test coverage — but since this is hand-written JSC [[Construct]] semantics with in-place CallFrame mutation, it's worth a quick human pass.
Extended reasoning...
Overview
This PR fixes a fuzzer-found crash where JSMockFunction registered jsMockFunctionCall as both its call and construct handler. Since [[Construct]] must return an object but the call handler can return undefined, Reflect.construct(jest.fn(), []) hit ASSERT(isCell()) in debug builds (and type-confused in release). The fix adds a dedicated jsMockFunctionConstruct (~25 lines) that synthesizes this from new.target.prototype, mutates the call frame's this-value, delegates to jsMockFunctionCall, and returns the result if it's an object or the new instance otherwise. Two new tests cover new, Reflect.construct, prototype inheritance, primitive-return discarding, and spyOn construction.
Security risks
None identified. This is test-runner-only code (bun:test mock functions), not reachable from production runtime paths. No auth, crypto, or untrusted-input parsing is involved. The get() on new.target.prototype can invoke user code, but that's spec-required behavior and is guarded with RETURN_IF_EXCEPTION.
Level of scrutiny
Medium. The logic is small and correct on inspection — it matches ordinary JS constructor semantics and Jest's behavior, exception scopes are placed correctly, and the callFrame->setThisValue() + delegate pattern is already used identically in src/jsc/bindings/NapiClass.cpp:62. However, this is native JSC binding code that manipulates a CallFrame in place and re-enters another host function with it; subtle invariants around construct frames are the kind of thing a maintainer familiar with Bun's JSC fork should sign off on rather than a bot.
Other factors
- No CODEOWNERS for these files.
- The bug-hunting system found no issues.
- The simpler
get(prototype)+constructEmptyObjectapproach here differs slightly from NapiClass'sgetFunctionRealm/createSubclassStructurepath, but that's a reasonable simplification for mock functions and is covered by the newfn6.prototype = {marker: 3}test. - Test coverage is thorough and the PR description lists the existing suites that still pass.
1 participant
What does this PR do?
Fixes a crash found by fuzzing (Fuzzilli, fingerprint
JSCJSValue.h(1043):ASSERTION FAILED: isCell()).JSMockFunctionregisteredjsMockFunctionCallas both its call and construct handler. A native[[Construct]]implementation must return an object, but the mock returns whatever the mock implementation produced (oftenundefined), so:Reflect.construct(jest.fn(), [])hitASSERT(isCell())insideasObject()in debug builds (type confusion on the returned value in release builds).new (jest.fn())()evaluated toundefinedinstead of an instance object (Jest returns an instance).This PR adds a dedicated construct handler that mirrors ordinary JS constructor semantics (and Jest's behavior for
new mockFn()):thisfromnew.target'sprototype(falling back to a plain object),thisso calls/contexts are recorded against the instance,How did you verify your code works?
mock() > new worksandspyOn > constructing a spy workstotest/js/bun/test/mock-fn.test.js; both fail before this change (new (jest.fn())()returnsundefined,Reflect.constructasserts in debug) and pass after.mock-fn.test.js,spyMatchers.test.ts,mock-disposable.test.ts,mock/mock-module.test.ts,expect-toHaveReturnedWith.test.js,expect/toHaveReturnedWith.test.ts.