Closed Bug 1500064 Opened 2 years ago Closed 2 years ago

IdSet in enumeration code needs to be rooted

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox-esr60 --- wontfix
firefox62 --- wontfix
firefox63 --- wontfix
firefox64 --- fixed

People

(Reporter: jandem, Assigned: jandem)

Details

(Keywords: csectype-disclosure, sec-moderate, Whiteboard: [post-critsmash-triage][adv-main64+])

Attachments

(1 file)

This is a Maybe<IdSet> and IdSet is a HashSet<jsid, ...>

Because (1) we can call arbitrary code while this set is on the stack (via scripted proxy traps, so content can trigger a GC etc) and (2) not all ids in the set are guaranteed to exist elsewhere (the |props| Vector contains a subset), this seems to be a rooting hazard.

The simplest option is probably to make this a GCHashSet. A bit unfortunate for me because I wanted to make this an InlineSet but oh well. We could add GCInlineSet later maybe.
I'm not sure how s-s this is because we only use this set to check whether we've already seen a particular id. OTOH, it might be possible to leak info like "is any origin using atom x?" or something.
Priority: -- → P1
Note that bug 1321014 will fix the part about the hazard analysis not seeing through the Maybe<> to report this problem. I think it may require further annotation to get it to complain about the HashSet, though, since HashSet contains a table of type js::detail::HashTableEntry<const jsid>*. That won't be treated as a GC pointer type because (1) it's a pointer to a GC pointer, and (2) HashTableEntry contains untyped data (uint8[8] in this case). But I should be able to use MOZ_INHERIT_TYPE_ANNOTATIONS_FROM_TEMPLATE_ARGS or something like it to see all the way through to the hazard.

I filed bug 1500247.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
This changes Maybe<IdSet> to Rooted<IdSet>. I think the Maybe<>
is not buying us much because HashTable's storage is now lazily
allocated (bug 1481998). I didn't notice any regressions in
micro-benchmarks; this patch might be a small improvement actually.
https://hg.mozilla.org/mozilla-central/rev/7141a7da1020
Group: javascript-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Is this something we're going to want to backport to ESR60 during the next cycle for 60.4?
Flags: needinfo?(jdemooij)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #6)
> Is this something we're going to want to backport to ESR60 during the next
> cycle for 60.4?

Probably not because (1) I still think the sec-moderate rating is correct and (2) the patch depends on some HashSet changes that are not in ESR60 (bug 1481998) and so the ESR60 patch would either have to be rewritten or regress perf.
Flags: needinfo?(jdemooij)
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main64+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.