Last Comment Bug 306902 - MathML crash [@ szone_free] [@ nsUnicodeRenderingToolkit::GetTextSegmentBoundingMetrics] [@ SetFontCharsetInfo] [@ OutlineMetrics]
: MathML crash [@ szone_free] [@ nsUnicodeRenderingToolkit::GetTextSegmentBound...
Status: VERIFIED FIXED
[sg:critical?] See bug 366493 for 1.8...
: crash, testcase, verified1.8.0.12, verified1.8.1.4
Product: Core
Classification: Components
Component: MathML (show other bugs)
: Trunk
: PowerPC Mac OS X
: -- critical (vote)
: ---
Assigned To: Josh Aas
: Hixie (not reading bugmail)
Mentors:
Depends on: atsui 323934
Blocks: stirdom
  Show dependency treegraph
 
Reported: 2005-09-02 16:45 PDT by Jesse Ruderman
Modified: 2008-02-08 01:00 PST (History)
17 users (show)
dveditz: blocking1.8.1.4+
dveditz: wanted1.8.1.x+
dveditz: blocking1.8.0.12+
dveditz: wanted1.8.0.x+
jruderman: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
apple crash report with stack trace (33.65 KB, text/plain)
2005-09-02 16:48 PDT, Jesse Ruderman
no flags Details
Stack + data (24.83 KB, text/plain)
2005-09-24 09:50 PDT, Mats Palmgren (:mats)
no flags Details
Add null-checks on 'mCCMap' (7.43 KB, patch)
2005-09-24 09:52 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Review
simple testcase for the Mac crash (4.69 KB, application/xml)
2006-01-25 23:21 PST, Jesse Ruderman
no flags Details
simple testcase for the Mac crash (4.68 KB, application/xml)
2006-01-26 16:52 PST, Jesse Ruderman
no flags Details
Crash log from Fx 2.0.0.3 (20.48 KB, text/plain)
2007-03-25 14:41 PDT, Samuel Sidler (old account; do not CC)
no flags Details
Real crash log from 2.0.0.3 (25.37 KB, text/plain)
2007-03-25 14:44 PDT, Samuel Sidler (old account; do not CC)
no flags Details
fix (809 bytes, patch)
2007-04-19 21:49 PDT, Stuart Morgan
jaas: review+
sfraser_bugs: superreview+
dveditz: approval1.8.1.4+
dveditz: approval1.8.0.12+
Details | Diff | Review

Description Jesse Ruderman 2005-09-02 16:45:35 PDT
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20050901
Firefox/1.6a1

1. Load the testcase.
2. Dismiss the "missing MathML fonts" dialog when it appears.

Result: Firefox crashes while the status bar counter says "1400".

Filing as security-sensitive becasue the testcase includes code from bug 306663.
Comment 1 Jesse Ruderman 2005-09-02 16:46:24 PDT
Created attachment 194712 [details]
testcase (not reduced)
Comment 2 Jesse Ruderman 2005-09-02 16:48:14 PDT
Created attachment 194713 [details]
apple crash report with stack trace
Comment 3 Jesse Ruderman 2005-09-02 18:15:39 PDT
Same crash on Gecko 1.8 branch.
Comment 4 Jesse Ruderman 2005-09-24 01:45:51 PDT
Still crashes with the same signature with an opt trunk build from a few hours ago.
Comment 5 Mats Palmgren (:mats) 2005-09-24 09:49:35 PDT
The crash also occurs in a GTK1 debug build, but not GTK2 (neither with MathML).
Comment 6 Mats Palmgren (:mats) 2005-09-24 09:50:17 PDT
Created attachment 197271 [details]
Stack + data
Comment 7 Mats Palmgren (:mats) 2005-09-24 09:52:28 PDT
Created attachment 197272 [details] [diff] [review]
Add null-checks on 'mCCMap'

This fixes my crash.
Comment 8 rbs 2005-10-02 18:22:38 PDT
Just switch to nsFontGTK::SupportsChar(). It already has the null-check.
Comment 9 Jesse Ruderman 2006-01-12 22:54:40 PST
I still see the Mac crash (comment 0) with yesterday's trunk nightly.  It happens while the status bar counter says 2400 now, though.
Comment 10 Jesse Ruderman 2006-01-25 23:21:49 PST
Created attachment 209675 [details]
simple testcase for the Mac crash
Comment 11 Jesse Ruderman 2006-01-25 23:24:49 PST
You might have to reload the testcase to trigger the crash.

The GTK patch should probably be moved into another bug.
Comment 12 Jonas Sicking (:sicking) 2006-01-26 16:24:29 PST
I can not reproduce this on windows. The stack jesse showed me seemed to indicate that some mac api was unable to deal with measuring too long words.

What surpriced me a lot though is that it made a difference if the node was a cdata node or a text node. That should never make a difference in which code paths we're executing.
Comment 13 Jesse Ruderman 2006-01-26 16:50:51 PST
Sorry, I was wrong; it crashes with a normal text node too.  I'm not sure how I convinced myself that it had to be a CDATA node.
Comment 14 Jesse Ruderman 2006-01-26 16:52:00 PST
Created attachment 209775 [details]
simple testcase for the Mac crash

Same testcase without CDATA markers.
Comment 15 Jesse Ruderman 2006-01-26 16:55:47 PST
Do we need to get someone from Apple to look at the simple testcase and crash?
Comment 16 Jesse Ruderman 2006-01-26 17:18:04 PST
With the switch to Thebes/Cairo, we won't be using this Apple API at all, so maybe this crash will just go away (at least on trunk).
Comment 17 Jonas Sicking (:sicking) 2006-01-26 17:19:18 PST
That won't happen on branch though, and given that this crashes due to a long string i get sort of worried about it being a buffer overrun.
Comment 18 YAMASHITA Makoto 2006-01-27 15:44:33 PST
It looks like the patch at bug 121540 solves this. But this is really odd, because  bounding metrics measurement is one of the parts left almost untouched by that.
Comment 19 Jesse Ruderman 2006-02-02 16:20:06 PST
Josh, can you take this bug?
Comment 20 Jesse Ruderman 2006-12-13 17:15:36 PST
Fixed by the switch to cairo on Mac, as expected:
* Crashes a 2006-11-21 nightly.
* Does not crash a 2006-11-25 nightly.
* Does not crash fresh opt and debug builds.

Mats, please move your GTK patch to another bug if it is still needed.
Comment 21 Mats Palmgren (:mats) 2006-12-14 04:54:26 PST
(In reply to comment #20)
> Mats, please move your GTK patch to another bug if it is still needed.

It still crashes. I'm pretty sure it's gtk1 or xlib widget only.
It's a SEGV crash from a read. I'm not sure how much of a priority that is.

The bug seems to have changed from the last time (attachment 197271 [details])
The crash now occurs when calling CCMAP_HAS_CHAR_EXT(m,c) when 'c' is a
surrogate character and 'm' is 'gDoubleByteSpecialCharsCCMap' which comes
from a generated file ("dbyte_special_chars.ccmap"):
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/gfx/src/gtk/nsFontMetricsGTK.cpp&rev=1.289&root=/cvsroot&mark=872-873#865
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/gfx/src/x11shared/dbyte_special_chars.ccmap&rev=1.5&root=/cvsroot

It could mean that the ccmap generator has a bug or CCMAP_HAS_CHAR_EXT
has bug, both are used on other (modern) platforms too I think.

Or maybe CCMAP_HAS_CHAR_EXT is just used inappropriately here:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/gfx/src/gtk/nsFontMetricsGTK.cpp&rev=1.289&root=/cvsroot&mark=4112,4122#4089
(We crash on line 4122, with 'c' == 120793, which was assigned on line 4112
from aString[i]=55349 and aString[i+1]=57305.)

Some more debug data, for posterity:
gDoubleByteSpecialCharsCCMap c=120793 (0x1d7d9) (c)&0xffff0000 is true (0x10000)
extraSurrogateLength=1 , aString[i]=55349 aString[i+1]=57305
ALU_SIZE=32 CCMAP_SIZE(gDoubleByteSpecialCharsCCMap)=16361
(CCMAP_FLAG(m) & CCMAP_SURROGATE_FLAG) is true
CCMAP_PLANE(c) = 1
CCMAP_TO_PAGE((m),(c)) = 32
CCMAP_ALU_INDEX(c & 0xffff) = 6
(CCMAP_TO_ALU(m, c & 0xffff) = 0
CCMAP_BIT_INDEX(c & 0xffff) = 25
(CCMAP_TO_ALU(m, c & 0xffff) >> CCMAP_BIT_INDEX(c & 0xffff) = 0
CCMAP_FOR_PLANE_EXT(gDoubleByteSpecialCharsCCMap, CCMAP_PLANE(c)) = 0xf6e1c24 (-824388892 bytes off gDoubleByteSpecialCharsCCMap)
Program received signal SIGSEGV, Segmentation fault.

Maybe Talkback can enlighten us if this a problem on other platforms as well?
(analyzing consumers of SupportsChar() and CCMAP_HAS_CHAR_EXT)
Comment 22 Mats Palmgren (:mats) 2007-01-04 08:15:02 PST
Filed bug 365923 on the remaining problem described in comment 21.
Comment 23 Daniel Veditz [:dveditz] 2007-01-09 16:32:20 PST
The original unreduced testcase still crashes both FF1.5.0.10pre and FF2.0.0.2pre. Filed bug 366493 since the crash is nothing like the one fixed in this bug.
Comment 24 Samuel Sidler (old account; do not CC) 2007-03-22 14:38:49 PDT
The reduced testcase still crashes Fx 2.0.0.3 (and I'd guess 1.5.0.11). I know that you filed bug 366493 on the original testcase, but should this bug be nominated as well for blocking?
Comment 25 Daniel Veditz [:dveditz] 2007-03-23 10:37:38 PDT
Does the crash look like 366493? Does it match the apple stack traces in this bug? If neither we probably need a new bug.
Comment 26 Samuel Sidler (old account; do not CC) 2007-03-25 14:41:50 PDT
Created attachment 259624 [details]
Crash log from Fx 2.0.0.3

Here's the log I got using 2.0.0.3 with the simplified testcase. They look very similar to me as the initial crash log above...
Comment 27 Samuel Sidler (old account; do not CC) 2007-03-25 14:44:02 PDT
Created attachment 259625 [details]
Real crash log from 2.0.0.3

Whoops. Wrong log. Here's the real one...
Comment 28 Daniel Veditz [:dveditz] 2007-03-25 16:58:14 PDT
Comment on attachment 194712 [details]
testcase (not reduced)

Sorry for marking this private prematurely. I was going to open the bug and must have noticed the spin-off bugs and stopped half-way
Comment 29 Daniel Veditz [:dveditz] 2007-03-25 17:53:39 PDT
(In reply to comment #20)
> Fixed by the switch to cairo on Mac, as expected:

Which is great for the trunk, but leaves an sg:critical bug on the branch. Still need a patch.
Comment 30 Josh Aas 2007-03-26 16:03:52 PDT
I don't know much of anything about our font code, and unfortunately the people who wrote that code are no longer around. I'll take a look or try to find someone better suited for this bug.
Comment 31 Daniel Veditz [:dveditz] 2007-04-19 10:41:58 PDT
Won't this potentially exploitable crash affect branch-based Camino as well? Maybe someone on the Camino team has the font expertise we need. Guess it's not likely to make this set of releases, though -- moving out.
Comment 32 Stuart Morgan 2007-04-19 21:49:36 PDT
Created attachment 262211 [details] [diff] [review]
fix

No domain knowledge needed, as it turns out. This is just a simple smasher from what looks like a copy-and-paste bug; Rect is 8 bytes, Fixed is 4, so the rects array was half the length it needed to be.
Comment 33 Josh Aas 2007-04-20 01:46:09 PDT
Comment on attachment 262211 [details] [diff] [review]
fix

Great!
Comment 34 Samuel Sidler (old account; do not CC) 2007-04-20 17:43:41 PDT
Comment on attachment 262211 [details] [diff] [review]
fix

Requesting approval on this patch for the 1.8.0 and 1.8 branches.
Comment 35 Daniel Veditz [:dveditz] 2007-04-21 00:31:42 PDT
Comment on attachment 262211 [details] [diff] [review]
fix

approved for 1.8.0.12 and 1.8.1.4, a=dveditz
Comment 36 Stuart Morgan 2007-04-21 08:53:48 PDT
Landed on MOZILLA_1_8_BRANCH and MOZILLA_1_8_0_BRANCH.
Comment 37 Josh Aas 2007-04-21 17:11:50 PDT
Stuart - can you please land this on trunk even though it isn't used there?
Comment 38 Stuart Morgan 2007-04-21 20:11:07 PDT
Landed on trunk.
Comment 39 Marcia Knous [:marcia - use ni] 2007-05-08 16:00:19 PDT
verified on the 1.8 branch using Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.4pre) Gecko/2007050804 BonEcho/2.0.0.4pre. I don't crash with the simple testcases. I do see what dveditz notes in Comment 23, and my stack appears to match what appears in Bug 366493. Adding branch verified keyword.
Comment 40 Marcia Knous [:marcia - use ni] 2007-05-08 16:03:49 PDT
verified on the 1.8.0 branch using Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.12pre) Gecko/20070508 Firefox/1.5.0.12pre. I don't crash with
the simple testcases. I do see what dveditz notes in Comment 23, and my stack
appears to match what appears in Bug 366493. Adding branch verified keyword.
Comment 41 Jay Patel [:jay] 2007-08-22 20:17:07 PDT
v.fixed with latest trunk nightly
Comment 42 Jesse Ruderman 2007-12-11 18:27:32 PST
Crashtest is in: gfx/src/mac/crashtests/306902-1.xml
Comment 43 Peter Weilbacher 2008-01-29 07:20:28 PST
Jesse, was your crashtest supposed to land in gfx/src/mac on trunk? Just wondering because I thought gfx/src/mac was going away (bug 376791).
Comment 44 Jesse Ruderman 2008-01-29 11:40:45 PST
That's where the patch was.  Want me to move the test to gfx/src/thebes/ or something?
Comment 45 Peter Weilbacher 2008-01-29 12:21:44 PST
I can't judge if it still makes sense to keep this test but if so then gfx/src/thebes/ is probably a good location.
Comment 46 Jesse Ruderman 2008-02-08 01:00:38 PST
Test moved.

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