Closed Bug 1341096 Opened 3 years ago Closed 3 years ago

JS::IsIncrementalBarrierNeeded returns false during incremental sweeping

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr45 53+ fixed
firefox51 --- wontfix
firefox52 --- wontfix
firefox-esr52 53+ fixed
firefox53 + fixed
firefox54 + fixed
firefox55 --- fixed

People

(Reporter: jonco, Assigned: jonco)

Details

(Keywords: sec-high, Whiteboard: [adv-main53+][adv-esr52.1+][adv-esr45.9+])

Attachments

(3 files, 1 obsolete file)

Attached patch fix-incremental-barrier-needed (obsolete) — Splinter Review
This is bad because we may still be marking some zones while sweeping others.

Usually we check the zone state, and checking whether it's marking is correct.  But for the runtime-wide state we need to allow sweeping too.
Attachment #8839229 - Flags: review?(sphink)
Comment on attachment 8839229 [details] [diff] [review]
fix-incremental-barrier-needed

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

The change seems safe enough, but I don't understand -- how we can still be marking zones when incrementalState == Sweep? We drain the mark stack before transitioning, right? Unless this is talking about the new setup with zone groups, I didn't think you could be doing independent incremental GCs on different zones.

::: js/src/jsgc.cpp
@@ +7505,5 @@
> +    if (JS::CurrentThreadIsHeapBusy())
> +        return false;
> +
> +    auto state = cx->runtime()->gc.state();
> +    return state == gc::State::Mark || state == gc::State::Sweep;

I guess MarkRoots doesn't have enough happening under it to be worth checking for? Would it be safer to add it in? (Incremental slices are just beginMarkPhase, but nonincremental has traceRuntimeForMajorGC and things -- but perhaps the barrier isn't needed then anyway?)
Attachment #8839229 - Flags: review?(sphink) → review+
(In reply to Steve Fink [:sfink] [:s:] from comment #1)
> The change seems safe enough, but I don't understand -- how we can still be
> marking zones when incrementalState == Sweep?

There's a call to drainMarkStack at the beginning of GCRuntime::sweepPhase() which is called when incrementalState == Sweep.

The way that incremental sweeping works is to sweep zones in groups (not related to ZoneGroups; this has now become confusing terminology).  Groups are swept in sequence one after the other.  During the sweeping, groups are transitioned from marking to sweeping and no marking happens in those zones after this time.  But zones that have not started sweeping are still marked, and barriers continue to fire and mark things in those zones.  Note that this bug is about use of the global GC state, not the state of any particular zone.

> I guess MarkRoots doesn't have enough happening under it to be worth
> checking for? Would it be safer to add it in? (Incremental slices are just
> beginMarkPhase, but nonincremental has traceRuntimeForMajorGC and things --
> but perhaps the barrier isn't needed then anyway?)

The root marking phase is not incremental so this is not observable.  We'd have to remember to change this if we incrementalise root marking though, so this a good idea.  I'll add it in.
Keywords: sec-high
Comment on attachment 8839229 [details] [diff] [review]
fix-incremental-barrier-needed

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

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 20.

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

Bug 790338.

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

Same patch should apply, or with trivial merge.

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

Unlikely.
Attachment #8839229 - Flags: sec-approval?
This appears to be too late for the Firefox 52 release, which means I'd have to delay sec-approval for checkin a couple of weeks out to a safer time.

ni? release management so they can make a decision if they want to take this for 52. We've had our last beta and are making release candidates next.
Flags: needinfo?(rkothari)
Flags: needinfo?(lhenry)
Flags: needinfo?(jcristau)
I'm going to wontfix this for 52, let's fix it in 53.
Flags: needinfo?(rkothari)
Flags: needinfo?(lhenry)
Flags: needinfo?(jcristau)
sec-approval+ for checkin on 3/21, two weeks into the next cycle. We're a week from final release of 52 right now.
Whiteboard: [checkin on 3/21]
Attachment #8839229 - Flags: sec-approval? → sec-approval+
Backed out because I messed up the condition in the last line.

https://hg.mozilla.org/integration/mozilla-inbound/rev/17d86d61b06c4f25ee790311a2ed9aaf3aab0afb

I'll retest an updated patch before relanding.
I made a mistake applying the code review comments.  Here is a fixed version.
Attachment #8839229 - Attachment is obsolete: true
Attachment #8849550 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/6f4a9281e405
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Please request Aurora/Beta/ESR52/ESR45 approval on this when you get a chance.
Flags: needinfo?(jcoppeard)
Whiteboard: [checkin on 3/21]
Comment on attachment 8849550 [details] [diff] [review]
bug1341096-incremental-barrier v2

Approval Request Comment
[Feature/Bug causing the regression]: Bug 790338.
[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?]: This is a simple change that results in triggering the incremental barrier more often than previously.
[String changes made/needed]: None.
Flags: needinfo?(jcoppeard)
Attachment #8849550 - Flags: approval-mozilla-aurora?
Approval Request Comment
[Feature/Bug causing the regression]: Bug 790338.
[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?]: This is a simple change that results in triggering the incremental barrier more often than previously.
[String changes made/needed]: None.
Attachment #8849941 - Flags: approval-mozilla-beta?
Comment on attachment 8849550 [details] [diff] [review]
bug1341096-incremental-barrier v2

Fix a sec-high. Aurora54+ & Beta53+.
Attachment #8849550 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8849941 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8849941 [details] [diff] [review]
bug1341096-backport

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: sec-high bug.
Fix Landed on Version: 55.
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.
Attachment #8849941 - Flags: approval-mozilla-esr52?
Attached patch bug1341096-esr45Splinter 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: 55.
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.
Attachment #8850481 - Flags: approval-mozilla-esr45?
Attachment #8850481 - Attachment is patch: true
Comment on attachment 8850481 [details] [diff] [review]
bug1341096-esr45

sec-high fix for esr45
Attachment #8850481 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
Comment on attachment 8849941 [details] [diff] [review]
bug1341096-backport

sec-high fix for esr52
Attachment #8849941 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Group: javascript-core-security → core-security-release
Whiteboard: [adv-main53+][adv-esr52.1+][adv-esr48.9+]
Whiteboard: [adv-main53+][adv-esr52.1+][adv-esr48.9+] → [adv-main53+][adv-esr52.1+][adv-esr45.9+]
Setting qe-verify- based on Jon's assessment on manual testing needs and the fact that this fix has automated coverage (see Comment 14).
Flags: qe-verify-
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.