1328251 - GC races involving the self-hosting zone and workers

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Jan de Mooij [:jandem]]

I was looking at the ConstraintTypeSet::sweep topcrash in bug 1220385 and noticed we can call ObjectGroup::sweep for the self-hosting zone. This seemed suspicious because it can race with worker threads (the s-h zone is shared).

I wrote some shell tests involving evalInWorker and below is a test case that asserts reliably on OS X and Linux:

$ JS_GC_ZEAL=2 dist/bin/js test.js
Assertion failure: CurrentThreadCanAccessZone(zone), at Heap.h:1268

Opt builds crash in GC trace functions.

function f() {
    for (var i=0; i<100; i++)
        evalInWorker("while (true) { var a = [1, 2, 3]; a.indexOf(1); relazifyFunctions(); }");
    while (true) {
        a = [];
	a.indexOf(a);
        gc();
    }
}
f();

Comment 1

[Jan de Mooij [:jandem]]

This is probably related to bug 1324512 and bug 1311060 that decoder found, same signature and these tests also use evalInWorker.

Comment 2

[Jon Coppeard (:jonco)]

(In reply to Jan de Mooij [:jandem] from comment #1)
Yes, this assertion failure is the same problem as bug 1324512: JSRuntime::cloneSelfHostedFunctionScript uses a context which has entered the target compartment to root a function which is in a different compartment (which is in the self hosting zone).  The rooting API pushes Rooteds onto a list stored in the context's zone in most cases so what's happening here is that we're pushing a Rooted onto the list for a different zone.  I'm currently trying to figure out a good way to assert that this doesn't happen.

Comment 4

[Jon Coppeard (:jonco)]

Attachment: bug1328251-check-traced-runtime

Here's a patch to not trace GC things in other runtimes.  This simplifies the current checks slightly and adds an assertion that the things we're skipping are those that are legitimately shared between runtimes: self hosted things, permanent atoms and well known symbols.

This fixes the assertion with the testcase above and that in bug 1324512.  I'm no sure whether it will fix the problem with calling ObjectGroup::sweep for the self-hosting zone.

It doesn't address the problem I identified in comment 2 which turned out not to be the issue here.  I'll file a separate bug for that.

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Jon Coppeard (:jonco) from comment #4)
> I'm no sure whether it will fix the problem with calling ObjectGroup::sweep
> for the self-hosting zone.

For this, can we assert in ObjectGroup::sweep etc that the Zone belongs to the current runtime?

Comment 6

[Jon Coppeard (:jonco)]

Comment on attachment 8824479 [details] [diff] [review]
bug1328251-check-traced-runtime

This causes assertions in CGC tests.  Cancelling review request.

Comment 7

[Jon Coppeard (:jonco)]

Attachment: bug1328251-check-traced-runtime v2

The check for tracing inter-runtime pointers is needed in CheckTracedThing too, and we also need to check this during compacting.

Comment 8

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

Comment on attachment 8825041 [details] [diff] [review]
bug1328251-check-traced-runtime v2

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

The new code makes more sense to read, too.

Comment 9

[Jon Coppeard (:jonco)]

Comment on attachment 8825041 [details] [diff] [review]
bug1328251-check-traced-runtime v2

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

Very difficult.

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

No.

Which older supported branches are affected by this flaw?

Everything back to 30.

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

Bug 964057.

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

Should be straightforward.

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

Unlikely.

Comment 10

[Julien Cristau [:jcristau]]

sec-high, tracking for 51+

Comment 11

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

Comment on attachment 8825041 [details] [diff] [review]
bug1328251-check-traced-runtime v2

sec-approval+ for trunk. We should get branch patches made and nominated as it is tracking+ for affected branches.

Comment 12

[Jon Coppeard (:jonco)]

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

Comment 13

[Jon Coppeard (:jonco)]

Attachment: bug1328251-esr45

Comment 14

[Carsten Book [:Tomcat]]

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

Comment 15

[Gerry Chang [:gchang]]

Hi :jonco, 
Do you think this is worth uplifting to Beta51 & Aurora52?

Comment 16

[Jon Coppeard (:jonco)]

Comment on attachment 8825041 [details] [diff] [review]
bug1328251-check-traced-runtime v2

Approval Request Comment
[Feature/Bug causing the regression]: Bug 964057.
[User impact if declined]: Possible crash / security vulnerability.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: It's a small change and is covered by tests
[String changes made/needed]: None

Comment 17

[Jon Coppeard (:jonco)]

Comment on attachment 8825740 [details] [diff] [review]
bug1328251-esr45

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

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

Comment 18

[Gerry Chang [:gchang]]

Comment on attachment 8825041 [details] [diff] [review]
bug1328251-check-traced-runtime v2

Fix a sec-high. Beta51+ & Aurora52+. Should be in 51 beta 14.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d50f8e9f913c

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3f09b0633184

Comment 21

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

Comment on attachment 8825740 [details] [diff] [review]
bug1328251-esr45

Sec-high, meets the ESR45 uplift criteria. Let's include this fix in ESR45.7

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-esr45/rev/e4ac8a66d3dbb93b51a59051ed8f183b743f43e9