Closed
Bug 420697
Opened 17 years ago
Closed 16 years ago
Crash [@ nsAttributeTextNode::~nsAttributeTextNode()] with text and stroke-dasharray and stroke-dashoffset
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
VERIFIED
FIXED
People
(Reporter: martijn.martijn, Assigned: dholbert)
References
Details
(4 keywords, Whiteboard: [sg:critical?] post 1.8-branch)
Crash Data
Attachments
(5 files)
129 bytes,
image/svg+xml
|
Details | |
8.64 KB,
text/plain
|
Details | |
2.09 KB,
patch
|
roc
:
review+
roc
:
superreview+
beltzner
:
approval1.9.1b2+
dveditz
:
approval1.9.0.6+
|
Details | Diff | Splinter Review |
102 bytes,
image/svg+xml
|
Details | |
1.10 KB,
patch
|
Details | Diff | Splinter Review |
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•17 years ago
|
Flags: blocking1.9?
Comment 1•17 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•16 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.
Comment 3•16 years ago
|
||
Who can own this?
Flags: wanted1.9.1?
Flags: wanted1.9.0.x?
Flags: wanted1.9.0.x+
Flags: blocking1.9.1?
Comment 4•16 years ago
|
||
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•16 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•16 years ago
|
||
Assignee | ||
Comment 7•16 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•16 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•16 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•16 years ago
|
||
(FWIW, the nsSVGGeometryFrame in play here is using the subclass "nsSVGGlyphFrame")
Assignee | ||
Comment 11•16 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•16 years ago
|
||
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•16 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•16 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•16 years ago
|
||
(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•16 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•16 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•16 years ago
|
||
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•16 years ago
|
Status: NEW → ASSIGNED
Comment 17•16 years ago
|
||
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 18•16 years ago
|
||
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 | ||
Comment 19•16 years ago
|
||
Pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/rev/26eff4ebdfbf
Assignee | ||
Updated•16 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 20•16 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 21•16 years ago
|
||
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•16 years ago
|
Keywords: fixed1.9.1
Assignee | ||
Comment 22•16 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
Comment 23•16 years ago
|
||
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•16 years ago
|
Flags: wanted1.8.0.x-
Comment 24•16 years ago
|
||
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
Updated•16 years ago
|
Group: core-security
Whiteboard: [sg:critical?] → [sg:critical?] post 1.8-branch
Comment 25•16 years ago
|
||
crash test added
http://hg.mozilla.org/mozilla-central/rev/4853476978fb
Flags: in-testsuite+
Updated•14 years ago
|
Crash Signature: [@ nsAttributeTextNode::~nsAttributeTextNode()]
You need to log in
before you can comment on or make changes to this bug.
Description
•