The default bug view has changed. See this FAQ.
Bug 792857 (CVE-2012-5836)

SVG text on path + setting a style crashes Firefox

VERIFIED FIXED

Status

()

Core
SVG
--
critical
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: slashrslashn, Assigned: jwatt)

Tracking

({crash, sec-critical, testcase})

15 Branch
crash, sec-critical, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox15 wontfix, firefox16 wontfix, firefox17+ verified, firefox18+ verified, firefox19+ verified, firefox-esr1017+ unaffected, firefox-esr17 verified)

Details

(Whiteboard: [adv-track-main17+][adv-track-esr17+][fixed by bug 786740], crash signature)

Attachments

(1 attachment, 1 obsolete attachment)

584 bytes, text/html
Details
(Reporter)

Description

5 years ago
Created attachment 663025 [details]
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.

Updated

5 years ago
Attachment #663025 - Attachment mime type: text/plain → text/html

Comment 1

5 years ago
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

Comment 2

5 years ago
dies calling FrameNeedsReflow from nsSVGUtils::ScheduleReflowSVG

Comment 3

5 years ago
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

Updated

5 years ago
Assignee: nobody → longsonr

Comment 5

5 years ago
Created attachment 663477 [details] [diff] [review]
patch (obsolete?)
Attachment #663477 - Flags: review?(jwatt)
Duplicate of this bug: 793658
(Assignee)

Comment 7

5 years ago
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.
(Assignee)

Comment 8

5 years ago
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.

Updated

5 years ago
Attachment #663477 - Flags: review?(jwatt)

Updated

5 years ago
Assignee: longsonr → nobody
(Assignee)

Updated

5 years ago
status-firefox15: --- → affected
status-firefox16: --- → affected
status-firefox17: --- → affected
status-firefox18: --- → affected
(Assignee)

Comment 11

5 years ago
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

Updated

5 years ago
Blocks: 795734

Updated

5 years ago
Blocks: 795740

Updated

5 years ago
Keywords: sec-critical
status-firefox15: affected → wontfix
Hi Johnathan, how is it going?
Attachment #663477 - Attachment description: patch → patch (obsolete?)
status-firefox16: affected → wontfix
status-firefox19: --- → affected
tracking-firefox17: --- → +
tracking-firefox18: --- → +
tracking-firefox19: --- → +
Blocks: 789719

Comment 13

5 years ago
This is a sec-critical crash and tracking+ for 17 - Jonathan or Mats, any ETA on a fix?
Flags: needinfo?(jwatt)
(Assignee)

Comment 14

5 years ago
I estimate by Monday.
Flags: needinfo?(jwatt)
(Assignee)

Comment 15

5 years ago
Likely to be another couple of days.

Comment 16

4 years ago
Patch for bug 779971 inadvertently fixes this too.

Updated

4 years ago
Attachment #663477 - Attachment is obsolete: true

Comment 17

4 years ago
(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

4 years ago
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.
(Assignee)

Comment 19

4 years ago
[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

Comment 22

4 years ago
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-firefox17: affected → fixed
status-firefox18: affected → fixed
status-firefox19: affected → fixed

Updated

4 years ago
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Is the ESR affected? If so, can we land bug 779971 on that branch as well?
tracking-firefox-esr10: --- → 17+

Comment 25

4 years ago
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.

Comment 27

4 years ago
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.)
status-firefox-esr10: --- → unaffected
(and "status-firefox17:fixed" implies "status-firefox-esr17:fixed" -- setting that as well)
status-firefox-esr17: --- → fixed
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/

Comment 37

4 years ago
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)
(Assignee)

Updated

4 years ago
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.
status-firefox17: fixed → verified
status-firefox18: fixed → verified
status-firefox19: fixed → verified
status-firefox-esr17: fixed → verified
Keywords: verifyme
Group: core-security
You need to log in before you can comment on or make changes to this bug.