Closed Bug 1328251 Opened 8 years ago Closed 8 years ago

GC races involving the self-hosting zone and workers


(Core :: JavaScript: GC, defect)

Not set



Tracking Status
firefox-esr45 51+ fixed
firefox50 --- wontfix
firefox51 + fixed
firefox52 + fixed
firefox53 + fixed


(Reporter: jandem, Assigned: jonco)



(Keywords: crash, sec-high, Whiteboard: [post-critsmash-triage][adv-main51+][adv-esr45.7+])


(2 files, 1 obsolete file)

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 = [];
Keywords: crash, sec-high
This is probably related to bug 1324512 and bug 1311060 that decoder found, same signature and these tests also use evalInWorker.
(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.
Assignee: nobody → jcoppeard
Attached patch bug1328251-check-traced-runtime (obsolete) — Splinter Review
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.
Attachment #8824479 - Flags: review?(sphink)
(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 on attachment 8824479 [details] [diff] [review]

This causes assertions in CGC tests.  Cancelling review request.
Attachment #8824479 - Flags: review?(sphink)
The check for tracing inter-runtime pointers is needed in CheckTracedThing too, and we also need to check this during compacting.
Attachment #8824479 - Attachment is obsolete: true
Attachment #8825041 - Flags: review?(sphink)
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.
Attachment #8825041 - Flags: review?(sphink) → review+
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?


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?

Attachment #8825041 - Flags: sec-approval?
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.
Attachment #8825041 - Flags: sec-approval? → sec-approval+
Attached patch bug1328251-esr45Splinter Review
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Group: javascript-core-security → core-security-release
Hi :jonco, 
Do you think this is worth uplifting to Beta51 & Aurora52?
Flags: needinfo?(jcoppeard)
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
Flags: needinfo?(jcoppeard)
Attachment #8825041 - Flags: approval-mozilla-beta?
Attachment #8825041 - Flags: approval-mozilla-aurora?
Comment on attachment 8825740 [details] [diff] [review]

[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 for more info.
Attachment #8825740 - Flags: approval-mozilla-esr45?
Comment on attachment 8825041 [details] [diff] [review]
bug1328251-check-traced-runtime v2

Fix a sec-high. Beta51+ & Aurora52+. Should be in 51 beta 14.
Attachment #8825041 - Flags: approval-mozilla-beta?
Attachment #8825041 - Flags: approval-mozilla-beta+
Attachment #8825041 - Flags: approval-mozilla-aurora?
Attachment #8825041 - Flags: approval-mozilla-aurora+
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Comment on attachment 8825740 [details] [diff] [review]

Sec-high, meets the ESR45 uplift criteria. Let's include this fix in ESR45.7
Attachment #8825740 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main51+][adv-esr45.7+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.