Closed Bug 1034262 Opened 7 years ago Closed 7 years ago
"apply" broken between same-origin compartments with asymmetric want
4.76 KB, patch
|Details | Diff | Splinter Review|
2.01 KB, patch
|Details | Diff | Splinter Review|
The following, run from a Chrome scratchpad: w = content; var f = Cu.evalInSandbox('x => typeof x', Cu.Sandbox(w)); w.wrappedJSObject.f = f; w.eval('[f.apply(null, [alert]), f.call(null, alert), f(alert)]') returns ["undefined", function", "function"], which is surprising: `f.call(a, b)` and `f.apply(a, [b])` are expected to behave the same. I guess the issue is that "alert" is a function-valued property (with name "0") on the argument passed to "apply", and thus is Xray-ed away and never reaches f. (Related to bug 1032457 and bug 1031336. We hit this in Firebug, see https://code.google.com/p/fbug/issues/detail?id=7580.)
The basic problem in the testcase is that one compartment requests same-origin Xrays via wantXrays=true (the default for Sandboxes) while the other does not. The current code only considers the wantXrays flag of the compartment performing the access, so we end up in a situation where we have same-origin compartments, but Xray in one direction and Transparent in the other. This is a problem for crossCompartmentFunction.apply(null, [arg]). If both globals get transparent wrappers, there's obviously no problem. And if both globals get XrayWrappers, then the |apply| happens on the XrayWrapper of the function in the caller's compartment. So the Array is unpacked in the caller's compartment, and again we have no problem. But if the caller gets Transparent and the callee gets Xrays, then we end up invoking |apply| from the callee's side, which then gets an XrayWrapper to the array. This XrayWrapper may do surprising things, leading to the odd situation in the testcase. Same-origin Xrays are kind of broken anyway, but I don't think we'll ever be able to get rid of them. So the most sensible thing to do is probably to honor the flag (if set) from either compartment. This patch does that.
Attachment #8450695 - Flags: review?(gkrizsanits)
Summary: "apply" on exported functions should not convert functions to undefined → "apply" broken between same-origin compartments with asymmetric wantXrays
In Jetpack in case of a panel is part of the add-on, so we trust it, we create the related content-script with same origin principal (instead of nsEP) and inject the sandbox (with the name 'addon') to its scope. With stuff like postMessage on it for enabling direct communication etc. I guess now you will have to waive xray before the injection to fix the test. (I _think_ here: http://mxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/content/sandbox.js#201)
Comment on attachment 8450695 [details] [diff] [review] Honor the wantXrays of both sides of the membrane when computing same-origin wrappers. v1 Review of attachment 8450695 [details] [diff] [review]: ----------------------------------------------------------------- I cancel this review for now until you figure something for the SDK issue, since I don't think we can waive xrays from content side... :(
Attachment #8450695 - Flags: review?(gkrizsanits) → review-
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #3) > In Jetpack in case of a panel is part of the add-on, so we trust it, we > create the related content-script with same origin principal (instead of > nsEP) and inject the sandbox (with the name 'addon') to its scope. It seems like we shouldn't be using wantXrays at all in that case, right? Try push with that change: https://tbpl.mozilla.org/?tree=Try&rev=ca23b75756d3
Comment on attachment 8452114 [details] [diff] [review] Don't use wantXrays for requiresAddonGlobal sandboxes. v1 Review of attachment 8452114 [details] [diff] [review]: ----------------------------------------------------------------- I was thinking that we might not need a sandbox at all in this case, but probably this is the simplest and cleanest solution.
Attachment #8452114 - Flags: review?(gkrizsanits) → review+
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Commit pushed to master at https://github.com/mozilla/addon-sdk https://github.com/mozilla/addon-sdk/commit/b54c63d3a4c3219cb9150cc6c3c2838655e9410d Bug 1034262 - Don't use wantXrays for requiresAddonGlobal sandboxes. r=gabor
You need to log in before you can comment on or make changes to this bug.