Repository navigation
Conversation
…r's realm remoteFunctionCallGeneric wrapped the return value of a wrapped function for the target's realm. remoteFunctionCallForJSFunction and the JIT thunk's operationGetWrappedValueForCaller both wrap it for the caller's realm, which is what OrdinaryWrappedFunctionCall specifies. So a wrapped function whose target is not a plain JSFunction (a callable Proxy, or a built-in such as the realm's own Function) returned a wrapper whose [[Prototype]] is the target realm's Function.prototype. The caller reads .constructor off that prototype, gets the target realm's genuine Function constructor, and through it that realm's global object. The leak runs both ways: into a shadow realm and out of one. wrapReturnValue now takes one global object, the caller's realm, so the two cannot be swapped.
| for (const source of sources) { | ||
| const wrapped = new ShadowRealm().evaluate(source); | ||
| for (let i = 0; i < 200; i++) | ||
| shouldBe(wrapperComesFromThisRealm(wrapped), true); |
There was a problem hiding this comment.
🟡 nit (optional): The inner loop hardcodes 200 iterations instead of using testLoopCount, which JSTests/README.md (imported by JSTests/CLAUDE.md) says new tests are required to use so the harness can raise the count for tier-up-sensitive configurations and drop it for the rest. Fix: replace 200 with testLoopCount so eager-JIT configurations exercise the remoteFunctionCallGenerator thunk path and no-JIT configurations exit quickly.
Extended reasoning...
JSTests/CLAUDE.md @ README.md-imports JSTests/README.md, whose rule 2 states new tests must use testLoopCount (or wasmTestLoopCount) to control iteration counts because the jsc CLI sets it per configuration. The new test's loop at line 27 uses a literal 200. In ftl-eager/no-cjit configurations 200 may not reach the JIT thunk that also wraps return values, and in interpreter-only configurations it wastes time — either way it diverges from the mandated convention that every other stress test in the tree follows (e.g. stress/impure-get-own-property-slot-inline-cache.js:13).
Verification: nit: JSTests/CLAUDE.md line 1 @ README.md-imports JSTests/README.md, whose rule 2 (line 20) states new tests are required to "Use testLoopCount or wasmTestLoopCount to control how many iterations a test runs. The jsc CLI sets these based on the configuration of the test, so tests iterate enough to tier up where that matters and exit early where it doesn't." The global is real —…
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 5 included reviews per hour; 1 remains after this review. WalkthroughChangesShadowRealm return-value wrapping
Priority: ➖ Normal — Schedule the ShadowRealm runtime fix because cross-realm callable returns can expose the wrong realm’s prototypes and globals across multiple callable paths. Merge Risk: ⚪ Minimal · up to Returned callables now retain the invoking realm’s behavior across ShadowRealm boundaries, with coverage for relevant callable forms and directions. No current merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Warning Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use Comment |
Preview Builds
|
… target's return value for the caller's realm Pins WEBKIT_VERSION at the preview build of oven-sh/WebKit#590. A callable returned by a wrapped function whose target is not a plain JSFunction (a callable Proxy, or a built-in such as the realm's own Function) was wrapped for the target's realm instead of the caller's. The caller could read the other realm's Function constructor off the wrapper's prototype and reach that realm's global object, in both directions. The range from 2e2aa2290fac also contains oven-sh/WebKit#561, #566 and #568, already merged on oven-sh/WebKit main.
Problem
JSFunction, JSC wrapped it for the target's realm instead. The caller then holds a function object whose[[Prototype]]is the other realm'sFunction.prototype, so.constructoron it is that realm's genuineFunction, andF("return globalThis")()is that realm's global object.remoteFunctionCallGeneric(Source/JavaScriptCore/runtime/JSRemoteFunction.cpp:158), which ended withwrapReturnValue(globalObject, targetGlobalObject, result).remoteFunctionCallForJSFunction(line 122) and the JIT thunk'soperationGetWrappedValueForCaller(jit/JITOperations.cpp:189) both wrap for the caller's realm, which is whatOrdinaryWrappedFunctionCallspecifies.Proxy, aProxywith anapplytrap, and anyInternalFunction, for example the realm's ownFunction. The leak runs both ways, into a shadow realm and out of one.Fix
wrapReturnValuenow takes one global object, the caller's realm, and both call paths pass it. The pair cannot be passed the wrong way around any more.globalObjectis the caller's realm inside these host functions. The native thunk loads the global object from the callee's scope, and the callee is the wrapped function itself.operationGetWrappedValueForCallerreaches the same realm throughcallee->realm().wrapped-function-proto-from-caller-realm.js), so the generic path diverged unnoticed.JSTests/stress/shadow-realm-wrapped-function-return-value-realm.jscovers one target of each kind, plus both directions.Verification
autobuild-preview-pr-590-c7520f66): the new cases in bun'stest/js/bun/jsc/shadow.test.jsgo from 3 pass, 5 fail (pin2e2aa2290fac) to 8 pass, 0 fail. The repro above printstrueandundefined. bun PR: Bump WebKit (oven-sh/WebKit#590 preview): ShadowRealm wraps a returned callable for the caller's realm bun#42028.test-shadow-realm*.jsparallel tests that bun runs still pass, and a throwingProxytarget still surfaces as aTypeErrorfrom the caller's realm.Background
JSRemoteFunctionis the wrapper that a value gets when it crosses a realm boundary. Its structure and[[Prototype]]come from the global object thatJSRemoteFunction::tryCreatereceives, so that argument decides which realm owns the wrapper.VM::getRemoteFunction(isJSFunction)picks the call path when the wrapper is created. A plainJSFunctiontarget getsremoteFunctionCallForJSFunctionand theremoteFunctionCallGeneratorthunk. Every other callable getsremoteFunctionCallGeneric.JSFunction, sof.bind()targets take the fast path and were never affected.