Closed Bug 792857 (CVE-2012-5836) Opened 12 years ago Closed 12 years ago

SVG text on path + setting a style crashes Firefox

Categories

(Core :: SVG, defect)

15 Branch
defect
Not set
critical

Tracking

()

VERIFIED FIXED
Tracking Status
firefox15 --- wontfix
firefox16 --- wontfix
firefox17 + verified
firefox18 + verified
firefox19 + verified
firefox-esr10 17+ unaffected
firefox-esr17 --- verified

People

(Reporter: slashrslashn, Assigned: jwatt)

References

Details

(Keywords: crash, sec-critical, testcase, Whiteboard: [adv-track-main17+][adv-track-esr17+][fixed by bug 786740])

Crash Data

Attachments

(1 file, 1 obsolete file)

Attached file 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.
Attachment #663025 - Attachment mime type: text/plain → text/html
Seems to die somewhere under ScheduleReflow crawling over dead frames.
Status: UNCONFIRMED → NEW
Component: Untriaged → SVG
Ever confirmed: true
OS: Linux → All
Product: Firefox → Core
Hardware: x86_64 → All
dies calling FrameNeedsReflow from nsSVGUtils::ScheduleReflowSVG
Here is a crash ID: bp-fa265555-0a24-4fa2-aec4-369492120920.
It has the same crash signature as bug 762494.
Severity: normal → critical
Crash Signature: [@ nsBlockInFlowLineIterator::nsBlockInFlowLineIterator(nsBlockFrame*, nsIFrame*, bool*)]
Keywords: crash, testcase
Marking as security-sensitive based on comment 1 (use-after-free?) and comment 3 (possibly dupe of or related to another security-sensitive bug).
Group: core-security
Assignee: nobody → longsonr
Attached patch patch (obsolete?) (obsolete) — — Splinter Review
Attachment #663477 - Flags: review?(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.
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?
> 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
(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.
Attachment #663477 - Flags: review?(jwatt)
Assignee: longsonr → nobody
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.
Assignee: nobody → jwatt
Blocks: 793658
Blocks: 795734
Blocks: 795740
Hi Johnathan, how is it going?
Attachment #663477 - Attachment description: patch → patch (obsolete?)
Blocks: 789719
This is a sec-critical crash and tracking+ for 17 - Jonathan or Mats, any ETA on a fix?
Flags: needinfo?(jwatt)
I estimate by Monday.
Flags: needinfo?(jwatt)
Likely to be another couple of days.
Patch for bug 779971 inadvertently fixes this too.
Attachment #663477 - Attachment is obsolete: true
(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?
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.
[We landed that patch (with the non-essential changes removed).]
Is this fixed on beta/aurora then?  Can we get verification?
Keywords: qawanted, verifyme
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
Based on comment 21, it's fixed in all branches but not by bug 779971.
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?
Keywords: qawanted, verifyme
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Is the ESR affected? If so, can we land bug 779971 on that branch as well?
Won't the ESR be cut from the beta branch?
ESR 10.0.10 does not appear to be affected.

Confirmed via bug file on Ubuntu 11.10.
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?
(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.)
(and "status-firefox17:fixed" implies "status-firefox-esr17:fixed" -- setting that as well)
Alias: CVE-2012-5836
Whiteboard: [adv-track-main17+]
Whiteboard: [adv-track-main17+] → [adv-track-main17+][adv-track-esr17+]
(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.
Flags: needinfo?(mwobensmith)
Keywords: qawanted
Whiteboard: [adv-track-main17+][adv-track-esr17+] → [adv-track-main17+][adv-track-esr17+] fix-range-wanted
Dan, for which branch do you need a fix range?
Flags: needinfo?(mwobensmith)
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)
Crash appears to have been fixed on nightly 2012-10-16. 
The previous day's build crashes. This is using Ubuntu 11.10.
Thanks, Matt! Could you grab the corresponding csets for those builds by viewing "about:buildconfig" (the "Built from ..." line there)
Of course, sorry.

2012-10-15 942ed5747b63 (crash)
2012-10-16 8f145599e4bf (no crash)
(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/
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.
I went through tinderbox builds:

Crash: 5f4a6a474455 (tinderbox build 1350324107)
Fixed: 8f145599e4bf (tinderbox build 1350374987)
Flags: in-testsuite?
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.
Bug 786740 fixed this, verified in local build with that patch backed out.
Status: RESOLVED → VERIFIED
Depends on: 786740
Keywords: qawanted
Whiteboard: [adv-track-main17+][adv-track-esr17+] fix-range-wanted → [adv-track-main17+][adv-track-esr17+][fixed by bug 786740]
Keywords: verifyme
No longer blocks: 795734
No longer blocks: 795740
Verified fixed on 17.0.2esr.

Setting all flags as verified, see comment 21.
Group: core-security
You need to log in before you can comment on or make changes to this bug.