Crash [@ nsAttributeTextNode::~nsAttributeTextNode()] with text and stroke-dasharray and stroke-dashoffset

VERIFIED FIXED

Status

()

Core
SVG
--
critical
VERIFIED FIXED
10 years ago
7 years ago

People

(Reporter: Martijn Wargers (zombie), Assigned: dholbert)

Tracking

(4 keywords)

Trunk
crash, testcase, verified1.9.0.6, verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
blocking1.9 -
wanted1.9.0.x +
wanted1.8.1.x -
wanted1.8.0.x -
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?] post 1.8-branch, crash signature)

Attachments

(5 attachments)

(Reporter)

Description

10 years ago
Created attachment 307023 [details]
testcase 1: crashes in GetStrokeDashoffset (pre-patch)

See testcase, which crashes in current trunk build.

http://crash-stats.mozilla.com/report/index/c0f94933-e920-11dc-93d3-001a4bd43e5c
0  	nsAttributeTextNode::~nsAttributeTextNode()  	 mozilla/content/base/src/nsTextNode.cpp:103
1 	xul.dll@0x7e3c07 	
2 	nsSVGLength::MaybeGetCtxRect() 	mozilla/content/svg/content/src/nsSVGLength.cpp:597
3 	nsSVGLength::MaybeAddAsObserver() 	mozilla/content/svg/content/src/nsSVGLength.cpp:608
4 	nsSVGLength::SetContext(nsIWeakReference*, unsigned char) 	mozilla/content/svg/content/src/nsSVGLength.cpp:515
5 	nsSVGUtils::CoordToFloat(nsPresContext*, nsSVGElement*, nsStyleCoord const&) 	mozilla/layout/svg/base/src/nsSVGUtils.cpp:668
6 	nsSVGGeometryFrame::GetStrokeDashoffset() 	mozilla/layout/svg/base/src/nsSVGGeometryFrame.cpp:238
7 	nsSVGGeometryFrame::SetupCairoStroke(gfxContext*) 	mozilla/layout/svg/base/src/nsSVGGeometryFrame.cpp:421
(Reporter)

Updated

10 years ago
Flags: blocking1.9?

Comment 1

10 years ago
This would be great to pickup in a dot release since the testcase is so simple - but we shouldn't block FF3 final on this..
Flags: wanted1.9.0.x?
Flags: blocking1.9?
Flags: blocking1.9-
(Reporter)

Comment 2

10 years ago
It still crashes in current trunk build.

An interesting thing is that this was fixed somehow during one of the .0.x releases of Firefox 2.
It does crash with a 2006-09-11 branch build, but not anymore with a 2006+-09-12 branch build:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=MOZILLA_1_8_BRANCH&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-09-11+04&maxdate=2006-09-12+09&cvsroot=%2Fcvsroot
I bet it was fixed on branch by bug 351501, somehow.
Who can own this?
Flags: wanted1.9.1?
Flags: wanted1.9.0.x?
Flags: wanted1.9.0.x+
Flags: blocking1.9.1?
On a WinXP build of 1.9.0.4pre this makes a virtual function call into nowhere land and gets slapped down by DEP (if you've turned it on). Definitely not a null dereference.
Flags: wanted1.8.1.x-
Whiteboard: [sg:critical?]
(Assignee)

Comment 5

10 years ago
Crashes on my up-to-date Linux mozilla-central debug build, too.
Platform --> All/All
OS: Windows XP → All
Hardware: PC → All
(Assignee)

Comment 6

10 years ago
Created attachment 347363 [details]
backtrace of crash on Linux
(Assignee)

Comment 7

10 years ago
Ok, so one issue is inside of GetStrokeDashoffset(), at stacklevel 7 in the backtrace (attachment 347363 [details]):
  142 nsSVGGeometryFrame::GetStrokeDashoffset()
  143 {
  144   return
  145     nsSVGUtils::CoordToFloat(PresContext(),
  146                              static_cast<nsSVGElement*>(mContent),
(mxr link: http://tinyurl.com/587neq )

Here, "mContent" is a nsTextNode -- NOT a nsSVGElement.  So, that static_cast is bogus & dangerous.
(Assignee)

Comment 8

10 years ago
... and then there's yet another bogus static_cast, at stacklevel 3, in nsSVGLength::MaybeGetCtxRect():

600 already_AddRefed<nsIDOMSVGRect> nsSVGLength::MaybeGetCtxRect()
601 {
602   if ((mSpecifiedUnitType == SVG_LENGTHTYPE_PERCENTAGE) && mElement) {
603     nsCOMPtr<nsIContent> element = do_QueryReferent(mElement);
604     if (element) {
605       nsSVGSVGElement *ctx =
606         static_cast<nsSVGElement*>(element.get())->GetCtx();
(mxr link: http://tinyurl.com/6dgbzj )

At line 606, "element" is that same nsTextNode -- NOT a nsSVGElement.

So, the call to GetCtx() puts us inside of nsSVGElement::GetCtx(), with "this" pointing to a non-nsSVGElement.  And we lose.
(Assignee)

Comment 9

10 years ago
So I'm not too familiar with this code, but based on comment 7, I'm guessing the issue is one of two things.

Either: 
   1. nsSVGGeometryFrame::GetStrokeDashoffset (and children) needs to be changed to deal with mContent being a non-nsSVGElement
...or...
   2. nsSVGGeometryFrame should never have mContent = a non-nsSVGElement in the first place.  (i.e. for this testcase, it should be given a different content node.)
(Assignee)

Comment 10

10 years ago
(FWIW, the nsSVGGeometryFrame in play here is using the subclass "nsSVGGlyphFrame")
(Assignee)

Comment 11

10 years ago
Bug 353172 added the static_casts in the first place (using NS_STATIC_CAST at the time) -- adding dependency.

I noticed in that bug's patch that we check specifically for glyph frames in GetStrokeWidth(), and in those cases, we use mContent->GetParent() instead of mContent.  The current version of that check is here:

86 nsSVGGeometryFrame::GetStrokeWidth()
87 {
88   nsSVGElement *ctx = static_cast<nsSVGElement*>
89                                  (GetType() == nsGkAtoms::svgGlyphFrame ?
90                                      mContent->GetParent() : mContent);
91 
92   return
93     nsSVGUtils::CoordToFloat(PresContext(),
94                              ctx,
(mxr: http://tinyurl.com/67mhcj )

I think nsSVGGeometryFrame::GetStrokeDashoffset() just needs that same check -- if I add that, the crash goes away on this bug's testcase.  Patch coming in a sec...
Assignee: nobody → dholbert
Blocks: 353172
(Assignee)

Comment 12

10 years ago
Created attachment 347384 [details] [diff] [review]
patch v1: Add check to GetStrokeDashoffset and GetStrokeDashArray, making them like GetStrokeWidth

This patch adds the check from GetStrokeWidth to both GetStrokeDashoffset and GetStrokeDashArray.

This means that, for svgGlyphFrames, we'll use mContent's *parent* as a our nsSVGElement in both of those functions (rather than using mContent itself, which is a nsTextNode).

The change in GetStrokeDashoffset fixes the crash.

The change in GetStrokeDashArray doesn't actually change the behavior of this testcase, but it does make us safer, by fixing a bogus static_cast.

In this bug's testcase, we DO hit that bogus cast in GetStrokeDashArray, but in this case the casted pointer is ignored, because CoordToFloat just falls into the simple 'eStyleUnit_Factor' clause.

If anyone can come up with a testcase that crashes without this patch's  GetStrokeDashArray chunk, it'd be much appereciated... I'll look into it a bit myself.
(Assignee)

Comment 13

10 years ago
(In reply to comment #11)
> I noticed in that bug's patch that we check specifically for glyph frames in
> GetStrokeWidth(), and in those cases, we use mContent->GetParent() instead of
> mContent.

FWIW: That check was initially added in bug 344892, to fix almost the exact same crash as this one here. However, that change *only* fixed GetStrokeWidth -- not the analogous sections of GetStrokeDashoffset or GetStrokeDashArray.
(Assignee)

Updated

10 years ago
Attachment #347384 - Flags: superreview?(roc)
Attachment #347384 - Flags: review?(roc)
Attachment #347384 - Flags: superreview?(roc)
Attachment #347384 - Flags: superreview+
Attachment #347384 - Flags: review?(roc)
Attachment #347384 - Flags: review+
(Assignee)

Comment 14

10 years ago
Created attachment 347386 [details]
testcase 2: crashes in GetStrokeDashArray (pre-patch)

(In reply to comment #12)
> If anyone can come up with a testcase that crashes without this patch's 
> GetStrokeDashArray chunk, it'd be much appereciated... I'll look into it a bit
> myself.

That was easy - I think this testcase crashes in GetStrokeDashArray pre-patching.
(Assignee)

Updated

10 years ago
Attachment #307023 - Attachment description: testcase → testcase 1: crashes in GetStrokeDashoffset (pre-patch)
Flags: wanted1.9.1?
Flags: blocking1.9.1?
Flags: blocking1.9.1+
(Assignee)

Comment 15

10 years ago
Comment on attachment 347384 [details] [diff] [review]
patch v1: Add check to GetStrokeDashoffset and GetStrokeDashArray, making them like GetStrokeWidth

Requesting to land this patch for 1.9.1b2 and 1.9.0.5.

Rationales:
 - security fix for a scary bug (see comment 4) 
 - Simple patch
 - Patch is clearly what we intended to do (see comment 13)
Attachment #347384 - Flags: approval1.9.1b2?
Attachment #347384 - Flags: approval1.9.0.5?
(Assignee)

Comment 16

10 years ago
Created attachment 347398 [details] [diff] [review]
crashtests patch (to land when bug is opened up)

Here are two crashtests, based on testcases 1 and 2, to regression-test the 2 chunks added by this bug's patch. 

I've verified that they crash pre-patch and they don't crash post-patch.
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED
Comment on attachment 347384 [details] [diff] [review]
patch v1: Add check to GetStrokeDashoffset and GetStrokeDashArray, making them like GetStrokeWidth

a=beltzner
Attachment #347384 - Flags: approval1.9.1b2? → approval1.9.1b2+
Comment on attachment 347384 [details] [diff] [review]
patch v1: Add check to GetStrokeDashoffset and GetStrokeDashArray, making them like GetStrokeWidth

We'll take this one in the next release.
Attachment #347384 - Flags: approval1.9.0.5? → approval1.9.0.6?
(Assignee)

Updated

10 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Reporter)

Comment 20

10 years ago
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20081130 Minefield/3.1b3pre
Status: RESOLVED → VERIFIED
Comment on attachment 347384 [details] [diff] [review]
patch v1: Add check to GetStrokeDashoffset and GetStrokeDashArray, making them like GetStrokeWidth

Approved for 1.9.0.6, a=dveditz for release-drivers.
Attachment #347384 - Flags: approval1.9.0.6? → approval1.9.0.6+
(Assignee)

Updated

10 years ago
Keywords: fixed1.9.1
(Assignee)

Comment 22

10 years ago
Fixed on 1.9.0 branch:
Checking in layout/svg/base/src/nsSVGGeometryFrame.cpp;
/cvsroot/mozilla/layout/svg/base/src/nsSVGGeometryFrame.cpp,v  <--  nsSVGGeometryFrame.cpp
new revision: 1.31; previous revision: 1.30
done
Keywords: fixed1.9.0.6
Verified with 1.9.0.6 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.6pre) Gecko/2009010504 GranParadiso/3.0.6pre.
Keywords: fixed1.9.0.6 → verified1.9.0.6

Updated

10 years ago
Flags: wanted1.8.0.x-
Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US;
rv:1.9.1b3pre) Gecko/20090130 Shiretoko/3.1b3pre Ubiquity/0.1.5 and 
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre)
Gecko/20090130 Minefield/3.2a1pre
Keywords: fixed1.9.1 → verified1.9.1
Group: core-security
Whiteboard: [sg:critical?] → [sg:critical?] post 1.8-branch
Crash Signature: [@ nsAttributeTextNode::~nsAttributeTextNode()]
You need to log in before you can comment on or make changes to this bug.