306902 - MathML crash [@ szone_free] [@ nsUnicodeRenderingToolkit::GetTextSegmentBoundingMetrics] [@ SetFontCharsetInfo] [@ OutlineMetrics]

Status: VERIFIED FIXED

(Core :: MathML, defect)

Description

[Jesse Ruderman]

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]

Attachment: testcase (not reduced)

Comment 2

[Jesse Ruderman]

Attachment: apple crash report with stack trace

Comment 3

[Jesse Ruderman]

Same crash on Gecko 1.8 branch.

Comment 4

[Jesse Ruderman]

Still crashes with the same signature with an opt trunk build from a few hours ago.

Comment 5

[Mats Palmgren (inactive)]

The crash also occurs in a GTK1 debug build, but not GTK2 (neither with MathML).

Comment 6

[Mats Palmgren (inactive)]

Attachment: Stack + data

Comment 7

[Mats Palmgren (inactive)]

Attachment: Add null-checks on 'mCCMap'

This fixes my crash.

Comment 8

[rbs]

Just switch to nsFontGTK::SupportsChar(). It already has the null-check.

Comment 9

[Jesse Ruderman]

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]

Attachment: simple testcase for the Mac crash

Comment 11

[Jesse Ruderman]

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) No longer reading bugmail consistently]

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]

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]

Attachment: simple testcase for the Mac crash

Same testcase without CDATA markers.

Comment 15

[Jesse Ruderman]

Do we need to get someone from Apple to look at the simple testcase and crash?

Comment 16

[Jesse Ruderman]

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) No longer reading bugmail consistently]

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]

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]

Josh, can you take this bug?

Comment 20

[Jesse Ruderman]

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 (inactive)]

(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 (inactive)]

Filed bug 365923 on the remaining problem described in comment 21.

Comment 23

[Daniel Veditz [:dveditz]]

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

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]]

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

Attachment: 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)]

Attachment: Real crash log from 2.0.0.3

Whoops. Wrong log. Here's the real one...

Comment 28

[Daniel Veditz [:dveditz]]

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]]

(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]

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]]

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]

Attachment: 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]

Comment on attachment 262211 [details] [diff] [review]
fix

Great!

Comment 34

[Samuel Sidler (old account; do not CC)]

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]]

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]

Landed on MOZILLA_1_8_BRANCH and MOZILLA_1_8_0_BRANCH.

Comment 37

[Josh Aas]

Stuart - can you please land this on trunk even though it isn't used there?

Comment 38

[Stuart Morgan]

Landed on trunk.

Comment 39

[Marcia Knous [:marcia]]

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]]

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]]

v.fixed with latest trunk nightly

Comment 42

[Jesse Ruderman]

Crashtest is in: gfx/src/mac/crashtests/306902-1.xml

Comment 43

[Peter Weilbacher]

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]

That's where the patch was.  Want me to move the test to gfx/src/thebes/ or something?

Comment 45

[Peter Weilbacher]

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]

Test moved.