1997118 - Aborting GC during marking can leave gray marking state invalid

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect, P3)

Description

[Jon Coppeard (:jonco)]

While working on bug 1994871 I realised that there's a problem with gray unmarking where aborting an incremental GC can leave things in a bad state.

Gray unmarking stops when it comes to a zone that is currently being incrementally marked and hands the work over to the GC marking (which is doing something very similar but can do it incrementally and in hopefully in parallel too). When GC marking traverses something gray in a zone that isn't being marked it will perform gray unmarking on it. So gray unmarking can flow through zones that are being collected and out again.

This breaks down if we abort incremental GC while it's marking. This can leave us with black to gray edges, violating our promise to the cycle collector. This can possibly lead to use after free if the CC can be persuaded to free the gray thing that should have been marked black.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: (secure)

We depend on GC marking to propagate gray umarking through zones that are being
marked incrementally. Aborting this raises the possibility of creating black to
gray edges as previously happened with this testcase.

Marking the gray bits as invalid will cause the CC to perform another GC to
redo the marking. Aborting GC is pretty rare in practice so I don't expect this
to have a performance impact.

Comment 2

[Jon Coppeard (:jonco)]

I don't know how one would craft an exploit based on this and aborting GC is pretty rare in practice so I'm going to mark this sec-moderate for now.

Comment 3

[Andrew McCreight [:mccr8]]

Yes, that seems reasonable. I'll mark this as a "use-after-free" but really it is more of a "use-after-unlink", which is usually less bad but maybe it could be bad in some weird circumstance depending on the unlink method.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/7a852d3774bf
https://hg.mozilla.org/integration/autoland/rev/62acfc07049f
Mark gray bits as invalid if we abort GC during marking r=sfink

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/12bf2b1c5311
https://hg.mozilla.org/integration/autoland/rev/d53295d3cd83
Revert "Bug 1997118 - Mark gray bits as invalid if we abort GC during marking r=sfink" for causing assertion failure in Bug 1997268

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

• Push with failures
• Failure log

PROCESS-CRASH | 649f64b7-b91c-342e-36d6-5f5fce254c15 | MOZ_CRASH(JS::UnmarkGrayGCThingRecursively(thing)) [@ js::gc::ExposeGCThingToActiveJS] | js/src/tests/non262/extensions/regress-363988.js

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/83ce0f20261e
https://hg.mozilla.org/integration/autoland/rev/d5d4c4541e46
Mark gray bits as invalid if we abort GC during marking r=sfink

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 9

[BugBot [:suhaib / :marco]]

The patch landed in nightly and beta is affected.
:jonco, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval. Also, don't forget to request an uplift for the patches in the regression caused by this fix.
  • See https://wiki.mozilla.org/Release_Management/Requesting_an_Uplift for documentation on how to request an uplift.
• If no, please set status-firefox145 to wontfix.

For more information, please visit BugBot documentation.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Doesn't seem like something we need to uplift at the last second for 145 given comment 2, but probably still worth an ESR140 uplift next cycle.

Comment 11

[Jon Coppeard (:jonco)]

Comment on attachment 9523270 [details]
(secure)

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is a sec-moderate security vulnerability
• User impact if declined: Potential crash or security vulnerability
• Fix Landed on Version: 146
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The change is very simple (mark gray marking state as invalid in one place). The change is covered by tests (the bulk of the patch). This has been on central for a while.

Comment 12

[Donal Meehan [:dmeehan]]

Comment on attachment 9523270 [details]
(secure)

The request flag was missing on the attachment

Comment 13

[Donal Meehan [:dmeehan]]

:jonco there are conflicts with ESR140. Could you please rebase?
https://wiki.mozilla.org/Release_Management/Requesting_an_Uplift#Manual_Cherry-Pick_(Recommended_if_Conflicts_Are_Likely)

Comment 14

[Jon Coppeard (:jonco)]

Attachment: (secure)

We depend on GC marking to propagate gray umarking through zones that are being
marked incrementally. Aborting this raises the possibility of creating black to
gray edges as previously happened with this testcase.

Marking the gray bits as invalid will cause the CC to perform another GC to
redo the marking. Aborting GC is pretty rare in practice so I don't expect this
to have a performance impact.

Comment 15

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr140/rev/53c28fcd52f9

Comment 16

[Jon Coppeard (:jonco)]

(In reply to Pulsebot from comment #15)
The uplift has caused some assertion failures on esr140. There have been several changes in this area recently and I'm not sure what the dependency is here. Since this is only sec-moderate on balance I'd rather leave this to ride the trains.

Comment 17

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr140/rev/4163953a96b2

Comment 18

[Donal Meehan [:dmeehan]]

Backed out from ESR140 for causing Bug 2003035, see Comment 16.