986843 - AutoHoldZone is a hazard with IGC

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bobby Holley (:bholley)]

I spent a few hours mulling over bug 960828 with sfink, terrence, and jonco yesterday. We were still stuck when we left off, but I had an epiphany this morning, and think I have this figured out.

The key point here is that the global doesn't actually get collected (that's why we never see the finalizer running). But but the global's _compartment_ does. So global->global() (the culprit, as identified in bug 960828 comment 72), which compiles down to global()->compartment()->maybeGlobal(), returns garbage, rather than identity.

The scenario goes like this. XPConnect invokes JS_NewGlobalObject, whose first act of business is to invoke js::NewCompartment. This creates a compartment, and cannot GC. Then, before the call to GlobalObject::create (which can GC), we instantiate an AutoHoldZone instance. Once the global object is created, it is rooted (by virtue of either being on the stack or being in a Rooted<>, depending on the rooting configuration), and therefore its compartment is held alive too.

But AutoHoldZone isn't actually good enough. It prevents the zone (and therefore the compartment) from being swept, but it doesn't actually mark anything. So if we happen to begin a GC at the very beginning of GlobalObject::create (before the global itself is actually allocated), we'll do the marking phase, and fail to mark the freshly-allocated compartment. We then return from JS_NewGlobalObject (at which point the AutoHoldZone is out of scope), and at some point later finish up the incremental sweeping. SweepZones invokes SweepCompartments, which finds the compartment unmarked, and deletes it. Boom.

We could mark AutoHoldZone smarter, but it might be simpler to just manually GC (if we're getting close) at the beginning of JS_NewGlobalObject, and then use an AutoLockGC for the rest of the call, since this is touchy stuff.

GC hackers - thoughts? Once we agree on the approach, I'll go ahead and write the patch.

Comment 1

[Andrew McCreight [:mccr8]]

Nice investigation, Bobby!  Once there's a patch ready, we probably want to land this everywhere, as it might help with various very intermittent JS crashes touching dead objects, like bug 984101.

Comment 2

[Andrew McCreight [:mccr8]]

FWIW, AutoHoldZone was added in bug 639270.

Comment 3

[Terrence Cole [:terrence]]

Jon and I discussed this IRL last night and we think we should have |class AutoCompartmentRooter : public CustomAutoRooter<JSCompartment*>| and just forward the trace method. Seems much less difficult than messing with GC timings.

Comment 4

[Bobby Holley (:bholley)]

Sounds good. I'll write a patch.

Comment 5

[Jon Coppeard (:jonco)]

Attachment: bug986843-removeAutoHoldZone

bholley, sorry I didn't see your last comment and wrote this patch!

I wasn't able to come up with a testcase to reproduce this though -- if anyone has any ideas it would be great if we could test this.  I was trying creating new globals with different frequency values for gczeal 9.

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 8395667 [details] [diff] [review]
bug986843-removeAutoHoldZone

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

::: js/src/jsapi.cpp
@@ +2494,3 @@
>      Rooted<GlobalObject *> global(cx);
>      {
> +        AutoCompartmentRooter hold(cx, compartment);

This scoping worries me. How about just giving AutoCompartmentRooter an operator JSCompartment*() doing:

AutoCompartmentRooter compartment(cx, NewCompartment(...))?

Comment 7

[Terrence Cole [:terrence]]

Comment on attachment 8395667 [details] [diff] [review]
bug986843-removeAutoHoldZone

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

r=me

::: js/src/jsapi.cpp
@@ -2410,5 @@
>  {
>    public:
> -    explicit AutoHoldZone(Zone *zone
> -                          MOZ_GUARD_OBJECT_NOTIFIER_PARAM)
> -      : holdp(&zone->hold)

We should be able to kill JSCompartment::hold now too.

Comment 8

[Jon Coppeard (:jonco)]

Attachment: bug986843-removeAutoHoldZone

Patch updated with comments.

Also I renamed the confusing one argument overload of JSCompartment::mark() to markRoots() leaving the no-argument version to set the compartment's mark flag.

Comment 9

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e05b578dcd6d

Comment 10

[Ryan VanderMeulen [:RyanVM]]

This should have had sec-approval before landing no?
https://wiki.mozilla.org/Security/Bug_Approval_Process

Also, does this affect B2G?

Comment 11

[Andrew McCreight [:mccr8]]

Yes on both. ;)

Comment 12

[Jon Coppeard (:jonco)]

Ah, I'm sorry, I didn't realise that!

It's worth pointing out that patch this doesn't necessarily fix the problem, but it's a nice cleanup anyway.

Here's the approval request details:

How easily could an exploit be constructed based on the patch?

I think it would be hard to create an exploit based on this as the GC would have to happen at exactly the right point.

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

The patch is a tidy up and removes some unnecessary functionality, so it's not obvious that this is a security fix.

Which older supported branches are affected by this flaw?

All of them - this code was introduced in 2011 in bug 639270.

If not all supported branches, which bug introduced the flaw?

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

Should be trivial.

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

Unlikely to cause regressions.

Comment 13

[Andrew McCreight [:mccr8]]

It looks like bug 960828 hasn't happened on inbound since it landed yesterday, which is promising.

Comment 14

[Andrew McCreight [:mccr8]]

Yeah, bug 960828 hasn't happened anywhere since this happened, so it looks like this worked.  Awesome!  Can you nominate for landing this on branches, Jon?  Thanks.  It seems like you can also close both bugs.

Comment 16

[Jon Coppeard (:jonco)]

Comment on attachment 8395785 [details] [diff] [review]
bug986843-removeAutoHoldZone

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 639270
User impact if declined: Potential security issue
Testing completed: On mozilla-central for a week.
Risk to taking this patch (and alternatives if risky): Low
String or UUID changes made by this patch: None

Comment 17

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/mozilla-central/rev/e05b578dcd6d

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8395785 [details] [diff] [review]
bug986843-removeAutoHoldZone

Per https://wiki.mozilla.org/Release_Management/B2G_Landing, this will land on the B2G branches once approved and landed on the other Gecko branches.

Comment 19

[Bobby Holley (:bholley)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #18)
> Comment on attachment 8395785 [details] [diff] [review]
> bug986843-removeAutoHoldZone
> 
> Per https://wiki.mozilla.org/Release_Management/B2G_Landing, this will land
> on the B2G branches once approved and landed on the other Gecko branches.

Does the "will land" part here mean that this process happens automatically by virtue of having landed on aurora/beta?

Comment 20

[Ryan VanderMeulen [:RyanVM]]

From the link: "sec-high and sec-critical patches have automatic approval to land if the fix has landed on all affected Firefox branches"

And the bottom of the page has the bug queries that are looked at daily for such uplifts ;)

Comment 21

[Jon Coppeard (:jonco)]

https://tbpl.mozilla.org/?tree=Mozilla-Aurora&rev=f2fcf4c36ee9

Comment 22

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f2fcf4c36ee9
https://hg.mozilla.org/releases/mozilla-beta/rev/bee9c026e827

Comment 23

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/releases/mozilla-esr24/rev/42531d5e6ca2

Comment 24

[Andrew McCreight [:mccr8]]

FYI, Ryan will do landings on older branches for you.  He has some queries that look for things with aurora approval set, so you don't even need to set checkin-needed.

If you do want to land stuff yourself, then you need to set the status once you land (except on inbound).

Comment 25

[Andrew McCreight [:mccr8]]

> aurora approval
and similarly for all the other branches, obviously...

Comment 26

[Ed Morley [:emorley]]

(In reply to Jon Coppeard (:jonco) from comment #23)
> https://hg.mozilla.org/releases/mozilla-esr24/rev/42531d5e6ca2

Whilst this has an intermittent present, there seems to be other failures too:
https://tbpl.mozilla.org/php/getParsedLog.php?id=37140911&tree=Mozilla-Esr24

have retriggered.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/998d84030f70
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/ab6f3500bce2

Comment 28

[Jon Coppeard (:jonco)]

I've backed this out of mozilla-esr24 as the test failures seem related to the patch:

https://hg.mozilla.org/releases/mozilla-esr24/rev/32160fd34f6b

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Same failures on b2g26.
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/c3527c157a13

Comment 30

[Ryan VanderMeulen [:RyanVM]]

And b2g28.
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/f2f9966908cd

Comment 31

[Jon Coppeard (:jonco)]

The assertion that's triggering is complaining that a compartment is not being destroyed even though the zone contains no allocated GC things.  The compartment is not being destroyed because it is marked by AutoCompartmentRooter.

This is in fact an allowable state now - the point of adding AutoCompartmentRooter was to mark the compartment in case we start a GC before we manage to create its global.

It's not happening on recent versions because the code that creates new type objects now suppresses GC for part of its operation and allocates GC things into the zone without the possibility of triggering GC.

I think the fix for this is to also not destroy zones if they contain any marked compartments, even if they don't contain any GC things.  I'm currently testing a patch for this.

I also think it's worth applying this change to mozilla-central (and everywhere else) since it's only an accident of the current arrangement of the object creation code that this doesn't happen there.

Comment 32

[Jon Coppeard (:jonco)]

Attachment: bug986843-dontSweepMarkedZones

Patch to change SweepZones() not destroy a zone if it contains any marked compartments, even if that zone doesn't contain any GC things.

Comment 33

[Terrence Cole [:terrence]]

Comment on attachment 8401750 [details] [diff] [review]
bug986843-dontSweepMarkedZones

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

r=me  Clear ownership semantics for compartments? What novelty!

Comment 34

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ded76bd93630

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ded76bd93630

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/70aebd1f91b2
https://hg.mozilla.org/releases/mozilla-beta/rev/ed9793adc2c7
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/836f5a97d1fd
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/5a9b4748be6a
https://hg.mozilla.org/releases/mozilla-esr24/rev/219fdd8ca2c6

Comment 37

[Andrei Vaida [:avaida]]

Does this fix require manual QA? If that is the case, could you please advise on how to verify it manually?

Comment 38

[Jon Coppeard (:jonco)]

I don't know what the criteria are for requiring manual QA, but this would be hard to test manually as it was very intermittent in the first place, so I'd say not.

Comment 39

[Andrei Vaida [:avaida]]

(In reply to Jon Coppeard (:jonco) from comment #38)
> I don't know what the criteria are for requiring manual QA, but this would
> be hard to test manually as it was very intermittent in the first place, so
> I'd say not.
Thanks for the quick reply, Jon! Marking this as [qa-] based on Comment 38.