Skip to content

Normalize scope-object this in mock function calls to prevent leak/crash #31605

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
robobun wants to merge 2 commits into from
+17 −1
Closed

Normalize scope-object this in mock function calls to prevent leak/crash #31605

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
00965e0
mock: normalize scope-object this in mock calls to prevent leak/crash
robobun May 30, 2026
009d791
ci: retrigger
robobun May 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/jsc/bindings/JSMockFunction.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -838,7 +838,11 @@
}

JSC::ArgList args = JSC::ArgList(callframe);
JSValue thisValue = callframe->thisValue();
// For `f()` calls where `f` resolves through a scope, JSC passes the resolved
// JSScope as the unconverted `this` argument and relies on the callee's ToThis to
// normalize it. Host functions never run ToThis, so normalize here to avoid leaking
// an engine-internal scope object through mockReturnThis / the contexts array.
JSValue thisValue = callframe->thisValue().toThis(globalObject, JSC::ECMAMode::strict());

Check notice on line 845 in src/jsc/bindings/JSMockFunction.cpp

View check run for this annotation

Claude / Claude Code Review

Same scope-object leak remains in JSMock__jsSetSystemTime 

Pre-existing: the same raw `callframe->thisValue()` leak fixed here is still present ~600 lines down in `JSMock__jsSetSystemTime` (returns `JSValue::encode(callframe->thisValue())` on both paths, lines ~1462/1467) and in `JSMock__jsUseRealTimers` (~1435). `setSystemTime` is exposed as a top-level `bun:test` export (jest.zig:224-225), so `const { setSystemTime } = require("bun:test"); const c = () => setSystemTime; typeof setSystemTime(0) === "function"` reproduces the same JSLexicalEnvironment l

@claude claude Bot May 30, 2026

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.

🟣 Pre-existing: the same raw callframe->thisValue() leak fixed here is still present ~600 lines down in JSMock__jsSetSystemTime (returns JSValue::encode(callframe->thisValue()) on both paths, lines ~1462/1467) and in JSMock__jsUseRealTimers (~1435). setSystemTime is exposed as a top-level bun:test export (jest.zig:224-225), so const { setSystemTime } = require("bun:test"); const c = () => setSystemTime; typeof setSystemTime(0) === "function" reproduces the same JSLexicalEnvironment leak / LLInt crash. The 7 return callframe.this() sites in FakeTimers.zig (useFakeTimers, useRealTimers, advanceTimersByTime, etc.) have the identical hazard. Since this is the same root cause and same fuzzer fingerprint being closed, it may be worth applying the same .toThis(globalObject, ECMAMode::strict()) (or returning jsUndefined()) to those while you're here.

Extended reasoning...

What the bug is

This PR correctly fixes jsMockFunctionCall by normalizing callframe->thisValue() via toThis(globalObject, ECMAMode::strict()) so that an engine-internal JSScope (e.g. JSLexicalEnvironment) cannot escape to JavaScript through mockReturnThis() or mock.contexts. However, the identical pattern remains unfixed in the same file:

BUN_DEFINE_HOST_FUNCTION(JSMock__jsSetSystemTime, (JSC::JSGlobalObject * globalObject, JSC::CallFrame* callframe))
{
    JSValue argument0 = callframe->argument(0);
    if (auto* dateInstance = dynamicDowncast<DateInstance>(argument0)) {
        ...
        return JSValue::encode(callframe->thisValue());   // <-- raw, no toThis()
    }
    ...
    return JSValue::encode(callframe->thisValue());       // <-- raw, no toThis()
}

and likewise JSMock__jsUseRealTimers at line ~1435. Additionally, src/runtime/test_runner/timers/FakeTimers.zig has 7 host functions (useFakeTimers, useRealTimers, advanceTimersToNextTimer, advanceTimersByTime, runOnlyPendingTimers, runAllTimers, clearAllTimers) that return callframe.this() raw with the same hazard.

Code path that triggers it

JSMock__jsSetSystemTime is registered directly on the bun:test module object in src/runtime/test_runner/jest.zig:224-225:

const setSystemTime = jsc.JSFunction.create(globalObject, "setSystemTime", JSMock__jsSetSystemTime, 0, .{});
module.put(globalObject, "setSystemTime", setSystemTime);

So const { setSystemTime } = require("bun:test") (or import { setSystemTime } from "bun:test") gives JS a bare host-function binding. When that binding is captured by a closure and called as a bare identifier, JSC's FunctionCallResolveNode::emitBytecode passes the resolved JSScope as the unconverted this argument. Native callees never run op_to_this, so callframe->thisValue() is the raw JSLexicalEnvironment, which is then returned straight to JS.

Why existing code doesn't prevent it

The PR description itself notes that ordinary JSFunction-wrapped host natives (citing CallSite.cpp / JSBuffer.cpp as precedent) need explicit toThis() normalization — the wrapper type doesn't shield them. JSMock__jsSetSystemTime is wrapped via jsc.JSFunction.create, which is exactly that case. There is no dynamicDowncast guard on thisValue in this function (unlike e.g. jsMockFunctionMockClear, which requires this to be a JSMockFunction and so can't leak a scope object).

Step-by-step proof

  1. const { setSystemTime } = require("bun:test"); — setSystemTime is now a binding in the local lexical environment.
  2. const capture = () => setSystemTime; — closure-capture forces setSystemTime to live in a heap-allocated JSLexicalEnvironment rather than a register.
  3. const leaked = setSystemTime(0); — bytecode emits resolve_scope → finds the JSLexicalEnvironment → emits call with that scope object in the this slot. The native callee reads it unmodified and returns JSValue::encode(callframe->thisValue()), handing the JSLexicalEnvironment back to JS.
  4. typeof leaked === "function" — llint_op_typeof_is_function on a JSLexicalEnvironment cell dereferences a null cell (or, in release builds, the scope object stringifies as [native code: JSLexicalEnvironment]) — the exact fuzzer fingerprint (signal:SIGNED_RIGHT_SHIFT, SIGSEGV) this PR is closing for mockReturnThis.

Impact

Same impact class as the bug being fixed: an engine-internal scope object becomes JS-observable, and reading a TDZ binding from it surfaces the empty JSValue, crashing the LLInt on a subsequent typeof. It's reachable from any test file that destructures setSystemTime (or the FakeTimers helpers) at module level into a closure-captured binding.

How to fix

Apply the same one-liner: replace callframe->thisValue() with callframe->thisValue().toThis(globalObject, JSC::ECMAMode::strict()) in JSMock__jsSetSystemTime (both return paths) and JSMock__jsUseRealTimers. For the Zig sites in FakeTimers.zig, either normalize via the equivalent toThis binding or simply return .js_undefined when callframe.this() isn't an object — the chaining return value isn't load-bearing for a bare call.

Severity

This is pre-existing — the PR does not touch these functions, add callers, or change their reachability. It's flagged because it's the same root cause, in the same file, producing the same fuzzer fingerprint the PR is meant to close, so the fuzzer will likely rediscover it. Not blocking, but a reasonable "while you're here".

JSC::JSArray* argumentsArray = nullptr;
{
JSC::ObjectInitializationScope object(vm);
Expand Down
12 changes: 12 additions & 0 deletions test/js/bun/test/mock-fn.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,18 @@ describe("mock()", () => {
const obj = { fn };
expect(obj.fn()).toBe(obj);
});
test("mockReturnThis on a closure-captured mock does not leak a scope object", () => {
// When a mock is called as a bare identifier captured by a closure, JSC passes the
// enclosing environment record as the unconverted `this`. mockReturnThis must not hand
// that engine-internal scope object back to JavaScript (reading a TDZ binding from it
// surfaces the empty JSValue and crashes the interpreter on a later `typeof`).
const fn = jest.fn().mockReturnThis();
const capture = () => fn;
expect(capture()).toBe(fn);
const result = fn();
expect(result).toBeUndefined();
expect(typeof result === "function").toBe(false);
});
if (isBun) {
test("jest.fn(10) return value shorthand", () => {
expect(jest.fn(10)()).toBe(10);
Expand Down
Loading