2042465 - WeakMap read barrier is different between VM and JIT paths

Status: RESOLVED FIXED

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

Description

[Jon Coppeard (:jonco)]

Found by Jan with the help of claude.

Here we always mark the symbol in the WeakMap's zone: https://searchfox.org/firefox-main/rev/9d2be0cb8dbf1c3a7ef6ba79be35d637d0327eaa/js/src/gc/WeakMap.h#533
but the JIT version skips that if the black bit is set: https://searchfox.org/firefox-main/rev/9d2be0cb8dbf1c3a7ef6ba79be35d637d0327eaa/js/src/jit/MacroAssembler.cpp#8385-8386

It's not clear how serious this is but marking security sensitive to be safe. We should make these paths the same.

Comment 1

[Iain Ireland [:iain]]

When I wrote this code, I was working under the assumption that the black bit being set implied that we had already done any necessary marking. It sounds like maybe that's not true?

IIUC, the problem is with symbols that live in the atom zone: even if they're marked black in their own zone, we need to do some work to update our own set of marked atoms. Bug 2029295 says that Symbol is "the only atoms-zone type that can be marked gray in the per-zone atom marking bitmap", so I assume that's why we don't need an equivalent path for strings.

In that bug, the issue is that we can manufacture a weakmap where a black key points to a gray value, and we return the gray value from the weakmap without marking it black. This comment in particular says that the Symbol is marked black in the atom zone, but gray in the weakmap's zone. (It's LLM analysis, but I don't see any reason to doubt it.) In the current jitcode, it is indeed the case that we exit early if the symbol is marked black in its own chunk, which sounds like the same situation to me.

I could very easily be missing something, but it does seem like this could be a real issue. If so, I think it would make sense to either:

1. Rename MacroAssembler::emitValueReadBarrierFastPath to make it WeakMap-specific, and add an isSymbol check somewhere around here that will unconditionally call the barrier code. This would also require changing js::jit::WeakMapValueReadBarrier, which currently asserts that the cell is not marked black.
2. Rename MacroAssembler::emitValueReadBarrierFastPath to something like MacroAssembler::emitExposeToActiveJSFastPath (which is more likely to be reusable in the future), then add a separate isSymbol check here in CodeGenerator::visitWeakMapGetObject to handle this case. (We might be able to set it up so that it calls the same C++ function, but with a boolean argument to indicate whether we need the ExposeToActiveJS path, or just the Symbol part.)

Comment 2

[Jon Coppeard (:jonco)]

(In reply to Iain Ireland [:iain] from comment #1)

When I wrote this code, I was working under the assumption that the black bit being set implied that we had already done any necessary marking. It sounds like maybe that's not true?

Indeed. Part of the reason this is confusing is because the GC mark bits and the atom marking bitmaps are separate things that should perhaps have had more dissimilar names. It is possible for a symbol which was previously marked black to be read out of a weakmap in a zone where the symbol is gray in that zone's atom marking bitmap. In that case we do need to update the atom marking bitmap.

Comment 3

[Jon Coppeard (:jonco)]

Attachment: (secure)

Claude was used to generate a test case for this.

Hopefully the extra check doesn't affect performance too much.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/1a134015e8ae
https://hg.mozilla.org/integration/autoland/rev/bf45fdd35618
Update JIT version of WeakMap read barrier to mark symbols in the map zone's atom marking bitmap r=iain

Comment 5

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

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

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta uplift.

Comment 7

[Jon Coppeard (:jonco)]

Attachment: (secure)

Claude was used to generate a test case for this.

Hopefully the extra check doesn't affect performance too much.

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

Comment 8

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined/Reason for urgency: Potential crash / 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: The patch adds an extra branch to the JIT compiled version of the barrier so that symbols are handled by the VM function and updates the latter accordingly. This is a relatively simple change for a complex area.

This fix is covered by tests and has been on central for six days with no issues.

• String changes made/needed?: None
• Is Android affected?: yes

Comment 9

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/387fb16ba99b
https://hg.mozilla.org/releases/mozilla-beta/rev/fe8f75175619