825392 - Create native-anonymous content reflectors in the XBL scope

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

this will let us get rid of SOWs.

Comment 1

[Bobby Holley (:bholley)]

Cody, are you still working on this? Just curious.

Comment 2

[Bobby Holley (:bholley)]

For posterity, here's what I wrote in a private email thread about what needs to be done here:

Here's what I think needs to be done (I'm CCing Boris so that he can correct me if I get anything wrong):

We have python code in dom/bindings that generates the JS/C++ glue for each concrete C++ class. You can look at the output of the code generation in your build directory (dom/bindings). See [1] for a detailed description of how all this stuff works.

For Nodes, we generally return the nsDocument* as the parent object (except for XUL, where we use the parent element). However, in this case, we're going to want to return the _wrapper_ for the nsDocument* in the XBL scope. Unfortunately, this can only be expressed as a JSObject*, which isn't something that can currently be returned by GetParentObject. So you'll need to add a GetParentObject overload that returns a JSObject* directly, and modify the WrapNativeParent stuff in BindingUtils.h to use this overload when it exists.

So we should just need to specialize WrapNativeParentHelper for JSObject*.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings

Comment 3

[Bobby Holley (:bholley)]

William is going to take a look at this.

Comment 4

[Cody Crews]

Ah actually you guys might want to hold up on this.   I have quite a bit of this done already and was waiting to go back over the results bholley and bz and figure out where to go with it but then the holidays hit and i haven't had a chance to get back to it.  I have the template specialization for JSObject's there and i was running into problems due to my lack of understanding of the GCrooting and sometimes compartment mismatches(due to the fact that bholley said we wanted the wrapper for the document in the XBL scope.)

Good call CC'ing me on this, and I can understand the point of open collaboration a lot more now because most of my code has been setting there done for well over 2 or 3 weeks.  Just let the holidays play out and ill be bringing some cleaned up versions to bholley and bz ok.

Comment 5

[Cody Crews]

Have to say my point there wasnt saying dont work on this, because what i have might need extensive work anyway, but at one point I had successfully came up with a solution that would build and run perfectly but threw assertions about dealing with styling elements directly instead of their JSObjects or something like that.  Sometimes I like the holidays and sometimes meh.

Comment 6

[Bobby Holley (:bholley)]

Oh, that's awesome!

William, how urgently are you blocked on this? Can it wait a few weeks while bz and I help cody get his patches landed?

Comment 7

[William Chen [:wchen]]

(In reply to Bobby Holley (:bholley) from comment #6)
> Oh, that's awesome!
> 
> William, how urgently are you blocked on this? Can it wait a few weeks while
> bz and I help cody get his patches landed?

It's not urgent, I'm glad to see there is already some work done for this bug. Thanks Cody!

Comment 8

[Cody Crews]

Should have part one of this on here tomorrow, its ready right now but im going to hope I can get lucky and have someone review it before uploading tomorrow. The first patch focuses on template specialization for JSObject*'s when dealing with parent objects, generating wrappers, etc.  I hope I can take care of the rest in part2 quick and leave it as basic(simple and clean, I'm messy apparently =/) yet functional as possible until you guys can take over.

Comment 9

[Cody Crews]

Attachment: specialization.patch

I'm not even putting this up for review or anything yet just putting it here so others can test it too this is the very basis for this work and I really need feedback.  The overall size of the firefox source alone is overwhelming and my inexperience in C++ makes the feedback even more crucial for this.  The idea here is to have everything ready to handle JSObject*'s being returned in places where they couldnt be handled and weren't expected before. As bz pointed out early on the actual work happens in WrapNativeParentHelper specialized for JSObject and everything else just helps to get us to that point.  Right now the JSObject* being passed back is the return value of a call to OwnerDocument()->GetWrapper() which works for now. The next part is to start focusing on exposing what needs to be exposed to code running as XBL which should lead up to eliminating the existing SystemOnlyWrapper implementation entirely.  I think follow up to clean up most wrapper handling code was being discussed by bholley and bz before and I definitely would rather have come in after that than before it.  Last thing is just a quick question, does the newest nightly not get properly tested before uploaded or does this happen with two seperate changes affecting each other but not interacting or causing problems separately. I wasted nearly 8 hours between yesterday and now thinking something was wrong before realizing that the clean source before my patch wasnt even building right at that time, but tested this with the newest nightly source just now and it was as smooth as it was before.  Sorry to put out so much information about such a small patch but I really do need the input back and I'm trying to be specific about my concerns at this level before trying to get back to tackling the next part.  The last thing to note is I can already say that the latest release source does not play nice with this patch, so don't waste your time and grab the newest nightly to test this.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Hmm.  So the reason you're specializing things other than WrapNativeParentHelper is that you want to pass a cx/obj to GetParentObject?  But you're not even using that cx/obj so far; do you plan to use it in the future?  I would think the cx/obj the operation is happening on is irrelevant here and that what really matters is the right compartment to wrap the document into, which I assume nodes would determine somehow, right Bobby?

If so, we could have them just determine it and then wrap the document into that compartment, or we could add a compartment member to the ParentObject struct (null by default or something) and handle wrapping into that compartment in BindingUtils...

Comment 11

[Boris Zbarsky [:bzbarsky]]

So I talked this over with Bobby.

We think the right thing to do is as follows:

1)  Add a boolean mUseXBLScope member to ParentObject.
2)  Set it to false in all the existing constructors.
3)  Add a new constructor (or a boolean arg to the templated constructor, with a default
    false value; that should be good enough here) to allow setting it to true.
4)  Add a GetUseXBLScope function in BindingUtils.h that takes ParentObject and returns
    the relevant member and also has a templated version that always returns false.  See
    how GetParentPointer works, for example.
5)  Add a boolean useXBLScope argument to the 4-argument overload of WrapNativeParent,
    possibly defaulting to false if we have manual calls to that overload, and pass it
    from the three-argument overload using the function added in step 4.
6)  In WrapNativeParent, after doing the WrapNativeParentHelper dance, if the boolean is
    set to true, use xpc::GetXBLScope on the object WrapNativeParentHelper::Wrap()
    returned, enter the compartment of that object, and JS_WrapObject into it before
    returning.  
7)  Hunt down all node subclasses with a GetParentObject (probably just Node, legend,
    generic html form elements, HTMLContentElement, and XULElement) and change them all
    to return ParentObject with the xbl bit set correctly.

or at least something along these lines.

Comment 12

[Bobby Holley (:bholley)]

Note that for (7), "accordingly" means "if the node is in a native anonymous subtree".

Comment 13

[Bobby Holley (:bholley)]

(Or actually, we can use the current ChromeOnlyAccess() that currently causes us to generate a SOW).

Comment 14

[Cody Crews]

The actual work that I scraped this together from is actually much further along than this and those arguments were used originally as a means to identify it as an obvious overload, but in the end I finally chose to just rename the function to GetParentObjectJSObject and test if it existed when hitting the GetParentObject template by using a mock of the existing HAS_MEMBER macro/SFINAE setup. The return value from this took the form of a second boolean argument to the template that I called AltParent, but when I was getting back to this after winding down from the holidays I realized that I was still having issues and I wasn't going to be getting anywhere with this without getting help beyond the specialization part. What I ended up with was a mess that was getting out of control and I knew that just breaking it back down to this level right here and getting it cleaned up and building would be the wiser move than wasting more time and energy on what I had.

I have to say thank you too for the very detailed feedback its exactly what I've been needing. My comment earlier about being messy was a reference to using the above setup just to move forward to test what I was thinking would be a much trickier problem up ahead. I should get up to speed much quicker with the holidays gone now. When I had ran so many different builds I couldn't keep up with them and was finally like meh I need to get some help/direction on this I was hesitant to try to get any feedback because I wasn't sure who I might bother/what people's schedules were.

Comment 15

[Cody Crews]

(In reply to Boris Zbarsky [:bz] from comment #11)
> 6)  In WrapNativeParent, after doing the WrapNativeParentHelper dance, if
> the boolean is
>     set to true, use xpc::GetXBLScope on the object
> WrapNativeParentHelper::Wrap()
>     returned, enter the compartment of that object, and JS_WrapObject into
> it before
>     returning.  

This is golden info, thank you so much for making that make sense.  I *believe* this stopped all the compartment mismatch crashes.  In the future I should be able to give some time even when my schedule gets bad to help update and further the documentation because it needs it.  On most things I fell like I can find more information grepping through the source and with mxr.mozilla.org than with any official documentation.

Also I plan to move to a full *nix box for a dev setup as soon as I'm satisfied with this.  I've been using the build tools for windows, and I think it can lead you into strange and dark alleyways sometimes but hey its microsoft so I should have expected that.

(In reply to Bobby Holley (:bholley) from comment #13)
> (Or actually, we can use the current ChromeOnlyAccess() that currently
> causes us to generate a SOW).

I already had my eye on this idea because of that recent round of XBL focused work, but I was considering changing it up so that it could be used something like the GetUseXBLScope function that was just added.

When I'm fairly sure that I've found all the subclasses that need my attention and have that ready Ill go back and remove all the unnecessary template parts and specialization but it was needed for me to explicitly execution flow for testing.  Thanks for the help guys, I'm getting the lay of the land now I believe ;-)

Comment 16

[Cody Crews]

I always skip words and all that, just ignore me I have ADD/ADHD and its hard sometimes when I'm in a hurry.  In all seriousness though thanks for your help and patience with me.  Expect the rest and a patch with it *maybe* tomorrow.

Comment 17

[Bobby Holley (:bholley)]

(moving the deps for bug 745392 and bug 825276 to the dependent bug)

Comment 18

[Cody Crews]

I thought before crashing for tonight that I should let it be known that there's now a new mismatch crash I'm hitting that's definitely not the same as the issues I was having before. If I can't get anywhere debugging it by early tomorrow evening I'll just get what I up on here and we can go from there.

I didn't have any time to even look at it today =/

Comment 19

[Cody Crews]

Attachment: AnonReflector-part1.patch

When I got the chance I was able to figure out that new compartment mismatch, and it probably wouldn't have been an issue if I was on the level already.  Here is the patch it implements 6 out of the 7 things listed above.  For the 7th I tried to get clever which probably wasn't the best choice.  Templates seem like they're against me by default, I keep trying to control them with logic.

The new crash was happening because the base document itself is always a node too, and might return true for ChromeOnlyAccess.  To factor this in I also compare for GetParentElement to return non-null.  With this I'm confused about what to do next, I left the specialization of WrapNativeParentHelper there, and the overload of GetParentObject(different argument) but they can easily be scrapped/reworked.  Ill be here for the rest of today and tomorrow probably, after that getting a response from me might take a while, not sure yet.  Point me in a direction and I'll get going on it.  I'm also itching to get back to security research, but that always happens.  Thanks bz and bholley, I've learned quite a bit already and I'm sure there's plenty more to come.

Comment 20

[Cody Crews]

I meant to mention this before, but when using DOM Inspector with this, I know that it lists some memory leaks specifically when looking at content marked ChromeOnlyAccess if I kill the build off leaving the DOM Inspector window but immediately closing it too.  This has to be due to my lack of understanding of the garbage collection setup at this point.  Also I could probably implement something using the HAS_MEMBER macro/SFINAE setup again to check for the JSObject* overload to GetParentObject.  I remember before renaming the function because of some issues but I can't remember the exact specifics.  In particular I didn't want to do this again because before I ended up with a mess, and you guys seem to have an idea already about what needs to go where.  The ChromeOnlyAccess check to determine wrapping could also be moved or reworked, just let me know.  

Until I hear something I'll be playing with the SFINAE setup to check for the overload to GetParentObject.  A quick question thinking about that is how to know when to call the overload that returns JSObject*, should it be called anytime it exists?  It seems that the ChromeOnlyAccess check should play a part in this, so might have to move that.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Wait.  Why do you need overloads that return GetParentObject or SFINAE?

If you're doing the 7 steps from comment 11, you shouldn't need any of that stuff.

Looking at the attached patch, you don't need the JSObject* specialization for WrapNativeParentHelper.  All you need to do is in WrapNativeParent _after_ you've called WrapNativeParentHelper<T>::Wrap to wrap that return value into the XBL compartment if needed.

You also don't need a GetParentObject that returns JSObject*.  You _do_ want one that returns a ParentObject (on all nodes that might need this sort of wrapping, and _that_'s where the ChromeOnlyAccess() checks should be happening, to set the boolean in the ParentObject correctly.

Comment 22

[Cody Crews]

Yeah I mentioned I tried to get clever for #7, I know that it has to be done I just thought being able to test this before changing a lot more code would be helpful.  Also I just tried gain headway while also not disturbing anyone's holidays originally.  I thought the logic implemented clearly showed the fact that ChromeAccessOnly would probably have to be moved.  I just wasn't assuming anything.  Beyond the nodes you mentioned before I already have a few others on my mind, the audio node variations for that API in particular.  Try to keep in mind that most of my C experience is from so long ago that ASLR and DEP weren't even  anywhere but in some of the *nix kernels.  Thanks for being so helpful, Ill try to be more assertive.

originally.(In reply to Bobby Holley (:bholley) from comment #2)
> For posterity, here's what I wrote in a private email thread about what
> needs to be done here:
> 
> Here's what I think needs to be done (I'm CCing Boris so that he can correct
> me if I get anything wrong):
> 
> We have python code in dom/bindings that generates the JS/C++ glue for each
> concrete C++ class. You can look at the output of the code generation in
> your build directory (dom/bindings). See [1] for a detailed description of
> how all this stuff works.
> 
> For Nodes, we generally return the nsDocument* as the parent object (except
> for XUL, where we use the parent element). However, in this case, we're
> going to want to return the _wrapper_ for the nsDocument* in the XBL scope.
> Unfortunately, this can only be expressed as a JSObject*, which isn't
> something that can currently be returned by GetParentObject. So you'll need
> to add a GetParentObject overload that returns a JSObject* directly, and
> modify the WrapNativeParent stuff in BindingUtils.h to use this overload
> when it exists.
> 
> So we should just need to specialize WrapNativeParentHelper for JSObject*.
> 
> [1] https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings

Originally I was told to explicitly add a GetParentObject overload that does just that, also how else can I be sure a member exists while dealing with templates without the HAS_MEMBER macro or just trying to grab a pointer to the member itself?  The original take was to call any such overload if it existed which seems to have been planned to take the place of the ChromeOnlyAccess check being used now.  Maybe that's why I was having such problems before ;-)

Comment 23

[Boris Zbarsky [:bzbarsky]]

> the audio node variations

AudioNode had nothing to do with Node.  You won't need to change AudioNode.

> Originally I was told to explicitly add a GetParentObject overload that does just that

Right, as of comment 11 the plan changed.

> how else can I be sure a member exists

Why do you need to check for a member existing?  The comment 11 plan doesn't require that.

Comment 24

[Cody Crews]

Yeah sorry about just getting back to you on this, I had already figured that out about AudioNode and I've now implemented #7 from the list before fully and on the last build I hit a crash related to GC.  Only had time to run that build once and didn't debug it yet, getting to it now.  Hopefully this evening I can get enough from the documentation and fix this and we *should* be done with this part.  I hope I haven't held anything up to much.

Comment 25

[Cody Crews]

It appears at first glance that my own debugging code was apparently holding a reference to something it shouldn't past GC, which makes this much easier.  Ill be cleaning up everything and continuing to build and test for a while yet, I'm still a bit skeptical to say the least.

Comment 26

[Cody Crews]

Attachment: AnonymousContentReflectors-v1.patch

I took the extra time to test a regular optimized non debug build because the debug build seemed to be running a bit more sluggish than usual.  Cleaning it up took a little longer than I thought it would too, but I made myself get up and get on it early this morning to get this up here.  Just see the comments for a few concerns I have, I think this *should* be about ready, so I wont toggle the review flag yet but I'm definitely tempted ;-)

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8361031 [details] [diff] [review]
AnonymousContentReflectors-v1.patch

Why do you need the UnwrapDOMObjectToISupports hackery in FragmentOrElement?   Why do you need any changes at all there?  You don't need to change anything about nodelists.  Same in nsContentList, and same in nsIHTMLCollection, and HTMLFormControlsCollection, and HTMLOptionsCollection etc.  What made you decide to change all these things that aren't nodes?

And even for nodes you shouldn't need all those complications with getting wrappers, I would think.   Just return ParentObject(whatever-the-old-return-value-was, ChromeOnlyAccess()).  In particular, what matters is the ChromeOnlyAccess() value of |this|, not of the native parent!

In WrapNativeParent<T>, why do you have separate calls to WrapNativeParentHelper<T>::Wrap on the two codepaths?  Make one call, store the result in a JSObject* on the stack.  Then if !useXBLScope just return the object, otherwise put it in a rooted and do the other stuff we need to do.

The rest looks good at first glance!

Comment 28

[Cody Crews]

(In reply to Boris Zbarsky [:bz] from comment #27)
> Comment on attachment 8361031 [details] [diff] [review]
> AnonymousContentReflectors-v1.patch
> 
> Why do you need the UnwrapDOMObjectToISupports hackery in FragmentOrElement?
> Why do you need any changes at all there?  You don't need to change anything
> about nodelists.  Same in nsContentList, and same in nsIHTMLCollection, and
> HTMLFormControlsCollection, and HTMLOptionsCollection etc.  What made you
> decide to change all these things that aren't nodes?

I was getting compilation errors that led me to begin looking into these additional changes.  The UnwrapDOMObjectToISupports is there because by default the mObject member of the ParentObject struct expects a pointer to nsISupports. I wasn't sure if it was possible to ever get a pointer that's base class wasn't derived from or directly nsISupports(is this even possible?) set there.  I was trying not to assume anything before.  Now I assumed at some point in the future that the mObject member might be referenced as a raw nsISupports pointer and then QI'ed into what the receiving end would it expect it to be.  If the call to QueryInterface failed then that could show a potential problem before it could become a potential security issue.

> 
> And even for nodes you shouldn't need all those complications with getting
> wrappers, I would think.   Just return
> ParentObject(whatever-the-old-return-value-was, ChromeOnlyAccess()).  In
> particular, what matters is the ChromeOnlyAccess() value of |this|, not of
> the native parent!

Again this was to be sure that that no tom foolery could take place.  About getting the native parent, how would it be possible for the parents ChromeOnlyAccess member function to return true and that not also apply to |this| ? If you're a child of a node that is anonymous/ChromeOnlyAccess then you by default are also anonymous and should have the ChromeOnlyAccess bit set which once set is never supposed to be changed.  I think the only place this could ever be an issue would be the element that triggers a binding or an chrome level/XUL binding that embeds a content level document.

> 
> In WrapNativeParent<T>, why do you have separate calls to
> WrapNativeParentHelper<T>::Wrap on the two codepaths?  Make one call, store
> the result in a JSObject* on the stack.  Then if !useXBLScope just return
> the object, otherwise put it in a rooted and do the other stuff we need to
> do.

This I admit makes more sense to me.  I've been trying to get this done on top of other things and my schedule gets very tight sometimes.  Thanks

I'll get rid of the separate codepaths in WrapNativeParent<T>, let me know about the rest because I think it makes sense as a very in depth preventative security measure.  I'm not sure what overall changes you guys are planning in the future but I assumed it was headed in this direction anyway and you just wanted this done to cover what mattered first.  Sorry if I was wrong about that.

Comment 29

[Boris Zbarsky [:bzbarsky]]

> because by default the mObject member of the ParentObject struct expects a pointer to
> nsISupports.

So what?  In either case:  1) Why are you changing anything about any non-node class?  2)  Why are you not just passing the existing parent to ParentObject?  Use ToSupports() if you're getting ambiguous inheritance from nsISupports (which I don't expect you are).

> how would it be possible for the parents ChromeOnlyAccess member function to return true
> and that not also apply to |this| ?

It's not.  But it's trivial to have the parent return false for ChromeOnlyAccess() while |this| returns true: take any |this| which is a native anonymous content root.  In this case you need to pass true to ParentObject, not false.

> because I think it makes sense as a very in depth preventative security measure. 

I don't think it does, sorry... ;)

Comment 30

[Cody Crews]

Ill have it fixed here shortly, and ill also add a testcase for something ive had my eye on showing that anytime you have an object in javascript that just has a QueryInterface function(generic nsISupports) all you have to do even though the components object has been removed is call a function from a matching object that has been QI'ed into like a window, then the generic nsISupports object will reveal itself as a window.  For instance I had a window that I could only see the QueryInterface function of and I call the alert function from my window on it, it gets *automagically* QI'ed to that window for you.  Its a bug with the newer bindings actually.

Comment 31

[Boris Zbarsky [:bzbarsky]]

> then the generic nsISupports object will reveal itself as a window

There is no such thing as "generic nsISupports" on the web, and the WebIDL bindings don't try to pretend such a fiction exists.  There is just the JS reflection of the object, period.

Comment 32

[Cody Crews]

Ill have to attach a detailed testcase to get my point across and its been some time since I did anything with it.  I understand that you know more about all of this than me, but I do believe that this is a serious issue.  Ill have to show you what I mean and it will make sense then.  It has something to do with the object being from a different compartment I believe, so when you obtain a reference to it you're only given an Object that appears to be a regular Object except that it has a QueryInterface function.  If you call its toString method its returns [xpconnect wrapped nsISupports].  Its actually a XPCWrappedJS Object implemented in JS usually, but months back it wasn't always.  Before the mozContact constructor was removed from the release builds it was even possible to pass in its prototype which was a COW as the prototype for a generated XPCWrappedJS Object, and then through playing with and remapping other things to have that prototype you could watch this JS generated Object transform into many different xpconnect wrapped nsI* objects. The toString method would continue to list them with commas, but it would only retain the actual functionality of say the last three.  I can demonstrate it using a couple of techniques but with simple things like an nsIDOMFileList right now.  Ill dig into it really deep again and see how far I can go but first, back to this patch!  I had simply done as you said above and passed the known parent and called the ChromeOnlyAccess method of |this| at one point but that felt wrong without at least checking to see if the parent was essentially just a null pointer.  Thinking about that and the issues above sidetracked me I admit, but its easy to go back.

Comment 33

[Boris Zbarsky [:bzbarsky]]

You're talking about double-wrapped natives.  That can't happen with WebIDL objects, I believe.

Comment 34

[Cody Crews]

That might be the case, I've heard of the double wrapping issues and was planning on looking into them in relation to that issue but I just didn't get to it.  I'll be waiting to actually see this build finish and looking into it some right now.  I just made a huge mess trying to reverse some patches and had to just go at it from a clean source without the unnecessary changes this time.  Ill be putting together something to show it off in at least a simple way shortly, the most it ever amounted to before even before all the changes was some interesting memory corruption crashes.  I have one of those(unrelated) to report that I found earlier actually, but its no real hurry didn't look easily exploitable if at all and it involves some code I could never see being used in any production environment.

Comment 35

[Cody Crews]

(In reply to Boris Zbarsky [:bz] from comment #11) 
> 7)  Hunt down all node subclasses with a GetParentObject (probably just
> Node, legend,
>     generic html form elements, HTMLContentElement, and XULElement) and
> change them all
>     to return ParentObject with the xbl bit set correctly.
> 
> or at least something along these lines.

I just saw it when I walked backed through this patch/build manually, HTMLContentElement is what led me astray originally, there is no GetParentObject in HTMLContentElement.h/cpp except a virtual function for the class DistributedContentList, its base class is nsINodeList.  Once I touched anything there everything complained until I did the follow work on all of it.  Ill add the new patch as an attachment right after a quick reboot, my desktop is completely cluttered from all of this.

Comment 36

[Cody Crews]

Attachment: AnonymousContentReflectors-v2.patch

Well, lets try this again.  Currently building this patch against a completely clean source, will mark obsolete and update if anything goes wrong.

Comment 37

[Cody Crews]

With the previous patch the build completed fine, but now hits a new compartment mismatch,  The only real difference here is that I removed the unnecessary things like nsINodeList, and started passing the parent directly to ParentObject instead of the setup I had before.  I'm quite tired and I probably messed something simple up.  I'm going to take a break and if that helps me get some energy back up Ill take one more go at this tonight maybe.  Just to let you know bz, I will get a testcase up showing that even without all the complicated stuff needed before You can still see evidence that QI'ing to something you dont even have access to the CID or JSID of is possible, not to mention when you factor in the XPCWrappedJS object angle.  It just might be tomorrow but Ill CC you on it when I do (will probably be marked private at first until you can look it over for yourself.)

Comment 38

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8361313 [details] [diff] [review]
AnonymousContentReflectors-v2.patch

You shouldn't need the static_cast<nsINode*> bit in legend.  Element already singly-inherits from all the things we care about.  The static_cast was there before just because the "a ? b : c" construct warns if b and c are different types, but you're not using that construct anymore.

Similar in nsGenericHTMLFormElement::GetParentObject.

The rest looks pretty good, but:

1)  There are lots of spacing issues.
2)  In WrapNativeParent, you need to put the nativeParent into a Rooted before
    calling GetXBLScope, since that call can GC.  
3)  The comments about falling through in WrapNativeParent no longer make sense.

Comment 39

[Boris Zbarsky [:bzbarsky]]

> but now hits a new compartment mismatch

Where?

Comment 40

[Cody Crews]

(In reply to Boris Zbarsky [:bz] from comment #38)
> Comment on attachment 8361313 [details] [diff] [review]
> AnonymousContentReflectors-v2.patch
> 
> You shouldn't need the static_cast<nsINode*> bit in legend.  Element already
> singly-inherits from all the things we care about.  The static_cast was
> there before just because the "a ? b : c" construct warns if b and c are
> different types, but you're not using that construct anymore.
> 
> Similar in nsGenericHTMLFormElement::GetParentObject.
> 
> The rest looks pretty good, but:
> 
> 1)  There are lots of spacing issues.
> 2)  In WrapNativeParent, you need to put the nativeParent into a Rooted
> before
>     calling GetXBLScope, since that call can GC.  
> 3)  The comments about falling through in WrapNativeParent no longer make
> sense.

Yeah I realized after uploading that some of the comments didn't make sense and the spacing issues, I even left a comment that referenced a totally non existent file In there.  I was planning on mentioning all of that before continuing but when I hit the compartment mismatch I decided it was time to step back and relax and look at it again fresh in a while.  Ill get to it again in just a second.  I tend to get obsessive about things and when I do my focus and productivity start to fall.

I can tell you where if I can get it actually not act like its updated(tries to check my addons for compatibility) because when it does this it doesnt give me the option to debug it on the fly as it crashes and actually thinks that microsoft has an idea about what is causing it.  Ill fire up another copy of nightly with that profile to bypass it real quick, just had to get my new DSL to keep sync to even be able to get this response typed out.  I do remember it had to with XBLBindingImplentation and something else I hadn't seen in any other mismatch crash yet.  Give me just a minute, and hope that my connection stays up.

Comment 41

[Cody Crews]

Attachment: callstack.txt

Heres a quick stack trace, barely got this in had just dropped again =/

Comment 42

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8361421 [details]
callstack.txt

The XBL code here seems to assume that "global" and "obj" are in the same compartment.  But ghe way it got "obj" is by doing:

  nsContentUtils::WrapNative(cx, global, aBoundElement, &v);

and nsContentUtils::WrapNative defaults to aAllowWrapping = false.  So after this &v.toObject() and global may not be same-compartment....

Bobby, do we want to be doing XBL binding init in the compartment of global or of obj?

Comment 43

[Bobby Holley (:bholley)]

This is exactly what Andrew is looking at in bug 956404 comment 25. Let's continue the discussion there.

Comment 44

[Cody Crews]

(In reply to Boris Zbarsky [:bz] from comment #42)
> Comment on attachment 8361421 [details]
> callstack.txt
> 
> The XBL code here seems to assume that "global" and "obj" are in the same
> compartment.  But ghe way it got "obj" is by doing:
> 
>   nsContentUtils::WrapNative(cx, global, aBoundElement, &v);
> 
> and nsContentUtils::WrapNative defaults to aAllowWrapping = false.  So after
> this &v.toObject() and global may not be same-compartment....
> 
> Bobby, do we want to be doing XBL binding init in the compartment of global
> or of obj?

I'm curious as to why this started all of a sudden on that most recent build.  When I said a clean source tree I didn't actually download a new copy of the source, I keep a clean copy that hasn't even had configure ran on it to make copies of.  My point here is that I wasn't hitting this crash with v1 of this patch up above so it's kind of strange.

Comment 45

[Cody Crews]

Just a quick thought, but maybe the unnecessary unwrapping that I was doing was somehow avoiding this.

Comment 46

[Bobby Holley (:bholley)]

OK, so:

The basic issue here is that the setup of the XBL prototype machinery expects the node's reflector to be same-compartment with that of its document. This is obviously not going to work.

So you'll need to change nsXBLProtoImpl::InitTargetObjects, and pull the global off of the element's reflector, rather than the document.

So something like:
JS::Rooted<JSObject*> global(cx, js::GetGlobalForObjectCrossCompartment(aBoundElement->GetWrapper()));

And probably enter its compartment while you're at it:

JSAutoCompartment ac(cx, global);

You'll also need to line 83:

81   JS::Rooted<JSObject*> globalObject(cx,
82     GetGlobalForObjectCrossCompartment(targetClassObject));
83   JS::Rooted<JSObject*> scopeObject(cx, xpc::GetXBLScope(cx, globalObject));

should be come:

83   JS::Rooted<JSObject*> scopeObject(cx, xpc::IsXBLScope(globalObject) ? globalObject : xpc::GetXBLScope(cx, globalObject));

Since GetXBLScope doesn't expect you to pass it an object that is already in an XBL scope.

Comment 47

[Cody Crews]

Ok, Ill give this a go in about an hour or so, should have time to get in at least a couple builds tonight.  Will let you know how things go but I think that shouldn't be to hard to take care of.

Comment 48

[Cody Crews]

This is an error generated during a build, so I should use ToSupports as bz said before?  I think certain parts of this has felt like running in circles after my own tail.

c:\mozilla-central-patchgen-test4\obj-i686-pc-mingw32\dist\include\mozilla/dom/B
indingDeclarations.h(485) : error C2594: 'initializing' : ambiguous conversions
from 'mozilla::dom::HTMLFormElement *' to 'nsISupports *const '
        c:\mozilla-central-patchgen-test4\content\html\content\src\nsGenericHTML
Element.cpp(2049) : see reference to function template instantiation 'mozilla::d
om::ParentObject::ParentObject<mozilla::dom::HTMLFormElement>(T *,bool)' being c
ompiled
        with
        [
            T=mozilla::dom::HTMLFormElement
        ]

Comment 49

[Andrew McCreight [:mccr8]]

> so I should use ToSupports as bz said before
Yes, that sounds right.

Comment 50

[Cody Crews]

I think that took care of it, now we will see after it finishes about the compartment mismatch.

Comment 51

[Cody Crews]

Attachment: callstack2.txt

Ok so now we can get to the meat.  xpc::IsXBLScope expects a JSCompartment* so I used xpc::IsXBLScope(js::GetObjectCompartment(globalObject)) in place of xpc::IsXBLScope(globalObject) above.  Here is a stack trace of the next crash.

Comment 52

[Cody Crews]

Ill be getting up early tomorrow and trying to get a jump on this.  I'm calling it a night as of right now.

Comment 53

[Cody Crews]

Needless to say I fell behind on my intended schedule, will be looking this over some today but I wont have a lot of free time.  I may look at alternate routes, but probably shouldn't as all this is still over my head.  I'm going to have to make a choice to make this a full time focus soon instead of giving it my spare time from other things.

Comment 54

[Cody Crews]

I can see from the stack trace above that the call to js::GetGlobalForObjectCrossCompartment is being handed a null pointer, and thats because aBoundElement doesn't yet have a wrapper for the call to GetWrapper to return.  What is the preferred way to generate a wrapper, or should I bypass it and try to get see if mParent, or its parent element has a wrapper that GetWrapper will return?  I would think since its going to get the global object associated with the returned value that this would suffice.  Hmm, im not really sure though.

Comment 55

[Bobby Holley (:bholley)]

(In reply to codyc from comment #54)
> I can see from the stack trace above that the call to
> js::GetGlobalForObjectCrossCompartment is being handed a null pointer, and
> thats because aBoundElement doesn't yet have a wrapper for the call to
> GetWrapper to return.

Ah, right. Sorry, you should call WrapNewBindingObject() on aBoundElement, since that'll generate the reflector if it isn't there already.

> should I bypass it and try to get see if mParent, or its parent element has
> a wrapper that GetWrapper will return?  I would think since its going to get
> the global object associated with the returned value that this would
> suffice.  Hmm, im not really sure though.

No, this wouldn't work, because the parent's wrapper might be in the document's scope (think about the case where aBoundElement is the root of a NAC subtree).

Comment 56

[Bobby Holley (:bholley)]

bz points out that you'll need to follow your call to WrapNewBindingObject() with js::UncheckedUnwrap(), since it'll apply security wrappers, and you want the canonical scope here.

Comment 57

[Cody Crews]

I had left this hanging. I know that in a few hours Ill have at least 4 or 5 hours to work on this, and I'm going to really dig into getting this fixed.

Comment 58

[Cody Crews]

I believe I may have handled the compartment mismatch originating from nsXBLProtoImpl.cpp, the next crash out had to do with my own modified code in BindingUtils.h.  I'm running a build real quick to get some more information then I'll definitely just throw up a stack trace or something.

Comment 59

[Cody Crews]

Attachment: stacktrace.txt

Here is a stack trace, this is after saving nativeParent on the stack and reworking it to only one codepath, so I cant understand how nativeParent which should be a valid pointer is ending up as a nullptr.  

Just before this crash is triggered where the old compartment mismatch would have been triggered it goes past that point to output these three lines in the debug output:

JavaScript error: , line 0: out of memory
JavaScript error: , line 0: out of memory
JavaScript error: , line 0: out of memory

I've played enough with objects out of scope(mainly their constructors when holding eval references etc) to know that this means I've probably messed up something when fixing that mismatch crash, but this still doesn't explain the nullptr being passed along later on.

Comment 60

[Cody Crews]

It looks like I've trapped myself here, the call to WrapNewBindingObject ends up calling into the modified WrapNativeParent, I tried once just for fun to skip WrapNewBindingObject and pretty much just wait until aBoundElement->GetWrapper() wasnt null, this treated me to nice big blank white screen.  I might have to specialize WrapNativeParent for nsISupports but that doesnt seem right.  What happens here is WrapNativeParentHelper falls through to WrapNativeParentFallback which in certain circumstances just returns a nullptr.  Hmm, tricky is all I have to say.

Comment 61

[Boris Zbarsky [:bzbarsky]]

> the call to WrapNewBindingObject ends up calling into the modified WrapNativeParent

Yes, that's correct.  Why is this a problem?

Comment 62

[Boris Zbarsky [:bzbarsky]]

Oh, also, you really need to root nativeParent before the GetXBLScope call, I'd think.

Comment 63

[Cody Crews]

Its already been modified to be rooted, and its a problem because I need WrapNewBindingObject to return so that aBoundElement actually has a wrapper to return.

Comment 64

[Cody Crews]

// Wrapping of our native parent.
template<typename T>
static inline JSObject*
WrapNativeParent(JSContext* cx, JS::Handle<JSObject*> scope, T* p,
                 nsWrapperCache* cache, bool useXBLScope = false)
{
  if (!p) {
    return scope;
  }
  
  JS::Rooted<JSObject*> nativeParent(cx, WrapNativeParentHelper<T>::Wrap(cx, scope, p, cache));
  if (!useXBLScope) {
    return nativeParent;
  }
  // If useXBLScope is true here then we should attempt to enter the XBL
  // comparentment and wrap nativeParent for that compartment before
  // returning it.
  //JS::Rooted<JSObject*> wrappedNativeParent(cx, nativeParent);
  JS::Rooted<JSObject*> XBLScopeObject(cx, xpc::GetXBLScope(cx, nativeParent));
  JSAutoCompartment ac(cx, XBLScopeObject);
  if (JS_WrapObject(cx, &nativeParent)) {
	return nativeParent;
  }
  return nullptr;		  
}

thats my current modifications.

Comment 65

[Boris Zbarsky [:bzbarsky]]

That looks reasonably sane.  What's failing?

Comment 66

[Cody Crews]

Take a look at the stack trace above, it ends up calling WrapNativeParent<nsISupports> which WrapNativeParentHelper then allows to fall through to WrapNativeParentFallback, which is templated to just return a nullptr if it doesnt get the input its expecting.  This causes GetXBLScope to be called on a nullptr, and and then JSAutoComparment to enter the compartment of a null pointer.  Once its at that point its setup to fail definitely.

Comment 67

[Cody Crews]

I might be my return that its hitting actually, either way the call to WrapNewBindingObject never returns and that makes it unable to get anything useful from aBoundElement.  I'll look it over some more after lunch, I'm actually EST so yeah.

Comment 68

[Boris Zbarsky [:bzbarsky]]

> This causes GetXBLScope to be called on a nullptr

Ah.  Well, if WrapNativeParentHelper<T>::Wrap returned null you better propagate out that null!

Comment 69

[Cody Crews]

I've been meaning to let you all know the status of this.  Essentially I hit a wall and was trying to think about the best approach from there.  Then the official announcement of the pwn2own and chrominum events made my focus shift.  I'll be cleaning up what I have and trying to get it up on here through this coming weekend.  To get by that compartment mismatch I was entering the appropriate compartment and had only tested if firefox would fire up after slightly modifying the logic involved with 2 assertions after the code should have finished this bug and finalized the changes we're aiming for with it.

With the modified logic in those assertions ff runs fine and everything is functional except things like firebug which use the jetpack API.  I haven't tested any other jetpack based extensions because before the modified logic in those assertions everything included with firefox like the web and browser console would hit one of those two assertions, but after the modifications which were extremely light that all stopped.  This led me to a new issue in firebug.

When using firebug it's also functional but for some reason when using the scroll bars or even doing anything that issues a mouseover event on them firebug would cause ff to crash because mIsXBLScope isnt true.  Since this crash happens from firebug it just crashes out without even the crash reported or any JIT debugging chances.  Ill be striping all my changes to nsXBLProtoImpl and related code except the part that stops the compartment mismatch and let you guys work it out from there.  bholly you were definitely right, still a little over my head to say the least and definitely a headache too.  Thanks for the opportunity and I did learn quite plenty from it so it definitely wasn't a waste.  I have all the ParentObject stuff in place and cleaned up to at least acceptable IMO.  There was probably going to be more issues involved and I didn't feel entirely comfortable with the logic changes in those assertions no matter how light/safe(seemingly to me) they were.  I was happy to see it all come together and most everything functional except the jetpack API based stuff which was something.  I just don't want to leave you guys having to come in behind me and clean code that might have been leading to a mess anyway.

I'm hoping my work focusing mainly on hitting the memory corruption side of this field for the last few weeks and renewed interest in getting my assembly understanding up to par will be just the push I've been needing to make it all come together over the coming months.  This should allow me to actually contribute more.  I have some experience with assembly(used to do a little cracking/hooking many many moons ago) and I can recognize the function prologue and cleanup when dealing with the cdecl function calling convention at least.  There's some issues relating to security I'll be bringing up soon too when I get a chance to go back and follow up on them more.  My current focus relates to a new update in the game Global Offensive which seems to have introduced what looks like at first glance a use after free and not only that it crashes at a jmp instruction referencing a dword ptr of a value derived from the value stored in a register+0x0C and that value definitely isn't always null.  It's the most obviously exploitable(looking at least) memory corruption issue I have just stumbled onto in some time.  I'm trying not to put a lot of time into it at this point but it could leave millions of steam users at risk right now.

I'll definitely take something a little easier like was suggested next time I need slight headache ;-)

I also might end in touch at some point soon to discuss that opportunity from many months ago, I've seen that internship program is posted publicly as of recent months and I'm honestly starting to think it might be the most efficient way to get my skills up to par and maybe be more collaborative with you all which would obviously be more helpful.

I'll be putting the cleaned up patch on before Monday even if it almost kills me, then back to those events for a minute.

Comment 70

[Bobby Holley (:bholley)]

Hey Cody,

Thanks for all your hard work on this bug! If you post your patch, I'll see if I can find some time soon to push it over the line.

Comment 71

[Bobby Holley (:bholley)]

I have patches that appear to work:

https://tbpl.mozilla.org/?tree=Try&rev=987ca899e882

Comment 72

[Bobby Holley (:bholley)]

Attachment: Part 1 - Fix up a test to use transitive wrapping rather than getPrivilegedProps. v1

Comment 73

[Bobby Holley (:bholley)]

Attachment: Part 2 - Parent Touch objects to the EventTarget's global. v1

Comment 74

[Bobby Holley (:bholley)]

Attachment: Part 3 - Infer the global from the reflector in DoInitJSClass. v1

Comment 75

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8392906 [details] [diff] [review]
Part 1 - Fix up a test to use transitive wrapping rather than getPrivilegedProps. v1

r=me

Comment 76

[Bobby Holley (:bholley)]

Attachment: Part 4 - Add xpc::GetXBLScopeOrGlobal and Sprinkle some calls to it where necessary. v1

Comment 77

[Bobby Holley (:bholley)]

Attachment: Part 5 - Generate anonymous content reflectors in the XBL scope. v1

Comment 78

[Bobby Holley (:bholley)]

Attachment: Part 6 - Remove SOWs. v1

Comment 79

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8392907 [details] [diff] [review]
Part 2 - Parent Touch objects to the EventTarget's global. v1

r=me, but it's a bit sad we have no way to get the inner for the target directly!

Comment 80

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8392908 [details] [diff] [review]
Part 3 - Infer the global from the reflector in DoInitJSClass. v1

r=me

Comment 81

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8392910 [details] [diff] [review]
Part 4 - Add xpc::GetXBLScopeOrGlobal and Sprinkle some calls to it where necessary. v1

r=me

Comment 82

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8392911 [details] [diff] [review]
Part 5 - Generate anonymous content reflectors in the XBL scope. v1

>+    if (ChromeOnlyAccess()) {
>+      p.mUseXBLScope = true;
>+    }

How about just:

  p.mUseXBLScope = ChromeOnlyAccess();

?

>+++ b/content/html/content/src/HTMLLegendElement.h

All the stuff in this file is in the mozilla::dom namespace, so no need to fully qualify ParentObject, right?

r=me with those nits.

Comment 83

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8392913 [details] [diff] [review]
Part 6 - Remove SOWs. v1

Please change WRAPPER_CACHE_FLAGS_BITS_USED to 2.

>GetSameCompartmentWrapperForDOMBinding(JSObject*& obj)

The right thing for this to return, unless you're removing all its callers, is IsDOMObject(obj).  Otherwise, you'll deoptimize things like MaybeWrapObjectValue and company to call JS_WrapValue() all the time.

>@@ -723,23 +665,16 @@ WrapNewBindingObject(JSContext* cx, JS::Handle<JSObject*> scope, T* value,

The change here is wrong.  The point was to avoid calling JS_WrapValue in the same-compartment case, as an optimization.  So you want to keep the compartment check, but replace WrapNewBindingForSameCompartment() with just rval.set(JS::ObjectValue(*obj)).

r=me with those bits fixed.

Comment 84

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=56cbea7d6997

Comment 85

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=5c9a3832124a

Comment 86

[Bobby Holley (:bholley)]

forgot to qref:

https://tbpl.mozilla.org/?tree=Try&rev=9e33dee044bf

Comment 87

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2e93a6862774
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8d63685f59f0
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7462463054e3
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3626a613190e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7c1d075bb7f6
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/dcdf23cc6232

Comment 88

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2e93a6862774
https://hg.mozilla.org/mozilla-central/rev/8d63685f59f0
https://hg.mozilla.org/mozilla-central/rev/7462463054e3
https://hg.mozilla.org/mozilla-central/rev/3626a613190e
https://hg.mozilla.org/mozilla-central/rev/7c1d075bb7f6
https://hg.mozilla.org/mozilla-central/rev/dcdf23cc6232

Comment 90

[Cody Crews]

Bobby, bz I'm sorry that you guys had to come in here and run cleanup on this.  As I've been taking it easy lately I've been putting more time into reading extensively on C++ and even asm beyond that.  I've always had a problem being too obsessive about things and I remember the last thing I said about getting my patch which probably still needed extensive work was along the lines of "even if it kills me".

I kind of have to laugh at the irony of that now and stay focused more on not getting obsessed.  I won't try to tackle an elephant again for a while, that's for sure.

Comment 91

[Bobby Holley (:bholley)]

No worries Cody! Thanks for your hard work on this (and everything else) regardless :-)