1652583 - Potential issue with WeakMap collection with uncollected value zone

Status: RESOLVED FIXED

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

Description

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

In looking at bug 1647747, I realized I'm not sure how this stuff works in some cross-zone cases. Specifically, consider the case where the value is in a different zone from the map. The value zone is being collected, but the map zone is not. If a WeakMap entry is the only thing keeping a value alive, then it seems like the value would not be marked and would end up getting swept, leaving a dangling pointer (and UAF).

In practice, it seems like we mostly avoid this: a regular WeakMap appears to normally be same-zone with its keys and values, and a DebuggerWeakMap is same-zone with its values. Except that a regular WeakMap might have a value in the atoms compartment, and I don't know why that couldn't result in the above scenario.

I attempted to construct a shell test that would trigger this, and it did not fail, so perhaps we have some kind of backstop that kicks in here. I haven't stepped through it yet to understand why.

Comment 1

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

Ah. There's a shape that holds an edge to my atom. I'll need to come up with a trickier way of getting something into the atoms zone.

Comment 2

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

A Symbol works better, but I still haven't managed to trigger a failure. Need to look into it more.

Comment 3

[Jon Coppeard (:jonco)]

It it's just the atoms zone, then the atom marking bitmap handles that. We mark atoms that are referenced by uncollected zones in AtomMarkingRuntime::markAtomsUsedByUncollectedZones.

Comment 4

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

Yeah, you're right. It still feels a little fragile, but I guess the per-Zone atom marking bitmap kind of serves the same purpose as the cross-compartment wrapper map in terms of being sort of a store buffer for incoming edges.

So this depends on values either being same-zone with the map, or being in the atoms zone. I have a patch that checks this, and it seems to hold.

Comment 5

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

Attachment: Bug 1652583 - Check that WeakMaps (and subclasses) are same-zone with their values

Comment 6

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

Filed bug 1652891 for the original problem I was worried about, which is what got me to thinking about this one.

Comment 7

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

Weakmaps are all same-zone with their values, which avoids this issue. I'll be landing the patch that asserts this.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/106aaee30016
Check that WeakMaps (and subclasses) are same-zone with their values r=jonco

Comment 9

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/106aaee30016