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

VERIFIED FIXED

Status

()

defect
--
critical
VERIFIED FIXED
12 years ago
8 years ago

People

(Reporter: martijn.martijn, Assigned: dholbert)

Tracking

(Blocks 1 bug, 4 keywords)

Trunk
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)

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
Flags: blocking1.9?
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-
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?]
Crashes on my up-to-date Linux mozilla-central debug build, too.
Platform --> All/All
OS: Windows XP → All
Hardware: PC → All
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.
... 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.
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.)
(FWIW, the nsSVGGeometryFrame in play here is using the subclass "nsSVGGlyphFrame")
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
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.
(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.
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+
(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.
Attachment #307023 - Attachment description: testcase → testcase 1: crashes in GetStrokeDashoffset (pre-patch)
Flags: wanted1.9.1?
Flags: blocking1.9.1?
Flags: blocking1.9.1+
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?
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.
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?
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
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+
Keywords: fixed1.9.1
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.
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
Group: core-security
Whiteboard: [sg:critical?] → [sg:critical?] post 1.8-branch
crash test added
http://hg.mozilla.org/mozilla-central/rev/4853476978fb
Flags: in-testsuite+
Crash Signature: [@ nsAttributeTextNode::~nsAttributeTextNode()]
You need to log in before you can comment on or make changes to this bug.