1667912 - Nonincremental weakmap marking incorrectly splits up Zones

Status: RESOLVED FIXED

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

Description

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

When I made the incremental weakmap marking preference, I kept the nonincremental code path in place in case we needed to switch back. But it didn't do exactly what it did before. In particular, nonincremental mode builds the whole gcWeakKeys table at one time when switching the weak marking mode. It should be doing that for all collecting zones at once, because the weakmaps in a key zone may add entries to gcWeakKeys in a delegate's zone. Instead, it's does it one zone at a time: clear out gcWeakKeys, then scan through the weakmaps in that zone. If you process a key's zone first, then the delegate's entry in gcWeakKeys will be missing.

This might be exploitable as a UAF.

Comment 1

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

Attachment: Bug 1667912 - Fix nonincremental weakmap marking

Comment 2

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

Comment on attachment 9178337 [details]
Bug 1667912 - Fix nonincremental weakmap marking

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Pretty hard. The patch itself just reorders things in a way that doesn't suggest why it might be important. If you understood the flaw being, then it's still pretty hard.
• 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?: esr78
• If not all supported branches, which bug introduced the flaw?: Bug 1633176
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: I expect it would apply pretty easily.
• How likely is this patch to cause regressions; how much testing does it need?: There is a good chance that there will still be even harder to trigger bugs after this, but the chances of this patch causing a regression is very low. It simplifies the non-incremental version of weakmap marking (which is what is currently enabled.)

Comment 3

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

Comment on attachment 9178337 [details]
Bug 1667912 - Fix nonincremental weakmap marking

Beta/Release Uplift Approval Request

• User impact if declined: Rare intermittent in CI, potentially exploitable UAF in production. Fairly hard to trigger/exploit.
• Is this code covered by automated tests?: Unknown
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce: (I don't have a way to trigger this reliably.)
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): It makes the behavior simpler, and matches earlier behavior that was mistakenly changed.
• String changes made/needed: none

Comment 4

[Julien Cristau [:jcristau]]

It's too late for 82 now.

Comment 5

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Comment on attachment 9178337 [details]
Bug 1667912 - Fix nonincremental weakmap marking

Approved to land and uplift

Comment 6

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

This landed: https://hg.mozilla.org/integration/autoland/rev/d922e9a28e68f4c19a731b20e0c9bf0f191e0150

And got backed out for Build bustage and reftest failure in builds/worker/checkouts/gecko/js/src/gc/GC.cpp:

https://hg.mozilla.org/integration/autoland/rev/a94f3815ca4a816c73f7e09b508176f7a719381f

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&selectedTaskRun=QW1WLiIfQBuBmnZrTk9uSA.0&revision=d922e9a28e68f4c19a731b20e0c9bf0f191e0150

Assertion failure: zone->gcWeakKeys().count() == 0, at /builds/worker/checkouts/gecko/js/src/gc/GC.cpp:4350

Comment 7

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

Ugh. I fear I may have messed up in determining which patches should land now and which can be later. This may require the first patch in bug 1667913 after all. But I have an rr recording of this happening locally, so I should be able to figure it out soon.

Comment 8

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

Crud. No, it's nothing to do with bug 1667913. It's that I left a diagnostic assertion in the latest patch that I uploaded -- I wasn't sure whether it would hold or not, so I was using to identify any remaining sources of gcWeakKeys entries that I hadn't yet stamped out. But that's just for efficiency and simplifying the proof of correctness; it's not actually needed for correctness. (It's pretty clear from the code -- I assert that a vector is empty immediately before clearing the vector.)

I may just remove the assert, but I'll first check to see if there isn't a way to weaken the assertion to make it hold. (The reason for a nonempty gcWeakKeys that it's identifying right now is a valid one: we mark twice, with different colors. The assert should only be made at the beginning of the first mark; it's ok if the first mark adds some entries before the second one. No barriers run in between.)

Comment 9

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

https://hg.mozilla.org/integration/autoland/rev/61c35792ca7021377e42150db54b3935b0fd3c40
https://hg.mozilla.org/mozilla-central/rev/61c35792ca70

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/172d59414161

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Comment 12

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/74c1b8acee38