Don't leak scope objects through host functions' raw this - #32172
Conversation
WalkthroughThis PR implements strict-mode ChangesStrict
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/test/mock-fn.test.js`:
- Around line 489-504: The test never exercises the closure-captured "bare call"
path because capture() is not invoked; instead local aliases are called
directly. Fix by calling the functions returned from capture() (e.g., const
[returnsThis, impl] = capture(); or const tuple = capture(); then invoke
tuple[0]() and tuple[1]()) and then assert on their return values and
mock.contexts so the activation-object path for returnsThis and impl is actually
exercised.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2eeba76e-53cf-44b3-9da4-d706c0c63b8e
📒 Files selected for processing (10)
src/jsc/JSValue.rssrc/jsc/bindings/BunPlugin.cppsrc/jsc/bindings/JSMockFunction.cppsrc/jsc/bindings/JSStringDecoder.cppsrc/jsc/bindings/bindings.cppsrc/runtime/test_runner/timers/FakeTimers.rstest/js/bun/plugin/plugins.test.tstest/js/bun/test/mock-fn.test.jstest/js/bun/test/test-timers.test.tstest/js/node/string_decoder/string-decoder.test.js
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Closed #31605 in favor of this PR; it contained only the jsMockFunctionCall hunk, which is included here unchanged. |
![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, but this touches the JSC bindings layer across C++ and Rust (including a control-flow refactor in JSStringDecoder::construct and a new FFI binding), so it's worth a human look.
Extended reasoning...
Overview
This PR fixes a fuzzer-found segfault by sanitizing raw this values in host functions before they escape to user JS. When a bare call resolves through a closure scope, JSC passes the JSLexicalEnvironment as the raw this; several Bun host functions returned or stored that value verbatim, leaking the activation object and ultimately allowing an empty TDZ slot to reach JIT code as a null cell.
The fix applies JSValue::toThis(..., ECMAMode::strict()) (identity for everything except scope objects, which become undefined) at each escape point: jsMockFunctionCall, the jest fake-timer methods (C++ setSystemTime and seven Rust host fns), the Bun.plugin builder chaining returns, and StringDecoder's construct path. It adds a new JSC__JSValue__toThisStrict FFI binding exposed as JSValue::to_this_strict in Rust, and deletes the dead C++ JSMock__jsUseRealTimers (verified: no remaining references). Four test files gain regression tests.
Security risks
The bug being fixed is itself security-adjacent: leaking a JSLexicalEnvironment into user JS lets scripts read raw TDZ slots (empty JSValues), which the JIT then dereferences as a null cell. The fix narrows the attack surface. The change uses strict-mode toThis, which is identity for all ordinary values, so normal call paths (obj.method(), fn.call(x)) are unchanged. I don't see new risks introduced — but engine-bindings changes are exactly where subtle mistakes have outsized consequences.
Level of scrutiny
This warrants careful human review. While most hunks are a mechanical .toThis(...) suffix, the PR also:
- Refactors
JSStringDecoder::construct: the old code didasObject(callFrame->newTarget())(asserting onundefinedwhen called withoutnew) and comparedJSObject* != JSObject*; the new code keepsnewTargetas aJSValueand comparesJSValue != JSObject*. The express body-parser hack path is preserved (regular-objectthisstill gets the privatedecode/encodingproperties), but the comparison semantics changed and deserve a second pair of eyes. - Adds a new cross-language FFI binding marked
[[ZIG_EXPORT(nothrow)]]/safe fn— thetoThisstrict path is documented as non-throwing, which matches JSC'sJSScope::toThisreturningjsUndefined(), but it's a new ABI surface. - Claims an audit of remaining
thisValue()uses insrc/jsc/bindingsandsrc/runtime— a substantive completeness claim worth spot-checking.
Other factors
- No CODEOWNERS cover the touched files.
- Each fix site has a regression test that the description says fails on current bun and passes with the change.
- I verified
JSMock__jsUseRealTimershas no remaining references, so its removal is safe. - The bug-hunting system found nothing; I also found nothing. The change looks correct to me, but it's core runtime/engine-bindings code rather than a trivial config or doc change.
There was a problem hiding this comment.
I didn't find any issues, but this touches the JSC host-function this handling across several subsystems (mock call path, plugin builder, fake timers, and a non-trivial refactor of the StringDecoder constructor's newTarget/thisValue logic), so it's worth a human pass.
Extended reasoning...
Overview
This PR fixes a fuzzer-found segfault by sanitizing the raw this value that JSC passes to host functions when the callee is resolved through a closure scope (a JSLexicalEnvironment activation object). It adds a new JSC__JSValue__toThisStrict C++ binding plus a Rust JSValue::to_this_strict wrapper, then applies toThis(..., ECMAMode::strict()) at every site where a callframe's raw this was previously returned or stored into user-visible JS: jsMockFunctionCall (mock.contexts / mockReturnThis / forwarded impl this), JSMock__jsSetSystemTime, the seven Rust fake-timer host functions, the three Bun.plugin builder chaining returns, and the StringDecoder constructor's express/body-parser hack branch. It also deletes the dead C++ JSMock__jsUseRealTimers (the live impl is the Rust one, which is patched here). Four regression tests are added, each constructed so the binding lives in the activation and the bare call goes through the scope-resolve path.
Security risks
The bug being fixed is itself the security-relevant part: leaking a JSLexicalEnvironment into user JS exposes raw TDZ slots (empty JSValue) and led to a null-cell deref in JIT code. The fix is strictly a narrowing — strict-mode toThis is identity for every value except scope objects, which become undefined — so normal method calls (jest.useFakeTimers(), builder.onLoad(...)) are unchanged and the new test asserts that. I don't see new injection, auth, or data-exposure surface introduced. The PR explicitly leaves the NAPI / V8-shim raw-this surface unchanged, calling it a separate compatibility question; that scoping decision is reasonable but worth a maintainer's nod.
Level of scrutiny
Medium-high. The per-call-site pattern is mechanical, but this is the core JSC host-function boundary and the changes span C++ and Rust bindings. The JSStringDecoder::construct hunk is the least mechanical: it renames thisValue → newTarget, drops the asObject(newTarget) that asserted in debug when called without new, recomputes thisValue via toThis, and re-derives thisObject from the sanitized value — a small refactor of constructor semantics that deserves a careful look even though it tests green. The dead-code removal of JSMock__jsUseRealTimers also warrants confirmation that nothing references it.
Other factors
The bug hunter found nothing. CodeRabbit's only inline comment (about the capture() test pattern) was withdrawn after the author demonstrated empirically that the test fails on the unfixed build. CI shows two failures (terminal.test.ts on macOS x64, init.test.ts on macOS aarch64) that look unrelated to this diff. None of the touched paths are CODEOWNER-gated. Given the breadth across the bindings layer and the constructor-path refactor, deferring to a human reviewer.
|
CI status for head I sampled four failed shards (darwin-26-aarch64, debian-13-x64, alpine-3.23-aarch64, windows-11-aarch64). Each has exactly one
For the previous head Locally against the rebased build the four regression suites are green: mock-fn 77, test-timers 5, string-decoder 96, plugins 35, 0 failing, including every test Earlier heads, each a single unrelated or infra failure: build 68949 (Buildkite agent incident affecting every build in its window plus the autobahn Docker arch mismatch, zero real My single retrigger is spent. Once #33916 lands on |
|
Another Fuzzilli crash reduces to the root cause fixed by the Deterministic repro on current main (00a93bd), no function outer() {
const fn = Bun.jest().mock(); // jest.fn()
fn.mockReturnThis();
const leaked = fn(); // should be undefined, is the activation object
const got = leaked.tdz; // empty JSValue read out of the TDZ slot
console.log(typeof got); // null JSCell deref
const tdz = 123;
function capture() {
return [fn, tdz];
}
}
outer();Not opening a separate PR for this fingerprint since the |
7357c74 to
8753b85
Compare
There was a problem hiding this comment.
No issues found, but this touches JSC bindings across five subsystems (mock, fake timers, plugins, StringDecoder) plus a new Rust↔C++ FFI export and a subtle newTarget/thisValue refactor in JSStringDecoderConstructor::construct, so it's worth a human pass.
Extended reasoning...
Overview
Fixes a fuzzer-found segfault where JSLexicalEnvironment scope objects leaked to user JS through host functions' raw callFrame->thisValue(). Applies toThis(…, ECMAMode::strict()) at every escape point: jsMockFunctionCall, JSMock__jsSetSystemTime, the seven Rust fake-timer host functions (via a new JSC__JSValue__toThisStrict binding + JSValue::to_this_strict wrapper), the three Bun.plugin builder returns, and JSStringDecoderConstructor::construct. Also deletes a dead JSMock__jsUseRealTimers C++ function. 10 files changed across src/jsc/bindings/, src/jsc/JSValue.rs, src/runtime/test_runner/timers/, and four test files.
Security risks
The change reduces risk — it closes an internal-object leak that let user JS read empty TDZ slots and crash via null-cell deref. toThis in strict mode is identity for every value except scope objects (→ undefined), so normal calls are unchanged. The new [[ZIG_EXPORT(nothrow)]] binding is a thin wrapper over JSC's existing toThis. No new attack surface introduced.
Level of scrutiny
High. This is JSC binding code on the crash/memory-safety boundary. The JSStringDecoder hunk is more than a one-liner: it renames thisValue→newTarget, drops an asObject(newTarget) that asserted in debug when called without new, and rebases the express-body-parser "hack" branch condition on the sanitized thisValue instead of the raw one. That's correct as far as I can tell, but it changes control flow in a Node-compat quirk path and deserves a maintainer's eyes.
Other factors
Tests are thorough and empirically verified to fail on the unfixed build (the CodeRabbit thread confirms the capture() pattern is load-bearing). CI is green modulo unrelated darwin flakes per robobun. The PR description explicitly audits remaining thisValue() sites and scopes out NAPI/V8 shims. A second Fuzzilli fingerprint was noted as covered by the same fix. All of this points to a well-executed change — just not one I'd auto-approve given the breadth across critical binding code.
8753b85 to
6076ef9
Compare
There was a problem hiding this comment.
I didn't find any bugs, but this is a crash fix that touches JSC bindings across C++ and Rust (including a new FFI export and a reworked JSStringDecoder constructor path), so it's worth a human look before merging.
Extended reasoning...
Overview
This PR fixes a fuzzer-found segfault where JSLexicalEnvironment scope objects leak into user JS through host functions that return/store the raw callFrame->thisValue(). It applies toThis(..., ECMAMode::strict()) at ~12 sites across four C++ bindings files (JSMockFunction.cpp, BunPlugin.cpp, JSStringDecoder.cpp, bindings.cpp), adds a new JSC__JSValue__toThisStrict FFI export with a Rust wrapper in JSValue.rs, and updates seven Rust host functions in FakeTimers.rs. Four test files gain regression coverage.
Security risks
The bug being fixed is itself security-relevant: leaking an activation object lets user JS read TDZ slots as empty JSValues, which the JIT then dereferences as a null cell (segfault; UBSAN "member call on null pointer"). The fix is defensive and strict-mode toThis is identity for everything except scope objects, so behavioral risk to normal callers is low. No new attack surface is introduced, but the correctness of the JSStringDecoder constructor rework (separating newTarget from the sanitized thisValue and re-gating the express body-parser hack path) deserves a second pair of eyes.
Level of scrutiny
Moderate-to-high. While most hunks are mechanical one-liners, the change spans C++/Rust FFI, adds a new exported binding, non-trivially refactors a constructor's newTarget/this handling, and went through a rebase with two hand-resolved conflicts (JSMock__jsSetSystemTime and test-timers.test.ts). Crash fixes in JSC binding code are exactly the category CLAUDE.md flags for careful review.
Other factors
The PR has thorough regression tests that the author verified fail on USE_SYSTEM_BUN=1 and pass on the fixed build, and the CodeRabbit thread on the test shape was resolved with a convincing empirical explanation. CI failures on the latest head are documented as unrelated tree-wide flakes. No prior human review is on the timeline. Given the breadth (10 files, 4 subsystems) and the JSC-internals subject matter, deferring rather than auto-approving.
6076ef9 to
fe2f52d
Compare
There was a problem hiding this comment.
I didn't find any bugs, but this is a systematic sweep across C++ and Rust JSC bindings (plus a new FFI surface) fixing a crash-class issue, so it's worth a human confirming the toThis approach and the completeness of the thisValue() audit.
Extended reasoning...
Overview
This PR fixes a fuzzer-found segfault where JSC's JSLexicalEnvironment (activation object) leaks into user JavaScript when host functions are called as bare functions through closure-captured bindings. The fix applies JSValue::toThis(..., ECMAMode::strict()) at every site where a raw callframe->thisValue() was returned or stored: three plugin builder methods in BunPlugin.cpp, jsMockFunctionCall and jsSetSystemTime in JSMockFunction.cpp, the StringDecoder constructor in JSStringDecoder.cpp, and seven fake-timer host functions in FakeTimers.rs. It also adds a new JSC__JSValue__toThisStrict C++ binding exposed as JSValue::to_this_strict in Rust, and regression tests in four test files.
Security risks
The bug being fixed is itself safety-relevant: leaking an engine-internal scope object to user JS allowed reading TDZ slots as empty JSValues, which the JIT then dereferenced as a null cell (segfault / UBSAN null-pointer member call). The fix closes that surface. I don't see new risks introduced — strict-mode toThis is identity for every value except scope objects, so normal method calls (jest.useFakeTimers(), builder.onLoad(...)) preserve their existing chaining behavior, which the tests confirm.
Level of scrutiny
This warrants human review. While the per-site pattern is mechanical and consistent, the change spans six native source files in the JSC bindings layer (the most correctness-critical part of the codebase), introduces a new FFI entry point, and includes a non-trivial refactor of the JSStringDecoder constructor (the newTarget variable changes from JSObject* to JSValue, and the express body-parser hack branch now derives thisObject from the sanitized thisValue rather than the raw callframe). A maintainer should confirm that strict-mode toThis is the right sanitization primitive here and that the author's audit of remaining thisValue() sites (host_fn shims, NAPI/V8 layers deliberately left unchanged) is complete.
Other factors
The bug-hunting system found nothing. Each fixed site has a targeted regression test that the author verified fails on the unfixed build for the right reason (per the CodeRabbit thread, which was resolved after empirical verification of the closure-capture trigger). CI on the rebased head passed all tests, with only infra-related job failures. The PR description is thorough and traces the crash mechanism precisely. Nothing here looks wrong to me — I'm deferring purely on scope and criticality, not on any specific concern.
When a call like f() resolves the callee through a closure scope, JSC passes the scope object (JSLexicalEnvironment) as the raw this value and relies on the callee to run toThis. JS functions do that in their prologue; host functions must call JSValue::toThis themselves. Several host functions returned or stored callframe->thisValue() raw, leaking the activation object to user JS. Reading a TDZ slot through the leaked object then yields an empty JSValue, which crashes JIT'd type checks (segfault dereferencing a null cell). Apply toThis with strict semantics (identity for everything except scope objects, which become undefined) at each exposure: - jsMockFunctionCall: mock.contexts, mockReturnThis, and the this forwarded to implementations - jest.setSystemTime (C++) and the Rust fake timer methods, via a new JSC__JSValue__toThisStrict binding and JSValue::to_this_strict - Bun.plugin builder onLoad/onResolve/module chaining returns - StringDecoder called without new: stop writing encoding onto the activation object and returning it; also fixes a debug assert when this is undefined (asObject on a non-cell) Delete the dead C++ JSMock__jsUseRealTimers; the live implementation is in FakeTimers.rs.
fe2f52d to
58767dd
Compare
There was a problem hiding this comment.
I didn't find any bugs, but this crash fix spans JSC bindings across C++ and Rust (including a new FFI export and reworked newTarget/thisValue handling in JSStringDecoder), so it's worth a human look.
Extended reasoning...
Overview
This PR fixes a fuzzer-found segfault by applying toThis(strict) to raw callFrame->thisValue() at every site where a host function returns or stores it, so a JSLexicalEnvironment passed by a scope-resolved bare call cannot leak into user JS. It touches JSMockFunction.cpp (jsMockFunctionCall, jsSetSystemTime), BunPlugin.cpp (three builder methods), JSStringDecoder.cpp (constructor), FakeTimers.rs (seven timer host functions), and adds a new JSC__JSValue__toThisStrict binding in bindings.cpp exposed as JSValue::to_this_strict in JSValue.rs. Four regression tests cover each surface.
Security risks
The bug itself is security-adjacent: leaking a JSLexicalEnvironment lets user JS read TDZ slots as empty JSValues, which then null-deref in JIT code. The fix is strictly narrowing (scope objects → undefined, everything else identity), so it does not introduce new attack surface. The new FFI export is a thin nothrow wrapper around JSC's existing toThis.
Level of scrutiny
Medium-high. The per-site pattern is mechanical, but the change spans C++ and Rust JSC bindings, adds a cross-FFI export, and the JSStringDecoder hunk reworks control flow: it separates newTarget from thisValue (previously conflated via asObject(callFrame->newTarget()), which asserted on undefined) and re-derives the express-body-parser hack path from the sanitized this. That is a real behavior change worth a maintainer's eyes.
Other factors
The PR has been rebased four times over conflicting changes to the same functions (WebKit upgrade, setSystemTime rework, JSC C API removal), and the description documents each resolution. The bug-hunting system found nothing, CI is green on all shards that ran, and the CodeRabbit thread on the test shape was resolved with an empirical demonstration. Still, given the breadth across critical binding code and the number of merge resolutions, I'm deferring rather than auto-approving.
|
Closing as superseded. Every hunk in this PR is now in a PR that merges cleanly against main: the mock function call and |
… fs.Stats, Node-API and V8 callbacks (#39525) ### Problem - When JS calls a function with no receiver through a binding that a closure captures (or a module binding), the callee's this slot holds the scope object that the binding was resolved through. This is the normal shape of `const { x } = require(...)` followed by `x()` inside any function. JS callees convert that slot in `op_to_this`; host functions read it raw. The native functions changed in this PR use their receiver generically and so act on the scope object: - `NodeError_proto_toString` (`src/jsc/bindings/ErrorCode.cpp:55`) reads `name`/`code`/`message` through it. A binding in the scope that is still in its TDZ reads back as the empty `JSValue`, and the process dies with `panic(main thread): Segmentation fault at address 0x5`. Without a TDZ binding it returns `"undefined [undefined]: undefined"`. Node throws a `TypeError`. - `fs.Stats` / `BigIntStats` `isFile()` and the other mode methods (`src/jsc/bindings/NodeFSStatBinding.cpp:142`) read `mode` through it: the same segfault with a TDZ `mode` binding, otherwise a bogus `false` (or a `TypeError` from `toBigInt64` for the BigInt variant). The `atime`/`mtime`/`ctime`/`birthtime` accessors in the same file (`getDateField`, `DatePutter`) have the same shape once their getter is pulled out of the property descriptor: JSC's `JSCustomGetterFunction` wrapper also passes the slot through raw, so a bare call segfaults on a TDZ `atimeMs` binding and otherwise returns an Invalid Date. - `StringDecoder(...)` called without `new` (`src/jsc/bindings/JSStringDecoder.cpp:596`) treats it as the body-parser style receiver: it writes the decoder state onto it and returns it, so the caller gets `[native code: JSLexicalEnvironment]` instead of a decoder. The same code also ran `asObject()` on the slot before checking that it holds an object, which aborts debug builds for `StringDecoder.call(undefined, enc)`. - `NAPICallFrame` (`src/jsc/bindings/napi.h:986`) and the V8 shim's `FunctionTemplate::functionCall` and `invokeAccessor` (`src/jsc/bindings/v8/shim/FunctionTemplate.cpp:151`, `shim/TemplateProperty.cpp:64`) emulate a sloppy-mode receiver by hand: undefined/null become globalThis, everything else goes through `toObject()`. A scope object is already an object, so the addon receives it as `this_arg` / `info.This()` / `HolderV2()`. Node gives it globalThis. - All of the above reproduce on the current release build (repros in the details block). - Root cause: `FunctionCallResolveNode::emitBytecode` (JavaScriptCore bytecompiler) stores the resolve-scope result in the this register and leaves the conversion to the callee. Converting `JSScope` receivers once, where JSC enters a host function, would retire the whole class; that is a change to the WebKit fork. Until then the fix is the one JSC's own host functions use: call `JSValue::toThis` before using the receiver generically. Bun already does this in `JSBuffer.cpp`, `JSDOMOperation.h` and `JSEventTargetCustom.h`. ### Fix - `NodeError_proto_toString`, the Stats mode methods and date accessors, and the `StringDecoder` constructor convert the receiver with `toThis(globalObject, ECMAMode::strict())` first. Strict `toThis` maps scope objects to `undefined` and returns every other value unchanged, so `err.toString()`, `stats.isFile()`, `stats.atime`, `StringDecoder.call(obj, enc)` and `new StringDecoder()` are unaffected. The scope receiver then takes the path an undefined receiver already took: `toString()` throws the `TypeError` Node throws, `isFile()` and the date getters return `undefined` like they do for `.call(undefined)`, and `StringDecoder()` returns a fresh decoder. The Stats accessors only ever live on the two Stats prototypes, never on the global object, so no legitimate property access can hand them a scope receiver. - The `StringDecoder` condition compares the converted receiver as a `JSValue` instead of calling `asObject()` on the slot. On a construct call the slot holds `new.target`, which is always a constructor object, so construct calls select the same branch as before. - `NAPICallFrame`, `FunctionTemplate::functionCall` and `invokeAccessor` replace the hand-rolled logic with `toThis(globalObject, ECMAMode::sloppy())`, which is the engine's definition of a sloppy receiver: undefined, null and scope objects become globalThis, primitives are boxed, other objects pass through. This is what the old code computed for every input except scope objects, and it is what Node hands addons. Every Node-API callable (`napi_create_function`, class constructors, methods, accessors) is a `NapiClass` that builds its frame through this one constructor. Sloppy `toThis` has no throwing path, so the existing `assertNoException()` still holds. - Verified with the debug build. Each new test fails on the release build as described: - `test/js/node/errors/error-code-toString-receiver.test.ts`: bare call throws (release: returns the garbage string), TDZ variant in a child exits 0 (release: exit 139). - `test/js/node/fs/fs-stats-constructor.test.ts`: bare `isFile()` / BigInt `isDirectory()` return `undefined` (release: `false` / `TypeError`), the extracted `atime` / BigInt `mtime` getters return `undefined` (release: Invalid Date), TDZ variant in a child exits 0 (release: exit 139). The `DatePutter` hunk has no test of its own: a setter called on a scope object only wrote onto that object, which JS cannot observe. - `test/js/node/string_decoder/string-decoder.test.js`: captured bare call returns a decoder (release: `[native code: JSLexicalEnvironment]`), explicit object receiver still initialized, undefined/primitive receivers in a child (unfixed debug build: SIGABRT in `asObject`). - `test/napi/napi.test.ts` (new `return_this` helper in the test addon) and `test/v8/v8.test.ts` (existing `return_this`): output compared against node; release prints `false` for the closure line where node prints `true`, the `call(undefined)` / `call(5)` / `call(receiver)` lines already matched. - `test/v8/v8.test.ts` accessor coverage (new `create_object_with_holder_accessor` fixture): property access compared against node, plus a Bun-only test that calls the getter and setter functions with bare, undefined, null, primitive and object receivers. Release differs only on the two bare-call lines. - Full `napi.test.ts` (175 pass) and `v8.test.ts` (77 pass, 1 pre-existing skip), the js-native-api suites for functions, callbacks, constructors, object wrap, properties and new.target, node's `test-fs-stat*` and `test-string-decoder*`, and the existing string_decoder, stats and error-code tests all pass. - Known sites of the same shape that this PR leaves alone: - `import.meta.resolve` / `resolveSync` (`ImportMetaObject.cpp`): a bare call already throws "must be bound to an import.meta object"; Node resolves it, because its function is bound to the module. That needs a per-module bound function (see #32248), not `toThis`. - `NodeVMScript` methods and other typed-receiver sites report the bad receiver as `Received [native code: JSLexicalEnvironment]` in the error message. Cosmetic, no crash. - Other custom accessors reached through `Object.getOwnPropertyDescriptor(...).get` get the raw receiver from JSC's `JSCustomGetterFunction` wrapper in the same way. The Stats ones are fixed here because they are in a file this PR already changes; the general fix is in the wrapper, which lives in the WebKit fork and covers every custom accessor at once. Note that `toThis` is not safe to add blindly to a custom accessor that can be installed on the global object, because a bare identifier read resolves on the global object itself, which is also a `JSScope`. - Relation to the other open PRs for this root cause: #39207 fixes the chaining functions and the `inspect.custom` fallbacks, #39509 the mock functions and `createInvalidThisError`. This PR shares no file with #39207 and touches a different function of `ErrorCode.cpp` than #39509, so the three merge in any order. #32172 was the first attempt; every hunk in it is now in one of these three, and it no longer merges, so it is closed in favor of them. ### Background - **this slot / `op_to_this`**: every call frame has a slot for the receiver. For `f()` where `f` lives in a scope rather than a local register, JSC stores the scope object it resolved `f` through in that slot and JS function prologues run `op_to_this`, which turns it into `undefined` (strict code) or globalThis (sloppy code). Native functions read the slot directly via `callFrame->thisValue()`. - **`JSValue::toThis(globalObject, ECMAMode)`**: the runtime form of that conversion. Any object inheriting `JSScope` (`JSLexicalEnvironment` for closures, `JSModuleEnvironment` for ES modules, the global object itself) becomes `undefined` in strict mode or globalThis in sloppy mode. Strict mode returns every other value as is. Sloppy mode also turns undefined/null into globalThis and boxes primitives. - **Scope objects and the TDZ**: property reads on a scope object go through its symbol table and return the raw variable slot. A `let`/`const` that is not initialized yet holds the empty `JSValue`, which the rest of the engine treats as a cell pointer; touching it is the segfault at address 5 (the offset of the cell type byte). - **body-parser hack in StringDecoder**: Bun's `StringDecoder` accepts being called as a function on an arbitrary object and initializes that object in place (`RealStringDecoder.apply(this, ...)` from a `util.inherits` subclass). That is why the constructor looks at its receiver at all. The fix keeps that path and only stops scope objects from qualifying. - **Holder in the V8 shim**: a native data property's callbacks receive the receiver as `HolderV2()`. Bun installs such properties as JS accessors, so their getter and setter can also be extracted from the property descriptor and called like functions; that is the path `invokeAccessor` converts. <details> <summary>Release-build repros (bun 1.4.0-canary)</summary> ```js // exits 139; node prints "result: TypeError" let toString; try { require("node:fs").readFileSync(); } catch (e) { ({ toString } = e); } let out; try { out = toString(); } catch (e) { out = e.constructor.name; } console.log("result:", out); let name = "after"; function keep() { return [toString, name]; } keep(); ``` ```js // exits 139; node throws a TypeError const { isFile } = require("node:fs").statSync(__filename); console.log(isFile()); let mode = 0; function keep() { return [isFile, mode]; } keep(); ``` ```js const { StringDecoder } = require("node:string_decoder"); function keep() { return StringDecoder; } const d = StringDecoder("utf8"); console.log(Bun.inspect(d)); // [native code: JSLexicalEnvironment] console.log(d instanceof StringDecoder, typeof d.write); // false undefined ``` ```js // addon.c: napi_create_function("returnThis") returning this_arg from napi_get_cb_info const { returnThis } = require("./addon.node"); function keep() { return returnThis; } console.log(returnThis() === globalThis); // bun: false, node: true console.log(returnThis.call(undefined) === globalThis); // both: true ``` </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/napi/napi.test.ts test/v8/v8.test.ts <!-- robobun:evidence:end -->
1 participant
Fixes a fuzzer-found segfault (Fuzzilli fingerprint
07bc0d3cfacec7ff, SIGSEGV in baseline JIT code).Root cause
When a bare call
f()resolves its callee through a closure scope (i.e.fis a captured variable living in the activation), JSC passes the scope object (JSLexicalEnvironment) as the rawthisvalue and relies on the callee to sanitize it. JS functions do that viato_thisin their prologue; host functions see the raw value throughcallFrame->thisValue()and must callJSValue::toThisthemselves.Several Bun host functions returned or stored that raw value, leaking the activation object into user JavaScript:
The crash chain in the fuzzer input:
mockReturnThisreturned the activation, the script then read a property of it that corresponded to a not-yet-initializedlet(a TDZ slot).JSLexicalEnvironment::getOwnPropertySlotreturns the raw slot contents, so the read produced an emptyJSValueinside user JS. A JIT-compiledtypeofcheck on that value then dereferenced offset 5 of a null cell (empty passes the is-cell tag check) and segfaulted. In the ASAN build UBSAN reportsmember call on null pointer of type 'JSC::JSCell'.Fix
Apply
toThiswith strict semantics (identity for every value except scope objects, which becomeundefined, so behavior for all normal calls is unchanged) at every place a rawthisescaped:jsMockFunctionCall(the crashing one, line 844 of JSMockFunction.cpp): coversmock.contexts,mockReturnThis, and thethisforwarded to mock implementationsjest.setSystemTime(C++) and the Rust fake timer methods (useFakeTimers,useRealTimers,advanceTimersByTime, etc. inFakeTimers.rs), via a newJSC__JSValue__toThisStrictbinding exposed asJSValue::to_this_strictBun.pluginbuilderonLoad/onResolve/modulechaining returnsStringDecodercalled withoutnew: it wroteencodingonto the activation object and returned it; this also fixes a debug-build assert (asObjecton a non-cell) whenthisisundefinedThe dead C++
JSMock__jsUseRealTimersthat originally returned the rawthiswas removed onmainindependently while this PR was open; the live implementation is the Rust one inFakeTimers.rs, which is fixed here.This same root cause also explains a second Fuzzilli crash, fingerprint
284a2ab00e963a39(thejsMockFunctionCallhunk covers it), so no separate PR is needed for that one.Rebased four times while open. First onto the WebKit upgrade (#33133):
JSMock__jsSetSystemTimehad been rewritten onmain(theoverridenDateNow"no override" sentinel changed from-1toNaN), resolved by takingmain's logic and applyingtoThisto its return; plus an EOF test-append intest-timers.test.ts, resolved by keeping both. Second ontojest.resetAllMocks()(#33374) and runtimeonResolve(#33409): sources auto-merged, one EOF test-append conflict inplugins.test.ts, resolved by keeping all three tests. Third ontoadvanceTimersByTime/setSystemTime(#33623):JSMock__jsSetSystemTimereworked to a single exit routing throughBun__FakeTimers__setSystemTime, resolved by takingmain's body withtoThison that return;FakeTimers.rsauto-merged with all sevento_this_strictsites intact. Fourth onto the JSC C API removal (#33731),string_decoderlastTotalfix (#33703), and real-clock-under-fake-timers (#33896): sources auto-merged (theJSValue.rsreorg landed aroundto_this_strict, which survived; all sevenFakeTimers.rssites intact), one EOF test-append conflict intest-timers.test.ts, resolved by keeping both.Audited the remaining
thisValue()uses insrc/jsc/bindingsand host functions insrc/runtime: the rest are type-checked downcasts (a scope object fails the cast and throws) orhost_fn(method)shims that validatethisbefore the body runs. The NAPI and V8 shim layers pass the rawthisto native addons; that is a separate surface with its own compatibility questions and is not changed here.Tests
Regression tests in
mock-fn.test.js,test-timers.test.ts,plugins.test.ts, andstring-decoder.test.js. Each fails on bun without this change (USE_SYSTEM_BUN=1) and passes with it. The original fuzzer input now exits with an ordinary JS error instead of crashing.[review] gate passed · iteration 11 · 10 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 11
evidence per changed file