2028416 - Key color not corectly updated when marking WeakMap symbol keys

Status: RESOLVED FIXED

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

Description

[Jon Coppeard (:jonco)]

Claude pointed out that we don't update |keyColor| in WeakMap::markEntry after making a symbol key that is referenced by an uncollected zone. This means that we may not mark the value of a weakmap entry when we should. It doesn't think this is a big deal because it thinks that this will be marked by iterative weak map marking.

However we don't usually do that any more since we use incremental weak map marking, so I think this could be a problem. Having said that, we've had a bunch of bugs in this area and we do have tests that exercise this and those tests pass. So I'm not sure whether this is a real problem or not, but out of caution I'll make this security sensitive. I'll mark it sec-other to start with.

Full Claude output:

  Finding 6: markEntry stale keyColor after symbol key marking                                                           
                                                                                                                           
  File: WeakMap-inl.h:217-230 and 249
                                                                                                                           
  CellColor keyColor = gc::detail::GetEffectiveColor(marker, key.get());  // line 217                                    
  ...                                                                                                                      
  if (keyIsSymbol && keyColor < markColor) {                                                                               
      ...                                                                                                                  
      TraceEdge(trc, &key, "WeakMap symbol key");  // May mark key! (line 228)                                             
      // keyColor is NOT updated here                                                                                      
  }                                                                                                                        
  ...                                                                                                                      
  if (IsMarked(keyColor)) {  // line 249: uses stale keyColor                                                              
                                                                                                                           
  After tracing a symbol key at line 228, the key's color increases, but keyColor is not updated. This means the           
  value-marking check at line 249 uses a stale keyColor. If the key was just marked from White, IsMarked(keyColor) is      
  false, and the value is NOT marked in this iteration.                                                                    
                                                                                                                         
  The comment at line 223 says "There's no need to set |marked| as this would have been marked later anyway" -- the symbol 
  was going to be marked anyway (referenced by an uncollected zone). The value will be marked on a subsequent markEntries
  iteration when the key's new color is observed. So correctness is maintained through the iterative marking loop.         
                                                                                                                         
  Severity: None for correctness; the iterative algorithm handles it. Noting it because the stale variable is subtle and a 
  future refactor could break the invariant.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: (secure)

This updates the local |keyColor| when marking the symbol key. We check this
below when working out whether to trace the value.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/de0543d592c5
https://hg.mozilla.org/integration/autoland/rev/b9ae7dbd0749
Update key color when marking weakmap symbol keys r=sfink

Comment 3

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

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

Comment 4

[Jon Coppeard (:jonco)]

Upgrading this to sec-high now Claude found a way to trigger this in bug 2029788. I'll dupe that to this one.

Comment 6

[Jon Coppeard (:jonco)]

Attachment: Bug 2028416 - Add testcase r?sfink

Comment 7

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined/Reason for urgency: Potential crash or security vulnerability.
• Code covered by automated testing?: yes
• Fix verified in Nightly?: yes
• Needs manual QE testing?: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: low
• Explanation of risk level: This is a simple fix and has been verified by tests. It's been on central without issues for 4 days.
• String changes made/needed?: None
• Is Android affected?: yes

Comment 8

[Jon Coppeard (:jonco)]

Attachment: (secure)

This updates the local |keyColor| when marking the symbol key. We check this
below when working out whether to trace the value.

Original Revision: https://phabricator.services.mozilla.com/D291527

Comment 9

[BugBot [:suhaib / :marco]]

A patch has been attached on this bug, which was already closed. Filing a separate bug will ensure better tracking. If this was not by mistake and further action is needed, please alert the appropriate party. (Or: if the patch doesn't change behavior -- e.g. landing a test case, or fixing a typo -- then feel free to disregard this message)

Comment 10

[Dianna Smith [:diannaS]]

does this need a delayed sec-approval to uplift now that it is sec-high?

Comment 11

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/77a8e46cf651
https://hg.mozilla.org/releases/mozilla-beta/rev/113edc19bfcc

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/5d2d1de4b1db
https://hg.mozilla.org/integration/autoland/rev/71e48c60d61f
Add testcase r=sfink

Comment 13

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/71e48c60d61f