792857 - (CVE-2012-5836) SVG text on path + setting a style crashes Firefox

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[slashrslashn]

Attachment: crashff.html

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:15.0) Gecko/20100101 Firefox/15.0.1
Build ID: 20120908010631

Steps to reproduce:

Start with SVG with text on a path. Make sure there is an earlier sibling in the svg's parent element. Set a parent container to have a relative position. Firefox crashes.

Pull up the attached html file and click "Crash Firefox" to reproduce it.


Actual results:

Firefox crashes. A cursory look in gdb indicates it's somewhere deep in the xul libraries.
Reproduced on Firefox 15.0.1 Linux and Mac and Firefox 16b3 Linux.




Expected results:

The style should be set correctly and Firefox does not crash.

Comment 1

[Robert Longson [:longsonr]]

Seems to die somewhere under ScheduleReflow crawling over dead frames.

Comment 2

[Robert Longson [:longsonr]]

dies calling FrameNeedsReflow from nsSVGUtils::ScheduleReflowSVG

Comment 3

[Scoobidiver (away)]

Here is a crash ID: bp-fa265555-0a24-4fa2-aec4-369492120920.
It has the same crash signature as bug 762494.

Comment 4

[Daniel Holbert [:dholbert]]

Marking as security-sensitive based on comment 1 (use-after-free?) and comment 3 (possibly dupe of or related to another security-sensitive bug).

Comment 5

[Robert Longson [:longsonr]]

Attachment: patch (obsolete?)

Comment 7

[Jonathan Watt [:jwatt]]

roc, it would be great to have your thoughts on how best to fix this issue.

longsonr: I have two reservations about this patch. The first is that observing frames are going to be left with the wrong bounds and overflow regions if we don't call InvalidateAndScheduleReflowSVG. The second is that, while this patch may prevent the crash at hand, it's not clear to me that we don't have other ways that we end up in code that pokes around the frame tree under the nsSVGEffects::InvalidateDirectRenderingObservers call in nsFrame::DestroyFrom. (Or that we won't accidentally introduce such code in future.)

I wonder if the best way to fix this is to delay the nsSVGEffects::InvalidateDirectRenderingObservers call in nsFrame::DestroyFrom, similar to the way that Mats' patch in bug 789719 delays the FrameNeedsReflow call. You could create an nsInvalidateDirectRenderingObserversRunable similar to nsReflowFrameRunnable and, if the frame has rendering observers, pass one of those containing the observer list to nsContentUtils::AddScriptRunner. Better still you could wrap that up in a helper function that makes sure we only ever have one such runable pending, and that we just maintain a hash of weak pointers to observer frames.

Comment 8

[Jonathan Watt [:jwatt]]

Probably a bit late at night for me to be thinking out loud, but here goes...

It's not ideal that we need to call ReflowSVG() on rendering observers in the first place. Ideally we'd only need to call UpdateOverflow() on them. (Although I'm not sure how much safer scheduling a restyle is to scheduling a reflow.)

There seem to be two places where rendering observers need the ReflowSVG. nsSVGPathGeometryFrame needs it because markers are currently included in the mRect, but I can't offhand see a reason why we need that to be the case (just including them in the visual overflow rect would seem to be fine). nsSVGTextFrame also needs a ReflowSVG because the offsets of the children frames' mRects depend on their textPath. That would seem to be harder to eliminate, and would be annoying to have to do given heycam's text rewrite.

Maybe for branches we can do some sort of script blocker thing, and for trunk we can investigate whether we can get rid of that by eliminating the need for rendering observer invalidation to include ReflowSVG() calls?

Comment 9

[Mats Palmgren (inactive)]

> Ideally we'd only need to call UpdateOverflow() on them.

... and on their ancestors as far as it results in a change, and then
you might bump in to a scroll frame where UpdateOverflow() can in the
worst case result in a reflow currently:
http://hg.mozilla.org/mozilla-central/annotate/483913c82d2d/layout/generic/nsGfxScrollFrame.cpp#l3733

Comment 10

[Mats Palmgren (inactive)]

(In reply to Jonathan Watt [:jwatt] from comment #7)
> I wonder if the best way to fix this is to delay the
> nsSVGEffects::InvalidateDirectRenderingObservers call in
> nsFrame::DestroyFrom

I agree, some kind of async approach is a more robust long term solution.

Comment 11

[Jonathan Watt [:jwatt]]

Thanks for having a crack at this, longsonr.

(In reply to Mats Palmgren [:mats] from comment #10)
> I agree, some kind of async approach is a more robust long term solution.

Okay, I'll have a go at implementing something along the lines of the fix described in the last paragraph of comment 7, unless you want to do it, Mats. Assigning to myself for now, but feel free to take.

Comment 12

[David Bolter [:davidb] (NeedInfo me for attention)]

Hi Johnathan, how is it going?

Comment 13

[Robert Kaiser]

This is a sec-critical crash and tracking+ for 17 - Jonathan or Mats, any ETA on a fix?

Comment 14

[Jonathan Watt [:jwatt]]

I estimate by Monday.

Comment 15

[Jonathan Watt [:jwatt]]

Likely to be another couple of days.

Comment 16

[Robert Longson [:longsonr]]

Patch for bug 779971 inadvertently fixes this too.

Comment 17

[Robert Kaiser]

(In reply to Robert Longson from comment #16)
> Patch for bug 779971 inadvertently fixes this too.

The bug here is marked sec-critical and tracking+ for 17, are we looking to land the patch from over there still for the current beta cycle?

Comment 18

[Robert Longson [:longsonr]]

Jonathan, what do you want to do with my patch in bug 779971. Presumably you don't want to land it as is otherwise you wouldn't have the cosmetic parts of it.

Comment 19

[Jonathan Watt [:jwatt]]

[We landed that patch (with the non-essential changes removed).]

Comment 20

[Lukas Blakk [:lsblakk] use ?needinfo]

Is this fixed on beta/aurora then?  Can we get verification?

Comment 21

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed crash on build 2012-9-20, nightly
Verified fixed on build 2012-11-6, nightly
Verified fixed on build 2012-11-6, Aurora 18
Verified fixed on build 2012-11-6, Beta 17
Ubuntu Linux 11.10

Comment 22

[Scoobidiver (away)]

Based on comment 21, it's fixed in all branches but not by bug 779971.

Comment 23

[u279076]

Removing qawanted and verifyme as per comment 21. Do we need to figure out when/why this was fixed or is it sufficient to mark this bug resolved?

Comment 24

[Alex Keybl [:akeybl]]

Is the ESR affected? If so, can we land bug 779971 on that branch as well?

Comment 25

[Robert Longson [:longsonr]]

Won't the ESR be cut from the beta branch?

Comment 26

[Matt Wobensmith [:mwobensmith][:matt:]]

ESR 10.0.10 does not appear to be affected.

Confirmed via bug file on Ubuntu 11.10.

Comment 27

[Robert Longson [:longsonr]]

Which ESR are you talking about Alex? ESR10 is unaffected. ESR17 comes from beta doesn't it and should be fixed by the beta landing shouldn't it?

Comment 28

[Daniel Holbert [:dholbert]]

(I think he was asking about ESR10, which IIRC we'll continue to support for a bit after ESR17 is released.  So -- Comment 26 & 27 answered his question. Setting status-firefox-esr10 to "unaffected" based on those comments.)

Comment 29

[Daniel Holbert [:dholbert]]

(and "status-firefox17:fixed" implies "status-firefox-esr17:fixed" -- setting that as well)

Comment 30

[Daniel Veditz [:dveditz]]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #23)
> Removing qawanted and verifyme as per comment 21. Do we need to figure out
> when/why this was fixed or is it sufficient to mark this bug resolved?

It's sufficient to resolve the bug, but we still need to know what fixed it to know whether it's merely masked and might come back or whether it's really dead.

Comment 31

[Matt Wobensmith [:mwobensmith][:matt:]]

Dan, for which branch do you need a fix range?

Comment 32

[Daniel Holbert [:dholbert]]

I'm not the Dan you were directing the question to, but I think any of {nightly, aurora, beta} would do.

In each of those branches, bug 779971 landed after the build that you mentioned in comment 21, so it definitely wasn't what fixed it. (and we want to find out what _did_ fix it)

Comment 33

[Matt Wobensmith [:mwobensmith][:matt:]]

Crash appears to have been fixed on nightly 2012-10-16. 
The previous day's build crashes. This is using Ubuntu 11.10.

Comment 34

[Daniel Holbert [:dholbert]]

Thanks, Matt! Could you grab the corresponding csets for those builds by viewing "about:buildconfig" (the "Built from ..." line there)

Comment 35

[Matt Wobensmith [:mwobensmith][:matt:]]

Of course, sorry.

2012-10-15 942ed5747b63 (crash)
2012-10-16 8f145599e4bf (no crash)

Comment 36

[u279076]

(In reply to Matt Wobensmith from comment #35)
> Of course, sorry.
> 
> 2012-10-15 942ed5747b63 (crash)
> 2012-10-16 8f145599e4bf (no crash)

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=942ed5747b63&tochange=8f145599e4bf

This contains a 143 change PGO merge. Can you narrow this down further using the tinderbox builds?

ftp://ftp.mozilla.org/pub/firefox/tinderbox-builds/mozilla-central-linux64/

Comment 37

[Robert Longson [:longsonr]]

It will be http://hg.mozilla.org/mozilla-central/rev/9edabc0ddc99 this is the one that allows us to traverse a frame tree during frame destruction.

bug 779971 stops us traversing the frame tree during frame destruction with textPaths.

Having either bug 779971 or bug 786740 would fix this issue and 17, 18, trunk all have both now.

Comment 38

[Matt Wobensmith [:mwobensmith][:matt:]]

I went through tinderbox builds:

Crash: 5f4a6a474455 (tinderbox build 1350324107)
Fixed: 8f145599e4bf (tinderbox build 1350374987)

Comment 39

[u279076]

Thanks Matt, that still includes the full PGO merge. I don't think we can realistically narrow this down any further. I think we proceed based on Robert's comment 37.

dveditz, does this satisfy your qawanted request? If not, please advise how we can break this down further.

Comment 40

[Mats Palmgren (inactive)]

Bug 786740 fixed this, verified in local build with that patch backed out.

Comment 41

[Matt Wobensmith [:mwobensmith][:matt:]]

Verified fixed on 17.0.2esr.

Setting all flags as verified, see comment 21.