Closed Bug 845862 Opened 7 years ago Closed 7 years ago

Transitive Xray waiving misses accessors in certain cases

Categories

(Core :: XPConnect, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(1 file, 2 obsolete files)

WaiveXrayWrapper::get{Own,}PropertyDescriptor only waives Xray on desc->value. If the result happens to be an accessor property, the transitive waiving chain will be broken, and we'll revert to Xray (this is safe, so it's a correctness problem rather than a security problem).

This generally isn't noticeable, because most property access ends up going through WaiveXrayWrapper::get, which will waive the result after the getter is invoked. But I'm adding some code that invokes GetPropertyDescriptor on a waived object, and we're breaking in that case.

I think the fix here is to wrap returned accessors in a function that invokes the accessor and then waives the result.  We could reify JSPropertyOps or something, but that's kind of a can of worms. I'm tempted to just not handle them.
Attachment #719129 - Flags: review?(mrbkap)
Yeah, wrapping in another function is definitely the right thing to do.  Agreed about reifying JSPropertyOps, too -- I tried it once, it was a mess.

If at some point we ever make it possible to have accessors directly backed by JSNative, I'd consider reifying, but not before.  Most of the trouble of JSPropertyOp is that it doesn't fit within the accessor mold required by ECMAScript.  But if the function pointer is equally functional as a JS function, there's no impedance mismatch, and it becomes much more plausible.  Still it'd be tricky to get right, and refiying a JSNative to a function could be hairy performance-wise.  It's definitely not something I'd want unless done after, or possibly concurrent with, JSPropertyOp removal.
Comment on attachment 719129 [details] [diff] [review]
Transitively apply waivers for accessor descriptors. v1

Given that mrbkap is overloaded and that Waldo has already looked at this bug, I think I can move the review over. ;-)
Attachment #719129 - Flags: review?(mrbkap) → review?(jwalden+bmo)
Comment on attachment 719129 [details] [diff] [review]
Transitively apply waivers for accessor descriptors. v1

Review of attachment 719129 [details] [diff] [review]:
-----------------------------------------------------------------

No good deed goes unpunished.  And have you *seen* my review queue?  I have to be at least as loaded as mrbkap is.  :-)  Probably shouldn't have looked in the first place.  :-)  :-(  But anyway.

The compartment bit here is the only bit that doesn't look good modulo nits.

::: js/xpconnect/wrappers/WaiveXrayWrapper.cpp
@@ +13,5 @@
>  #include "AccessCheck.h"
>  #include "WrapperFactory.h"
>  
> +using namespace JS;
> +using namespace js;

Why these?  I thought tradition was to not do this, and prefix everything, for clarity, or something.  Well, especially for js::.

@@ +48,5 @@
> +{
> +    // If there's no getter, we have nothing to do.
> +    if (!(desc->attrs & JSPROP_GETTER))
> +        return true;
> +    MOZ_ASSERT(desc->getter);

Add an "Xray around a property that has a setter but no getter?" to help out people who hit this.  (It's barely possible for this to happen, I think with Object.defineProperty(..., { get: undefined, set: undefined }) or somesuch.  Craziness, sure, but at least leave breadcrumbs for the fuzzers to follow.  :-) )

@@ +53,5 @@
> +
> +    // Create our wrapper.
> +    JSObject *baseFun = JS_FUNC_TO_DATA_PTR(JSObject *, desc->getter);
> +    JSFunction *outerFun =
> +      NewFunctionWithReserved(cx, CallAndWaive, 0, 0,

I'd rather see a js:: prefix on this, seeing as it's friend API and we want to get rid of it or figure out some better way to do it at some point.

@@ +57,5 @@
> +      NewFunctionWithReserved(cx, CallAndWaive, 0, 0,
> +                              JS_GetGlobalForObject(cx, baseFun), "CallAndWaive");
> +    if (!outerFun)
> +        return false;
> +    SetFunctionNativeReserved(JS_GetFunctionObject(outerFun), 0, ObjectValue(*baseFun));

Could you pull out this 0 into a |const uint32_t BASE_FUNCTION_SLOT = 0;| and use it in CallAndWaive as well?

It looks like |outerFun| could be from a different compartment, judging by "AndWrap" in the previous function called.  Don't you need some wrapping and/or compartment-entering to handle this?  (Both here and for desc->getter, looks like.)
Attachment #719129 - Flags: review?(jwalden+bmo) → review-
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #4)
> No good deed goes unpunished.  And have you *seen* my review queue?  I have
> to be at least as loaded as mrbkap is.  :-)  Probably shouldn't have looked
> in the first place.  :-)  :-(  But anyway.

Yeah, but I personally have like 30 patches in blake's review queue. ;-)
 
> ::: js/xpconnect/wrappers/WaiveXrayWrapper.cpp
> > +using namespace JS;
> > +using namespace js;
> 
> Why these?  I thought tradition was to not do this, and prefix everything,
> for clarity, or something.  Well, especially for js::.

Ok, I'll fix.


> Add an "Xray around a property that has a setter but no getter?" to help out
> people who hit this.  (It's barely possible for this to happen, I think with
> Object.defineProperty(..., { get: undefined, set: undefined }) or somesuch. 
> Craziness, sure, but at least leave breadcrumbs for the fuzzers to follow. 
> :-) )

If it can happen, I'll just check for it.


> Could you pull out this 0 into a |const uint32_t BASE_FUNCTION_SLOT = 0;|
> and use it in CallAndWaive as well?

Sure.

> It looks like |outerFun| could be from a different compartment, judging by
> "AndWrap" in the previous function called.  Don't you need some wrapping
> and/or compartment-entering to handle this?  (Both here and for
> desc->getter, looks like.)

We discussed this on IRC. The patch is ok.
Comment on attachment 719129 [details] [diff] [review]
Transitively apply waivers for accessor descriptors. v1

Review of attachment 719129 [details] [diff] [review]:
-----------------------------------------------------------------

Um, pretend I never said anything about the wrapping stuff, it's clearly wrong.  :-)  Nitfix and you're good here.
Attachment #719129 - Flags: review- → review+
Attachment #719129 - Attachment is obsolete: true
Attachment #719217 - Flags: review?(jwalden+bmo)
Attachment #719217 - Flags: review?(jwalden+bmo) → review+
The more recent try push for this was https://tbpl.mozilla.org/?tree=Try&rev=759cf5bbb6e0

This turned one webconsole test orange because that test was relying on some logic in WebConsoleUtils.jsm to determine whether a getter was "native" or not, and and wrapping all the getters in CallAndWaive made them all appear native, even the scripted ones.

This, in turn, made me realize that my previous solution was may more complicated than it needed to be. In particular, we don't need to wrap things in a callable at all - we just need to waive the getters and setters (if they exist), so that their proxy handler becomes WaiveXrayWrapper, and calling them goes through WaiveXrayWrapper::call, which waives the result.

Patch coming up.
Attachment #719217 - Attachment is obsolete: true
Attachment #721473 - Flags: review?(mrbkap)
Comment on attachment 721473 [details] [diff] [review]
Simpler Approach. v3

Yeah, this is clearly the way to go.
Attachment #721473 - Flags: review?(mrbkap) → review+
Just a stupid osx-only XUL test that should have been a chrome test. Fixed up and repushed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c6d2adc18ca
https://hg.mozilla.org/mozilla-central/rev/9c6d2adc18ca
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.