1551907 - WeakMaps can become gray after being marked black

Status: RESOLVED FIXED

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

Description

[Jon Coppeard (:jonco)]

I added some more tests for weak map marking and ran into a bug: if we mark a weak map gray after it has already been marked black, it changes colour black to gray. This should not happen. (This refers to the weak map's internal concept of its color, not that of its associated JSObject).

I don't know how it would be possible to exploit this but it makes sense to hide this just in case.

Comment 1

[Andrew McCreight [:mccr8]]

It feels like the worst that can happen here is a reference from a black object to a gray object, but I think the worst that can do is a "use after unlink", which will result null pointer dereferences, unless there are other bugs in code, so I'll mark it sec-moderate.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: Bug 1551907 - Fix interaction between gray unmarking and weakmap marking r=sfink

We shouldn't allow a weak map to become gray after it has already been marked black.

Depends on D30948

Comment 3

[Jon Coppeard (:jonco)]

Attachment: Bug 1552118 - Don't release foreground finalized arenas until the end of sweeping the zone group r=sfink

This releases all foreground finalized arenas at the end of sweeping the sweep group rather than at the end of sweeping the zone (for objects) or immediately (for everything else) as happens currently. This simplifies the code in a couple of places and I don't think it will have any noticeable effects.

Comment 4

[Jon Coppeard (:jonco)]

Comment on attachment 9065353 [details]
Bug 1552118 - Don't release foreground finalized arenas until the end of sweeping the zone group r=sfink

I uploaded this patch to the wrong bug though - this is a fix for bug 1552118.

Comment 5

[Phabricator Automation]

Comment on attachment 9065353 [details]
Bug 1552118 - Don't release foreground finalized arenas until the end of sweeping the zone group r=sfink

Revision D31415 was moved to bug 1552118. Setting attachment 9065353 [details] to obsolete.

Comment 6

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5d9d8566c195a2602a47cf4f9973a6e672fe7a16

Comment 7

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

https://hg.mozilla.org/mozilla-central/rev/5d9d8566c195

Comment 8

[Ryan VanderMeulen [:RyanVM]]

I'm guessing we'll want to nominate this for Beta uplift at least. Do we want this on ESR60 also?

Comment 9

[Jon Coppeard (:jonco)]

Comment on attachment 9065345 [details]
Bug 1551907 - Fix interaction between gray unmarking and weakmap marking r=sfink

Beta/Release Uplift Approval Request

• 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?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The change to the code is very simple and is covered by additional tests in the patch.
• String changes made/needed:

Comment 10

[Jon Coppeard (:jonco)]

Comment on attachment 9065345 [details]
Bug 1551907 - Fix interaction between gray unmarking and weakmap marking r=sfink

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: I don't know how easy this would be to trigger but it has the potential to be a crash / security vulnerability. The fix is simple so it would be nice to take this.
• User impact if declined: Possible crash / security vulnerability.
• Fix Landed on Version: 69
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is a simple change that is covered by tests in the patch.
• String or UUID changes made by this patch:

Comment 11

[Julien Cristau [:jcristau]]

Comment on attachment 9065345 [details]
Bug 1551907 - Fix interaction between gray unmarking and weakmap marking r=sfink

sec fix in GC, approved for 68.0b4

Comment 12

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

The patch has conflicts with bug 1551275. Please provide a patch which applies cleanly.

Comment 13

[Jon Coppeard (:jonco)]

(In reply to Sebastian Hengst [:aryx] (needinfo on intermittent or backout) from comment #12)
Sorry, this does require uplift of bug 1551275. I'll request.

Comment 14

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

https://hg.mozilla.org/releases/mozilla-beta/rev/62a00a1d8a7b4726b6ecb424bc5e15533bb0d6cc

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9065345 [details]
Bug 1551907 - Fix interaction between gray unmarking and weakmap marking r=sfink

Fixes a possibly s-s GC crash. Includes automated tests. Approved for 60.8esr.

Comment 16

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

Please provide a patch for ESR60. There are conflicts due to bug 1479388 (at least).

Comment 17

[Jon Coppeard (:jonco)]

Turns out that this bug was introduced in bug 1463462 with landed in FF66 so the backport is not required.