851416 - Use-after-free with SVG and privacy.sanitize.sanitizeOnShutdown

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase (see comment 0)

1. Create a new profile
2. Set:
     user_pref("privacy.sanitize.sanitizeOnShutdown", true);

3. Download the testcase as q4-a.html
4. Fix the path in the testcase.

5. Run firefox -profile ~/pxa/ q4-a.html
6. Quit Firefox

Result:
Crash [@ nsNodeUtils::ParentChainChanged] or 
Crash [@ nsNodeUtils::LastRelease]

Comment 1

[Jesse Ruderman]

Attachment: stack (gdb)

Comment 2

[Jesse Ruderman]

Attachment: stack (ASan)

Comment 3

[Robert Longson [:longsonr]]

Might be webidl changes. If this is a regression then a regression range would likely be useful.

Comment 4

[Jesse Ruderman]

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/1cf12e699dc7
user:        Seth Fowler
date:        Thu Feb 28 12:02:31 2013 -0800
summary:     Bug 846028 - Coalesce invalidations in VectorImage. r=dholbert

Comment 5

[Seth Fowler [:seth] [:s2h]]

I'll take a look at this.

Comment 6

[Seth Fowler [:seth] [:s2h]]

So bug 846028 causes SVGRootRenderingObserver to stay out of the rendering observer list for the SVG root element it's watching from the time when some initial invalidation happens until the time when VectorImage::Draw is called. Unfortunately this test case shows that this can cause it to miss important messages - in particular it misses a call to nsSVGRenderingObserver::NotifyEvictedFromRenderingObserverList, which (logically enough) is only called if you're in the rendering observer list, but also has the effect of removing you from the _mutation_ observer list. SVGrootRenderingObserver thus isn't removed from the mutation observer list at the right time in certain circumstances, leading to the crash.

Perhaps this behavior should not cause a crash, and if not we can fix the API design in a separate bug, but for this bug I think reimplementing bug 846028 in a way that's safer with the current API is the right way to go. Patch shortly.

Comment 7

[Seth Fowler [:seth] [:s2h]]

Attachment: Ensure SVGRootRenderingObserver always gets added back to the rendering observer list.

Here's the fix. We ensure that SVGRootRenderingObserver always gets added back to the rendering observer list, one way or another. If we send an invalidation, VectorImage::Draw will take care of it as before. The new thing is that if we _don't_ send an invalidation, we go ahead and add ourselves back right there in SVGRootRenderingObserver::DoUpdate. With this patch applied I can't reproduce the testcase crash anymore.

Comment 8

[Seth Fowler [:seth] [:s2h]]

Try job here: https://tbpl.mozilla.org/?tree=Try&rev=4b54557545a7

Comment 9

[Seth Fowler [:seth] [:s2h]]

Thanks for the super fast review, Daniel!

Fortunately, this doesn't affect anything but nightly AFAICT. Will push in if things look good on try.

Comment 10

[Mats Palmgren (inactive)]

>      user_pref("privacy.sanitize.sanitizeOnShutdown", true);

Looking at the patch, I'm guessing this bug can occur also without that pref?
Just that it make crashing more reliable?

> Crash [@ nsNodeUtils::ParentChainChanged] or 
> Crash [@ nsNodeUtils::LastRelease]

I'll assume the worst and flag as csec-uaf unless someone thinks otherwise.

Comment 11

[Mats Palmgren (inactive)]

Seth, it looks like this is ready to land; or did you find some problem?

Comment 12

[Seth Fowler [:seth] [:s2h]]

Mats, yeah, I think it's pretty likely that this bug could occur without the pref. And indeed, try looks OK and it's ready to land. I'll push it in now.

Comment 13

[Seth Fowler [:seth] [:s2h]]

Sorry it took so long to actually push; inbound was closed earlier.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e152c2763cad

Comment 14

[Seth Fowler [:seth] [:s2h]]

Hmm, and I reset that flag. I don't understand why that happens. It's not like this was a mid-air collision.... Sorry, but I don't appear to have the permissions to fix it.

Comment 15

[Robert Kaiser]

All us mere mortals can do is request it, and then those with god-like powers can +/- it. ;-)

Comment 16

[bhavana bajaj [:bajaj]]

(In reply to Robert Kaiser (:[e-mail]) from comment #15)
> All us mere mortals can do is request it, and then those with god-like
> powers can +/- it. ;-)

I guess you meant to nominate for tracking-firefox22 here, so going ahead and setting the right flags here

Comment 17

[Robert Kaiser]

(In reply to bhavana bajaj [:bajaj] from comment #16)
> I guess you meant to nominate for tracking-firefox22 here, so going ahead
> and setting the right flags here

Gah, was late at night for me, sorry. Thanks for doing the right thing there. :)

Comment 18

[Ed Morley [:emorley]]

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

Comment 19

[Scoobidiver (away)]

https://hg.mozilla.org/releases/mozilla-beta/rev/7a6eb3c298e2