1406570 - Rooting hazards revealed by improved constructor handling

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Steve Fink [:sfink] [:s:]]

I'm working on a sixgill toolchain build, which has the side effect of applying a couple of fixes to sixgill that I apparently never released. And which fix a break in the callgraph, resulting in a rooting hazard.

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Root writeArrayBuffer

Trivial fix

Comment 2

[Steve Fink [:sfink] [:s:]]

It seems like a pain to file a bug and separately request approval for each of the things uncovered by updating sixgill, so I'm going to try putting them all here for now.

I either need to land these before switching over to the sixgill toolchain task, since it's building a newer sixgill, or figure out what version I was using before so I can disconnect these landings.

Comment 3

[Steve Fink [:sfink] [:s:]]

Attachment: Resolve rooting hazard in CustomElementRegistry by reordering initialization

This is *probably* a false positive, but I'm not confident. The hazard analysis believes that the mConstructor CustomElementConstructor can GC, for somewhat questionable reasons. I annotated away a few, but it came up with more, and the fix to avoid the problem is easy -- copy the aPrototype JSObject* into mPrototype first, before calling the CustomElementConstructor ctor. Then aPrototype will be dead, and mPrototype will have the post barrier run on it.

jonco -- except... why does this work? It will protect it from the generational collection's pointer move. But what if CustomElementConstructor::CustomElementConstructor triggered a compacting GC? In CustomElementDefinition constructor, the Heap<JSObject*> here is not actually linked into the heap yet, so won't get traced. Are we relying on compacting GCs from not getting triggered in weird places like this? (eg if compacting GCs only happened from an empty event loop or something, this would be fine.) And why do I not know this already?

Comment 4

[Steve Fink [:sfink] [:s:]]

Attachment: Re-fetch realCallback after AutoEntryScript

My try run hasn't finished yet to confirm that this resolves all of the problems here, but I'm requesting review to ensure that there's not some crazy way that aCallback->CallbackPreserveColor() could somehow return a different value here than it did at the beginning, or something. Or if you have a better idea of how to fix this one.

The issue is that AutoEntryScript has an AutoJSAPI which can GC during construction, and we're holding realCallback and wrappedCallback live across it. This was revealed when I rebuilt sixgill with a fix I made a while back that improves constructor handling (and that I thought I was already running with.) The hazard in detail:

Function

  void mozilla::dom::CallbackObject::CallSetup::CallSetup(mozilla::dom::CallbackObject*, mozilla::ErrorResult*, int8*, uint32, JSCompartment*, uint8)

has unrooted 'realCallback' of type 'JSObject*' live across GC call

  void mozilla::Maybe<T>::emplace(Args&& ...) [with Args = {nsIGlobalObject*&, const char*&, const bool&}; T = mozilla::dom::AutoEntryScript]' at dom/bindings/CallbackObject.cpp:197

GC Function: _ZN7mozilla5MaybeINS_3dom15AutoEntryScriptEE7emplaceIIRP15nsIGlobalObjectRPKcRKbEEEvDpOT_$void mozilla::Maybe<T>::emplace(Args&& ...) [with Args = {nsIGlobalObject*&, const char*&, const bool&}; T = mozilla::dom::AutoEntryScript]
    void mozilla::dom::AutoEntryScript::AutoEntryScript(nsIGlobalObject*, int8*, uint8)
    void mozilla::dom::AutoEntryScript::AutoEntryScript(nsIGlobalObject*, int8*, uint8)
    void mozilla::dom::AutoJSAPI::AutoJSAPI(nsIGlobalObject*, uint8, uint32)
    void mozilla::dom::AutoJSAPI::AutoJSAPI(nsIGlobalObject*, uint8, uint32)
    void mozilla::dom::AutoJSAPI::InitInternal(nsIGlobalObject*, JSObject*, JSContext*, uint8)
    uint8 JS_GetProperty(JSContext*, class JS::Handle<JSObject*>, int8*, class JS::MutableHandle<JS::Value>)
    ...(more stuff, but yeah, JS_GetProperty can GC)...

try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=efaffaebbc27ccdec6b7b0604fd66bb70c59f310

Comment 5

[Steve Fink [:sfink] [:s:]]

Attachment: Rooting in IPC

The analysis now believes that JSAutoCompartment's ctor can GC, with a reason that sounds plausble to me: it could yield to another zone group, which could then trigger a GC.

Comment 6

[Steve Fink [:sfink] [:s:]]

Attachment: Rooting annotations

I don't remember now whether these annotations are necessary for any of the new hazards, or whether they just improve the reasons for GCing that the analysis reoprts. Either way, they're useful.

Comment 7

[Nathan Froyd [:froydnj]]

Comment on attachment 8916713 [details] [diff] [review]
Resolve rooting hazard in CustomElementRegistry by reordering initialization

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

rs=me, I guess?, though depending on ordering of initialization for this stuff to not trigger hazards seems pretty fragile.

Comment 8

[Andrew McCreight [:mccr8]]

Comment on attachment 8916717 [details] [diff] [review]
Re-fetch realCallback after AutoEntryScript

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

Some of this changed in bug 1396904, so it might be worth taking a look at that.

This looks reasonable to me, but bz should take a look at it.

::: dom/bindings/CallbackObject.cpp
@@ +229,5 @@
>      // Check that it's ok to run this callback at all.
> +    //
> +    // Re-fetch the (unwrapped) callback because we may have GC'd and
> +    // invalidated realCallback and wrappedCallback since they were originally
> +    // retrieved. not the wrapper.

"not the wrapper." looks like it should be removed.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8916717 [details] [diff] [review]
Re-fetch realCallback after AutoEntryScript

So this is happening because the JSAutoCompartment inside AutoEntryScript can gc?  Or is there something else going on?

I honestly don't see how JSAutoCompartment being able to gc can be safe.  It gets passed a JSObject*.  Given the above comments, the GC path is via the JSContext::enterZoneGroup call in JSContext::enterNonAtomsCompartment, right?  But after that call we keep using the JSCompartment that came in via our argument... except by that point it can be dead, no?

There are also comments in this code about how we can't CC above this point, and it's not clear to me whether _that_ is still true...

Anyway, my preferred fix here would be to move this whole "is script enabled?" check to right after we grab realCallback.  And probably re-scope some things so that realCallback and wrappedCallback go out of scope as soon as possible (right after setting up win and globalObject for the former, right after grabbing realCallback for the latter) instead of lingering like this.

Comment 10

[Jon Coppeard (:jonco)]

Comment on attachment 8916713 [details] [diff] [review]
Resolve rooting hazard in CustomElementRegistry by reordering initialization

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

Ugh.  I feel like we should have a better solution than reordering of fields for problems like this.  How about moving the possible GC call out of the constructor?

Comment 11

[Steve Fink [:sfink] [:s:]]

(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #9)
> Comment on attachment 8916717 [details] [diff] [review]
> Re-fetch realCallback after AutoEntryScript
> 
> So this is happening because the JSAutoCompartment inside AutoEntryScript
> can gc?  Or is there something else going on?

Yes, that's what it is.

> I honestly don't see how JSAutoCompartment being able to gc can be safe.  It
> gets passed a JSObject*.  Given the above comments, the GC path is via the
> JSContext::enterZoneGroup call in JSContext::enterNonAtomsCompartment,
> right?  But after that call we keep using the JSCompartment that came in via
> our argument... except by that point it can be dead, no?

The JSObject* live range ends before we could GC, but yes, the JSCompartment's live range does reach the GC call. However, entering a compartment "roots" the compartment (and its global, fwiw: http://searchfox.org/mozilla-central/source/js/src/jscompartment.cpp#770 ).

> There are also comments in this code about how we can't CC above this point,
> and it's not clear to me whether _that_ is still true...
> 
> Anyway, my preferred fix here would be to move this whole "is script
> enabled?" check to right after we grab realCallback.  And probably re-scope
> some things so that realCallback and wrappedCallback go out of scope as soon
> as possible (right after setting up win and globalObject for the former,
> right after grabbing realCallback for the latter) instead of lingering like
> this.

Ok, though the analysis doesn't care; it looks at where the live range ends.

Comment 12

[Steve Fink [:sfink] [:s:]]

Attachment: Avoid holding realCallback across AutoEntryScript

I should have looked closer to see that this code is easily movable. I was afraid to disturb anything.

This patch is going to be a pain to review with whitespace changes. I'll upload a non-whitespace diff separately.

Comment 13

[Steve Fink [:sfink] [:s:]]

Attachment: non-whitespace diff of CallbackObject.cpp changes

Comment 14

[Boris Zbarsky [:bzbarsky]]

> the JSCompartment's live range does reach the GC call. However, entering a compartment "roots" the compartment

That doesn't help, unfortunately.  The order of operations in JSContext::enterNonAtomsCompartment is:

1)  Call enterZoneGroup.
2)  Call c->enter().

The enterCompartmentDepth on c is bumped in step 2.  The potential GC happens in step 1, as far as I can tell.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8917199 [details] [diff] [review]
Avoid holding realCallback across AutoEntryScript

r=me; thank you for the -w diff!

Comment 16

[Steve Fink [:sfink] [:s:]]

(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #14)
> > the JSCompartment's live range does reach the GC call. However, entering a compartment "roots" the compartment
> 
> That doesn't help, unfortunately.  The order of operations in
> JSContext::enterNonAtomsCompartment is:
> 
> 1)  Call enterZoneGroup.
> 2)  Call c->enter().
> 
> The enterCompartmentDepth on c is bumped in step 2.  The potential GC
> happens in step 1, as far as I can tell.

Gah. You are right, of course. The full order of operations is

1) enterCompartmentDepth_++
2) Call enterZoneGroup
3) Call c->enter()

which fooled me into thinking that enterCompartmentDepth had already been incremented, because I mixed up JSContext::enterCompartmentDepth_ and JSCompartment::enterCompartmentDepth. :-( Maybe using the same name isn't such a good idea.

Well, crud. It would seem a little weird to swap the order, because then the compartment would be entered during a potentially yielding enterZoneGroup, and I'm not sure of the invariants here -- the cx running in that ZoneGroup might reasonably expect that all of JSCompartment::enterCompartmentDepth_ is from itself. I wonder if the MOZ_ASSERT(!source->hasBeenEntered()) in mergeCompartments might fail?

Heh. I suppose we could make a JSCompartment::root() (or hold()) that increments a different counter that also roots the global.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Sounds like we should just get a separate bug filed on that...

Comment 18

[Steve Fink [:sfink] [:s:]]

Attachment: "Root" compartment while entering it

This is pretty heavyweight. And perhaps I should work on making JSCompartment a GCThing so it can be Rooted<>. Hmm... or maybe to speed up compartment entry, we'd want to specialize Rooted<JSCompartment*> to do exactly what this is doing?

Alternatively, we could just move up the c->enter() call and remove/replace any assertions that might break.

Comment 19

[Jon Coppeard (:jonco)]

Comment on attachment 8917618 [details] [diff] [review]
"Root" compartment while entering it

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

> we could just move up the c->enter() call and remove/replace any assertions that might break.

Yeah we should try that... I don't know what the invariants are here either.

But this is fine.

::: js/src/jscompartment.h
@@ +660,5 @@
> +    }
> +    void releaseGlobal() {
> +        globalHolds--;
> +    }
> +    bool canCollectGlobal() const { return globalHolds == 0 && !hasBeenEntered(); }

I'd prefer inverting the sense of this - shouldTraceGlobal() is clearer to me than canCollectGlobal().

Comment 20

[Steve Fink [:sfink] [:s:]]

Filed bug 1408120 for extending the analysis to detect the untraced-during-construction hazards.

Comment 21

[Steve Fink [:sfink] [:s:]]

Attachment: Pass a Handle into a constructor to avoid a rooting hazard

Ok, I'm going to go with a simple fix for now. This just passes in a Handle to avoid the reported hazard. The current field order is correct; if that mConstructor constructor really *can* GC, then you'd want it in the existing order so that the object stays safe in the Handle during the GC, then gets placed into the Heap<> afterwards. Reordering would make the hazard report go away, but would actually introduce a real GC invalidation hazard because of the untraced Heap<> that wouldn't be reported until bug 1408120 is implemented.

Comment 22

[Steve Fink [:sfink] [:s:]]

Attachment: Rollup patch

Comment 23

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8918040 [details] [diff] [review]
Rollup patch

This was labeled sec-audit, and I'm not sure whether I need sec-approval for that. So I'll request it. Note that one constituent patch is not yet r+, but in its final form it's pretty trivial. I will wait for r+ before landing.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Difficult. You need to trigger a GC at just the right time, and you don't have a whole lot of control over how the stale memory gets used.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? 

"rooting hazards" imply an opportunity for UAF for people aware of what they are.

Which older supported branches are affected by this flaw?

all

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

They should be pretty much the same. I wouldn't be surprised if the analysis patch, at least, had some cosmetic fuzz.

How likely is this patch to cause regressions; how much testing does it need?

Unlikely. There is very little behavior change in this patch. Some operations are reordered a bit.

Comment 24

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8918040 [details] [diff] [review]
Rollup patch

Technically a sec-audit doesn't need sec-approval but I'll give it here anyway in case we re-rate it at some point.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/750de14d8371

Are there branch backports that need to be considered here or can this ride the 58 train?

Comment 26

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8918040 [details] [diff] [review]
Rollup patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
Fix Landed on Version:
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch: 

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
Fix Landed on Version:
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch: 

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/Bug causing the regression]: This goes way back, more or less to generational GC.
[User impact if declined]: probably small number of random crashes, vaguely possible exploits
[Is this code covered by automated tests?]:not usefully, or we would have gotten crashes
[Has the fix been verified in Nightly?]:pushed 2017-10-12 (yesterday)
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]:none. The hazard analysis fixes to detect these could be uplifted in bug 1339989, I guess.
[Is the change risky?]: none of the individual changes seemed very risky at all
[Why is the change risky/not risky?]: most of it is just simple rooting, with one thing involving some code reordering but there's not much that could go wrong within that one function
[String changes made/needed]:none

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: I'm not sure whether we generally land these sorts of minor cleanups on esr52.
User impact if declined: see above. Mostly theoretical crashiness (that would likely fall into one of the existing buckets of "weird crashes we don't know what to do about".)
Fix Landed on Version: 58
Risk to taking this patch (and alternatives if risky): not much
String or UUID changes made by this patch: none

Comment 27

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #25)
> https://hg.mozilla.org/mozilla-central/rev/750de14d8371

https://hg.mozilla.org/mozilla-central/rev/0c3f75e78a5b

Comment 28

[Steve Fink [:sfink] [:s:]]

Attachment: rollup for beta

Ok, I've made a mess here. Everything that should have landed has landed, and if this is rejected for backporting, we're done. (I have no opinion on whether it's worth a backport.)

But if it is approved, use this patch.

What happened: I somehow decided that the patch '"Root" compartment while entering it' belonged to a different, non-security bug, and I spliced it out of my patch stack and landed it separately. Then when RyanVM merged it to mozilla-central, I saw the comment in this bug and assumed that he'd landed the rollup, which I thought contained everything. (Why did I think he would land this patch for me? I dunno, it's a security bug with backport requests, and he does magic landings for backports, and I got confused.) Then later, I landed bug 1339989 which was in the same stack in my workspace (because it depends on these bugs being fixed so they don't show up as hazards after bug 1339989), which resulted in the rollup patch getting pushed. RyanVM noticed that I landed another patch from a bug that was already marked closed.

Bleagh. Sorry for the confusion.

Comment 29

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8918483 [details] [diff] [review]
rollup for beta

Crash fix, Beta57+

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/fe7a664b11b7

Comment 31

[Liz Henry (:lizzard) (relman/hg->git project)]

Since this is a security issue and we've fixed it on other branches, OK, let's try it on ESR.