stylo: refcount ServoStyleContext::mPresContext

RESOLVED FIXED in Firefox 56

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bholley, Assigned: manishearth)

Tracking

(Blocks 1 bug, {sec-low})

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox54 unaffected, firefox55 unaffected, firefox56 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

2 years ago
Right now we're storing a weak prescontext pointer on ServoStyleContext. This is a problem since style contexts can outlive the prescontext via getComputedStyle. This works fine for Gecko, since the computed style object holds an ArenaRefPtr to the style context, which is nulled out when the arena is destroyed, which happens before the prescontext goes away, which prevents style contects from outliving the prescontext. But in the Servo case we just have normal refcounting, so style contexts can live arbitrarily long.

This is a problem because the style context destructor does this:

http://searchfox.org/mozilla-central/rev/b7e531410ebc1c7d74a78e86a894e69608db318c/layout/style/nsStyleContext.cpp#166

So if the PresContext is gone, we're invoking a method on a freed |this|, which is bad.

Currently ServoStyleContexts are only minted on the main thread, but after bug 1367904 they'll be created in parallel. So we'll need to use atomic refcounting, but we should be able to get away with using atomics from Rust code only. This will avoid unnecessary overhead for all the other consumers of nsPresContext.
Reporter

Comment 1

2 years ago
Manish, given that this would bitrot bug 1367904, we should coordinate the fix. Do you want to work the fix into your patches in that bug, or should we land it on top?
Flags: needinfo?(manishearth)
I think there are also cases where StyleSet() is null when that dtor is called. Unsure what we're supposed to do there. My code currently bypasses all this; the destructor was moved completely into Gecko.
Reporter

Comment 3

2 years ago
(In reply to Manish Goregaokar [:manishearth] from comment #2)
> My code currently bypasses
> all this; the destructor was moved completely into Gecko.

Sure, but my point is that we want to make mPresContext a strong pointer, which interacts with your patches.
(That comment was typed before I got your next comment, and I missed posting this one)

So the root style context count machinery isn't necessary for Servo AFAICT. The only time mRootStyleContextCount gets read is in nsPresContext::HasCachedStyleData(), and it returns early in the servo case.


The added/removed calls do nothing in the servo case too.

But in general we shouldn't be holding onto a pointer that may be invalidated, so yeah, we should make this change. I would prefer it to be on top of my patches.
Flags: needinfo?(manishearth)
Reporter

Updated

2 years ago
Assignee: nobody → manishearth
Group: core-security → layout-core-security
Keywords: sec-high
Won't holding to the prescontext hold on too much data? Can we find a way to clear the DOM style context pointer instead of keeping the whole thing alive?

Alternatively, we could consider moving the prescontext pointer into the frame.
Reporter

Comment 6

2 years ago
(In reply to Emilio Cobos Álvarez [:emilio] from comment #5)
> Won't holding to the prescontext hold on too much data?

I'm not super worried about it. Holding a style context alive past presshell destruction _can_ happen, but I don't think it's particularly common or easy to do by accident - so we just need to make it safe.

> Can we find a way to
> clear the DOM style context pointer instead of keeping the whole thing alive?

That would require either registering every style context with something (which would be slow, complex, and difficult to do from servo), or using a weak pointer, which would impose an unacceptable overhead during layout when we need the prescontext.

> 
> Alternatively, we could consider moving the prescontext pointer into the
> frame.

This could work, but would be a big change, so I'm not itching to do it unless we have a clear reason to.

The change I'm proposing in this bug is basically a no-op (modulo the refcounting traffic), and the only behavior change comes in the situations where we currently risk UAF.
This is only an issue for style contexts coming from computed style, right?

So really, we would only really need to register the computed style stylecontexts, not all of them, and we wouldn't have to do it from servo or background threads or anything...  That's what Gecko does, really, with its ArenaRefPtr thing (the thing that gets registered is the ArenaRefPtr, not the style context; we could also simply register the computed style object somewhere).
Reporter

Comment 8

2 years ago
(In reply to Boris Zbarsky [:bz] from comment #7)
> This is only an issue for style contexts coming from computed style, right?

That I'm aware of. But in general, given that the objects holding the raw prescontext pointer are not arena-managed, it seems dicey.
 
> So really, we would only really need to register the computed style
> stylecontexts, not all of them, and we wouldn't have to do it from servo or
> background threads or anything...  That's what Gecko does, really, with its
> ArenaRefPtr thing (the thing that gets registered is the ArenaRefPtr, not
> the style context; we could also simply register the computed style object
> somewhere).

And then have it just clear the strong ref to the style context? Seems like that could work, though it does make me a bit nervous per above.
> But in general, given that the objects holding the raw prescontext pointer are not arena-managed, it seems dicey

Hmm.  I guess the failure mode with Gecko would be a deref of the style context pointer that it thinks it owns but is actually dead.  But the failure mode with stylo would be that this would succeed but then the prescontext pointer is dead...  I suppose the latter could be worse because it's got virtual functions and whatnot?

> And then have it just clear the strong ref to the style context?

Right.
Reporter

Comment 10

2 years ago
So as discussed using ArenaRefPtr would basically solve the problem here. However, we had to disable ArenaRefPtr for ServoStyleContext because the current implementation is leading to invalid casts (see bug 1367904 comment 224). We should revisit that in this bug.
This should do the ArenaRefPtr special casing.

I guess after this I just need to make mPresContext a strong pointer?
Attachment #8887715 - Flags: review?(bobbyholley)
Reporter

Comment 12

2 years ago
Comment on attachment 8887715 [details] [diff] [review]
Bug 1379830 - stylo: Allow ServoStyleContext to participate in ArenaRefPtr

Review of attachment 8887715 [details] [diff] [review]:
-----------------------------------------------------------------

At this point the whole PRES_ARENA_OBJECT_WITH_ARENAREFPTR_SUPPORT business is unused, right? Can we kill it?

> I guess after this I just need to make mPresContext a strong pointer?

No, the strong pointer and this patch are mutually exclusive.
Attachment #8887715 - Flags: review?(bobbyholley)
> At this point the whole PRES_ARENA_OBJECT_WITH_ARENAREFPTR_SUPPORT business is unused, right? Can we kill it?


Still need it to opt out of this switch, right?
Ah, I think I can simplify a lot of this code
Attachment #8887715 - Attachment is obsolete: true
Attachment #8888121 - Flags: review?(bobbyholley)
Reporter

Updated

2 years ago
Attachment #8888121 - Attachment is patch: true
Reporter

Updated

2 years ago
Attachment #8888121 - Flags: review?(bobbyholley) → review+
Reporter

Comment 16

2 years ago
We're pretty sure this is not exploitable on trunk, and is just a belt-and-suspenders fix.
Keywords: sec-highsec-low
https://hg.mozilla.org/mozilla-central/rev/31ae02774ca9
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Group: layout-core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.