Last Comment Bug 590291 - Crash [@ nsSVGGlyphFrame::GetExtentOfChar] with getExtentOfChar(0)
: Crash [@ nsSVGGlyphFrame::GetExtentOfChar] with getExtentOfChar(0)
Status: RESOLVED FIXED
[sg:critical?]
: crash, regression, testcase, verified1.9.2
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- critical (vote)
: ---
Assigned To: Robert Longson
:
Mentors:
Depends on:
Blocks: crossfuzz 478792
  Show dependency treegraph
 
Reported: 2010-08-24 13:44 PDT by Martijn Wargers [:mwargers] (not working for Mozilla)
Modified: 2011-06-13 10:01 PDT (History)
14 users (show)
jruderman: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
final+
needed
.11-fixed
unaffected


Attachments
testcase (243 bytes, image/svg+xml)
2010-08-24 13:44 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
patch (1.17 KB, patch)
2010-08-31 19:52 PDT, Robert Longson
dholbert: review+
dveditz: approval1.9.2.11+
Details | Diff | Review

Description Martijn Wargers [:mwargers] (not working for Mozilla) 2010-08-24 13:44:40 PDT
Created attachment 468796 [details]
testcase

See testcase, which crashes current trunk build and Firefox3.6.8.

http://crash-stats.mozilla.com/report/index/bp-c88327df-4ec8-4970-8c25-fe09d2100824
0  	xul.dll  	nsSVGGlyphFrame::GetExtentOfChar  	 layout/svg/base/src/nsSVGGlyphFrame.cpp:1077
1 	xul.dll 	nsSVGTextContainerFrame::GetExtentOfChar 	layout/svg/base/src/nsSVGTextContainerFrame.cpp:195
2 	xul.dll 	nsSVGTextElement::GetExtentOfChar 	content/svg/content/src/nsSVGTextElement.cpp:324
3 	xul.dll 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:102
4 	xul.dll 	js::InvokeCommon<int > 	js/src/jsinterp.cpp:566
5 	xul.dll 	js::Invoke 	js/src/jsinterp.cpp:699
6 	xul.dll 	js::Interpret 	js/src/jsinterp.cpp:4709
7 	xul.dll 	js::Execute 	js/src/jsinterp.cpp:883
8 	xul.dll 	JS_EvaluateUCScriptForPrincipals 	js/src/jsapi.cpp:4769
9 	xul.dll 	nsJSContext::EvaluateString 	dom/base/nsJSEnvironment.cpp:1811
10 	xul.dll 	nsScriptLoader::EvaluateScript 	content/base/src/nsScriptLoader.cpp:767
11 	xul.dll 	nsScriptLoader::ProcessRequest 	content/base/src/nsScriptLoader.cpp:677
12 	xul.dll 	nsScriptLoader::ProcessScriptElement 	content/base/src/nsScriptLoader.cpp:617
13 	xul.dll 	nsScriptElement::MaybeProcessScript 	content/base/src/nsScriptElement.cpp:195
14 	xul.dll 	nsSVGScriptElement::DoneAddingChildren 	content/svg/content/src/nsSVGScriptElement.cpp:280
15 	xul.dll 	nsXMLContentSink::CloseElement 	
etc..
Comment 1 Marcia Knous [:marcia - use ni] 2010-08-24 13:54:40 PDT
Crashes on Mac as well, changing to all.
Comment 2 Johnny Stenback (:jst, jst@mozilla.com) 2010-08-31 13:54:49 PDT
Jwatt, any updates here? Marking sg:critical? until we learn more about the reason for this crash.
Comment 4 Daniel Veditz [:dveditz] 2010-08-31 17:06:17 PDT
I think this is an sg:dos null deref. The crash-stacks on windows point at an uninteresting line 1077, but I think the optimizer is playing games with the loops. In mac debug and opt builds build I crash just after calling GetLength() on a null mTextRun. I don't understand why I don't crash calling mTextRun->IsClusterStart(limit) first though.

In the debug build it ran quite a long time before crashing. Is it possible we're reusing a dead object that just happens to have nulls in an opt build?
Comment 5 Daniel Veditz [:dveditz] 2010-08-31 17:40:14 PDT
Doesn't crash 1.9.0 or 1.9.1. The code that's crashing was added in bug 478792
http://hg.mozilla.org/mozilla-central/rev/f6cdd2d6a9ea
Comment 6 Robert Longson 2010-08-31 19:52:18 PDT
Created attachment 471012 [details] [diff] [review]
patch

the CharacterIterator tries to create the mTextRun so that needs to come first. In this case even that does not set a mTextRun since we go into an error state with mInError = true. Chicken and egg at first sight since you need to advance to detect the error and you need a text run to figure out where to advance to but you can just advance to the beginning of the string and then figure out whether you need to advance any more later.
Comment 7 Robert Longson 2010-09-01 02:42:35 PDT
Is that it for me in the brave new world of security fixes being checked in by the security group? The patch should apply to both trunk and 1.9.2 though you may need to fuzz things a little for the branch.
Comment 8 Daniel Veditz [:dveditz] 2010-09-07 22:11:02 PDT
Comment on attachment 471012 [details] [diff] [review]
patch

Approved for 1.9.2.10, a=dveditz

Please land this fix on trunk and the 1.9.2 branch.
Comment 9 Jesse Ruderman 2010-09-07 22:13:47 PDT
(We're not really using the private repositories yet.)
Comment 11 [On PTO until 6/29] 2010-09-21 17:11:05 PDT
Verified fixed for 1.9.2 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2.11pre) Gecko/20100921 Namoroka/3.6.11pre. Verified crash with testcase in 1.9.2.10.
Comment 12 Michal Zalewski 2010-10-19 23:28:06 PDT
I see that this shipped, but without any credit for cross_fuzz? Bummer :-(
Comment 13 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-10-20 01:00:12 PDT
I think Martijn, the original reporter, wasn't using cross_fuzz.
Comment 14 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-10-20 01:01:09 PDT
Oh, he was. Sorry.
Comment 15 Daniel Veditz [:dveditz] 2010-10-20 12:00:14 PDT
Is cross_fuzz public? I thought it wasn't and I don't know with whom you've shared it. Didn't want to start a deluge of "why won't you share it with _me_?" requests headed your way.

Should we add a note like the ones in MFSA 2010-49 and MFSA 2010-59 every time we fix a crossfuzz-found bug? Forever? Until the fuzzer is public?
Comment 16 Michal Zalewski 2010-10-20 12:10:22 PDT
It's not public, it's shared with all the major browser vendors (as all are affected). I certainly don't mind a deluge of requests (if there's one thing I'm good at, it's turning people down).

It's no big deal, really; it'd be cool to get a customary acknowledgment on fixed bugs, as it's pretty much the currency in the open source infosec world - but it's your call, depending on how much you believe cross_fuzz actually contributed to spotting the flaw, etc.
Comment 17 Jesse Ruderman 2010-11-06 18:29:03 PDT
Crashtest: http://hg.mozilla.org/mozilla-central/rev/d29ac45571d9

Note You need to log in before you can comment on or make changes to this bug.