Repository navigation
Fix type confusion assigning onmessage/onerror through a Proxy of globalThis - #37053
Conversation
…balThis
The global onmessage/onerror accessors are CustomValue properties, so JSC
invokes their callbacks with the property receiver as thisValue. Assigning
through a receiver that is not the global object, e.g.
new Proxy(globalThis, {}).onmessage = null, passed a ProxyObject to an
unchecked downcast and segfaulted. Cast safely and fall back to the lexical
realm's global object, so the handler lands on the real global.
The 9000-iteration loop made 45,000 expect() calls, overrunning the 5s per-test budget on debug builds. Check the same predicate inline and assert once.
|
Warning Review limit reached
Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — narrow crash fix replacing uncheckedDowncast with dynamicDowncast + lexical-global fallback.
What was reviewed:
- Verified
dynamicDowncast<T>(JSValue)is an established pattern in bindings anddefaultGlobalObject()handles the non-Zig lexical-global case. - Confirmed these are the only two
putDirectCustomAccessor(..., 0)(CustomValue) slots on the global, so the whole bug class is covered. - Tests follow harness conventions (subprocess isolation,
test.concurrent, stderr→stdout→exitCode order); the randomUUID loop change preserves the assertion — still checks all 9000 iterations, oneexpect()at the end.
Extended reasoning...
Overview
Four uncheckedDowncast<Zig::GlobalObject>(JSValue::decode(thisValue)) calls in the onmessage/onerror custom getter/setter callbacks are replaced by a small helper that dynamicDowncasts the receiver and falls back to defaultGlobalObject(lexicalGlobalObject). Because these properties are installed as CustomValue (attributes 0), JSC's definePropertyOnReceiverSlow passes the receiver (e.g. a Proxy) as thisValue, so the unchecked cast type-confused a ProxyObject as a GlobalObject and dereferenced garbage through eventTarget(). Two subprocess tests (main-thread Proxy + with-scope, and worker Proxy of self) are added, and an unrelated slow loop in the same test file is trimmed to one expect().
Security risks
The prior code was a type-confusion segfault reachable from unprivileged JS — the fix strictly removes an unchecked cast in favor of a checked one. defaultGlobalObject already handles a non-Zig lexicalGlobalObject by returning the process default global, so the helper never returns null. No new attack surface.
Level of scrutiny
ZigGlobalObject.cpp is core, but the diff is ~10 lines: a static helper plus four one-line call-site swaps. The pattern (dynamicDowncast<T>(JSValue::decode(thisValue)) with a fallback) is used pervasively across the bindings (e.g. JSCommonJSModule.cpp), and the fallback helper is the existing defaultGlobalObject. I grepped for other putDirectCustomAccessor(..., CustomGetterSetter::create(...), 0) calls on the global — onmessage and onerror are the only two, so no sibling sites share the bug class.
Other factors
The behavioral choice — install the handler on the running realm's global rather than throw a TypeError — is a design decision, but it's documented as matching Deno and keeps the with (new Proxy(globalThis, {})) sandbox idiom working; either way it strictly beats the segfault. Tests use bunEnv/bunExe/tempDir, drain stdout/stderr/exited concurrently, assert stderr before stdout before exitCode, and run as test.concurrent. The crypto.randomUUID loop rewrite still visits all 9000 iterations and calls expect(malformed).toBeUndefined() once, so the assertion can still fail and reports the offending UUID.
|
@coderabbitai review |
|
|
CI status: 195 of 196 jobs passed. The one red lane (Debian 13 x64-asan) is test/js/node/worker_threads/worker-transfer-terminate-stress.test.ts failing a JSC ExceptionScope assertion (SIGABRT). That failure also occurs on main and is unrelated to this change; it has been flagged for separate triage. The new tests in test/js/web/web-globals.test.js passed on all lanes. Ready for review. |
2 participants
Assigning
onmessageoronerrorthrough a Proxy of the global object segfaults the process on 1.4.0:The same crash fires for
onerror, for the sloppy-mode sandbox idiomwith (new Proxy(globalThis, {})) { onmessage = function () {} }, and inside a worker (new Proxy(self, {}).onmessage = ...), where it takes down the whole process.Cause
onmessage/onerrorare installed on the global withputDirectCustomAccessor(..., 0), which makes them CustomValue properties. For a CustomValue slot, JSC's put-on-receiver path (JSObject::definePropertyOnReceiverSlow) invokes the custom setter with the receiver asthisValue. When the receiver is a ProxyObject rather than the global,uncheckedDowncast<Zig::GlobalObject>insetGlobalOnMessage/setGlobalOnError(ZigGlobalObject.cpp) type-confuses the Proxy and dereferences a boguseventTarget().Fix
Cast
thisValuewithdynamicDowncastin the four onmessage/onerror callbacks and fall back todefaultGlobalObject(lexicalGlobalObject)when the receiver is not a global. The assignment then installs the handler on the real global of the running realm, which matches Deno's behavior for this pattern and keeps thewith (proxy)sandbox idiom working. In a worker, the handler lands on the worker's own global scope.Verification
New tests in
test/js/web/web-globals.test.js:onmessage/onerrorthroughnew Proxy(globalThis, {}), plus thewithscope variantnew Proxy(self, {})echoes a message backBoth fail with the segfault on the released binary and pass with this change.
The second commit trims the pre-existing
crypto.randomUUIDfast-path loop in the same file from 45,000expect()calls to one assertion; it was overrunning the 5s per-test budget on debug builds.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/web/web-globals.test.js