Skip to content

bun:test: stop leaking JSC scope objects through mock host functions - #33163

Closed
robobun wants to merge 1 commit into
mainfrom
farm/a1033a16/fix-mock-this-scope-leak
Closed

bun:test: stop leaking JSC scope objects through mock host functions#33163
robobun wants to merge 1 commit into
mainfrom
farm/a1033a16/fix-mock-this-scope-leak

Conversation

@robobun

@robobun robobun commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator

Fixes a deterministic segfault found by fuzzing (fingerprint f3ce0925415c7f37).

What happens

When JavaScript calls a function through a binding that lives in a lexical environment (a const/let that an inner function also references), JSC's bytecode generator puts the resolved JSLexicalEnvironment into the call's implicit this slot (FunctionCallResolveNode::emitBytecode). Ordinary JS callees run it through op_to_this, which maps a JSScope to undefined. Host functions see the raw value, which is why JSC's own built-ins all start with callFrame->thisValue().toThis(globalObject, ECMAMode::strict()).

jsMockFunctionCall did not, so for

const fn = mock().mockReturnThis();
function keep() { return [fn, later]; }
const r = fn();     // r is the JSLexicalEnvironment
typeof r.later;     // segfault: `later` is still in its TDZ
const later = 1;

the scope object escapes to JavaScript in three places: fn.mock.contexts, the this passed to the user implementation, and the return value of mockReturnThis(). The scope's "own properties" are the enclosing function's captured locals, and the slot of a const that has not been initialized yet holds JSC's TDZ marker, the empty JSValue. typeof on that reads the JSType byte of a cell at address 0:

Thread 1 received signal SIGSEGV, Segmentation fault.
0x13a09e93 in llint_op_typeof_is_function ()
=> cmpb $0x21,0x5(%rax)    rax = 0x0

Fix

Run the raw thisValue through JSValue::toThis(globalObject, ECMAMode::strict()), the same thing ArrayPrototype, ObjectPrototype, and ProxyObject::performCall do in JSC. It is an identity for every value except a JSScope, which becomes undefined, so the normal fn(), obj.fn(), and fn.call(x) paths are unchanged. undefined is also what the spec mandates here (a declarative environment record's WithBaseObject() is undefined) and what Jest reports, since its mockConstructor is a strict JS function.

Two other host functions in the same file read callframe->thisValue() without a type check and get the same treatment:

  • JSMock__jsSetSystemTime returns the raw value, so a destructured setSystemTime() called through a captured binding returned the scope.
  • jsMockFunctionGetter_mockGetLastCall only checks isObject(), which a scope passes, then does a get() on it. A captured const calls still in its TDZ would hand an empty JSValue to jsDynamicCast and null-deref.

Every other thisValue() in the file already goes through dynamicDowncast<JSMockFunction>, which rejects a scope.

The same return JSValue::encode(callFrame->thisValue()) pattern exists in BunPlugin.cpp's three builder methods (onLoad, onResolve, module). That is a different module with its own tests, so it is left for a follow-up.

Tests

Added regression tests to mock-fn.test.js (fail before with Received: [native code: JSLexicalEnvironment], pass after) and test-timers.test.ts. The original fuzzer script no longer crashes.

When a function is called through a closure-captured binding, JSC places
the resolved JSLexicalEnvironment in the call's implicit `this` slot.
Ordinary JS callees convert it with op_to_this, but host functions see
the raw value and must run it through JSValue::toThis(), which is what
every built-in host function in JSC does.

jsMockFunctionCall read the raw value, so the scope object escaped into
mock.contexts, the `this` of the mock implementation, and the return
value of mockReturnThis(). Reading a property of that scope whose
binding is still in its temporal dead zone yields an empty JSValue,
which segfaults as soon as anything inspects it.

jsMockFunctionGetter_mockGetLastCall and JSMock__jsSetSystemTime had
the same unchecked read, so they get the same toThis().
@github-actions github-actions Bot added the claude label Jul 1, 2026
@robobun

robobun commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 8:28 PM PT - Jun 30th, 2026

@robobun, your commit 6dabce3 has 1 failures in Build #67455 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 33163

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

bun-33163 --bun

@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

This PR may be a duplicate of:

  1. Don't leak scope objects through host functions' raw this #32172 - Fixes the same scope object leak through host functions' raw thisValue() using the same .toThis(globalObject, ECMAMode::strict()) pattern, and also covers additional files (BunPlugin.cpp, FakeTimers.rs, JSStringDecoder.cpp)

🤖 Generated with Claude Code

@coderabbitai

coderabbitai Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 7b39bffc-2f60-44ed-b682-e57cad37c6c6

📥 Commits

Reviewing files that changed from the base of the PR and between 8973584 and 6dabce3.

📒 Files selected for processing (3)
  • src/jsc/bindings/JSMockFunction.cpp
  • test/js/bun/test/mock-fn.test.js
  • test/js/bun/test/test-timers.test.ts

Walkthrough

JSMockFunction.cpp now normalizes the callframe's this value using toThis(globalObject, strict) in three call sites—mock call recording, mockGetLastCall, and setSystemTime—instead of using the raw thisValue. New tests verify undefined this for bare calls through closure-captured bindings for jest.fn and setSystemTime.

Changes

This-value normalization in mock/timer bindings

Layer / File(s) Summary
Normalize this in mock call recording and getter
src/jsc/bindings/JSMockFunction.cpp, test/js/bun/test/mock-fn.test.js
jsMockFunctionCall and jsMockFunctionGetter_mockGetLastCall normalize thisValue/thisObject via toThis(globalObject, strict) before recording or reading call contexts; new tests assert undefined this and mock.contexts for closure-captured bare jest.fn() calls.
Normalize this in setSystemTime
src/jsc/bindings/JSMockFunction.cpp, test/js/bun/test/test-timers.test.ts
JSMock__jsSetSystemTime normalizes thisValue via toThis and returns the normalized value consistently across the DateInstance branch and final return; a new test asserts setSystemTime() returns undefined for a bare call through a closure-captured binding.

Related PRs: None identified.

Suggested labels: bug, javascript-jsc

Suggested reviewers: None identified.

🐰 A mock's own "this" was wild and free,
Now toThis tames it, strict and tidy,
Bare calls through closures lose their grip,
undefined returns on every trip,
Tests confirm the carrot's now snug and neat. 🥕

🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description explains the fix and tests, but it does not follow the required template headings or include a dedicated verification section. Add the required "What does this PR do?" and "How did you verify your code works?" sections, and summarize the validation steps or tests.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and accurately describes the main change to mock host-function this handling.
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.

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

@robobun

robobun commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator Author

Closing this in favor of #32172, which is the earlier PR for the same bug and covers more of the affected call sites (BunPlugin, JSStringDecoder, and the fake-timers path in addition to JSMockFunction).

One site in this PR is not in #32172 and is worth carrying over when that branch gets rebased: jsMockFunctionGetter_mockGetLastCall only guards with thisObject.isObject(), which a JSScope passes, and then calls get() on it. A captured const calls still in its temporal dead zone hands an empty JSValue to jsDynamicCast<JSArray> and null-derefs. The same one-line toThis(globalObject, ECMAMode::strict()) covers it.

@robobun robobun closed this Jul 1, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — targeted application of the standard toThis(…, ECMAMode::strict()) pattern to stop JSScope objects leaking into JS, with regression tests.

Extended reasoning...

Overview

Three one-line changes in src/jsc/bindings/JSMockFunction.cpp wrap raw callframe->thisValue() reads with .toThis(globalObject, ECMAMode::strict()) in jsMockFunctionCall, jsMockFunctionGetter_mockGetLastCall, and JSMock__jsSetSystemTime. Two regression tests are added to test/js/bun/test/mock-fn.test.js and one to test/js/bun/test/test-timers.test.ts.

Security risks

None. The change strictly reduces what escapes to userland — internal JSLexicalEnvironment objects (which could expose TDZ-marker empty JSValues and segfault on typeof) are now mapped to undefined, which is what the spec and Jest both mandate. toThis in strict mode is an identity for every non-scope value and cannot throw, so no new exception paths are introduced.

Level of scrutiny

Low-to-medium. This is C++ in the JSC bindings, but the fix is a mechanical application of an established idiom already used in this codebase (JSBuffer.cpp, JSDOMOperation.h, JSEventTargetCustom.h) and throughout upstream JSC (ArrayPrototype, ObjectPrototype, ProxyObject::performCall). The PR description traces the crash to FunctionCallResolveNode::emitBytecode and op_to_this, and the author audited every other thisValue() in the file (all go through dynamicDowncast<JSMockFunction>, which already rejects scopes).

Other factors

  • No CODEOWNERS cover the touched paths.
  • The bug-hunting system found no issues.
  • Tests are placed in the existing test files for the affected modules per repo convention, and the author reports they fail-before/pass-after.
  • Normal call shapes (obj.fn(), fn.call(x), jest.setSystemTime() chaining) are unaffected because toThis in strict mode is the identity on ordinary objects.
  • The follow-up for BunPlugin.cpp is explicitly called out and reasonably scoped out of this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant