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

VERIFIED FIXED

Status

()

Core
MathML
--
critical
VERIFIED FIXED
12 years ago
10 years ago

People

(Reporter: Jesse Ruderman, Assigned: Josh Aas)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
PowerPC
Mac OS X
crash, testcase, verified1.8.0.12, verified1.8.1.4
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1.4 +
wanted1.8.1.x +
blocking1.8.0.12 +
wanted1.8.0.x +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?] See bug 366493 for 1.8 branch null deref, bug 365923 for GTK1 crash, crash signature)

Attachments

(6 attachments, 2 obsolete attachments)

(Reporter)

Description

12 years ago
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.
(Reporter)

Comment 1

12 years ago
Created attachment 194712 [details]
testcase (not reduced)
(Reporter)

Comment 2

12 years ago
Created attachment 194713 [details]
apple crash report with stack trace
(Reporter)

Comment 3

12 years ago
Same crash on Gecko 1.8 branch.
(Reporter)

Comment 4

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

Comment 5

12 years ago
The crash also occurs in a GTK1 debug build, but not GTK2 (neither with MathML).
Summary: MathML/StirDOM crash [@ szone_free] [@ nsUnicodeRenderingToolkit::GetTextSegmentBoundingMetrics] → MathML/StirDOM crash [@ szone_free] [@ nsUnicodeRenderingToolkit::GetTextSegmentBoundingMetrics] [@ SetFontCharsetInfo]

Comment 6

12 years ago
Created attachment 197271 [details]
Stack + data

Comment 7

12 years ago
Created attachment 197272 [details] [diff] [review]
Add null-checks on 'mCCMap'

This fixes my crash.

Comment 8

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

Updated

12 years ago
Whiteboard: [patch] for GTK crash only

Updated

12 years ago
Assignee: rbs → bugmail
(Reporter)

Comment 9

12 years ago
I still see the Mac crash (comment 0) with yesterday's trunk nightly.  It happens while the status bar counter says 2400 now, though.
(Reporter)

Comment 10

12 years ago
Created attachment 209675 [details]
simple testcase for the Mac crash
(Reporter)

Comment 11

12 years ago
You might have to reload the testcase to trigger the crash.

The GTK patch should probably be moved into another bug.
Keywords: testcase
Summary: MathML/StirDOM crash [@ szone_free] [@ nsUnicodeRenderingToolkit::GetTextSegmentBoundingMetrics] [@ SetFontCharsetInfo] → MathML/StirDOM crash [@ szone_free] [@ nsUnicodeRenderingToolkit::GetTextSegmentBoundingMetrics] [@ SetFontCharsetInfo] [@ OutlineMetrics]
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.
(Reporter)

Comment 13

12 years ago
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.
(Reporter)

Comment 14

12 years ago
Created attachment 209775 [details]
simple testcase for the Mac crash

Same testcase without CDATA markers.
Attachment #209675 - Attachment is obsolete: true
(Reporter)

Comment 15

12 years ago
Do we need to get someone from Apple to look at the simple testcase and crash?
(Reporter)

Comment 16

12 years ago
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).
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

12 years ago
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.
(Reporter)

Comment 19

12 years ago
Josh, can you take this bug?
Assignee: bugmail → nobody
Assignee: nobody → joshmoz
Whiteboard: [patch] for GTK crash only → [sg:investigate][patch] for GTK crash only

Updated

12 years ago
Depends on: 121540
(Reporter)

Updated

11 years ago
OS: Mac OS X 10.2 → Mac OS X 10.4
Whiteboard: [sg:investigate][patch] for GTK crash only → [sg:critical?] [patch for GTK crash only]
(Reporter)

Comment 20

11 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Depends on: 323934
Resolution: --- → FIXED
Whiteboard: [sg:critical?] [patch for GTK crash only] → [sg:critical?] Stir DOM testcase
(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)
Filed bug 365923 on the remaining problem described in comment 21.
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.
Whiteboard: [sg:critical?] Stir DOM testcase → [sg:critical?] Stir DOM testcase. See bug 366493 for 1.8 branch
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?
Does the crash look like 366493? Does it match the apple stack traces in this bug? If neither we probably need a new bug.
Summary: MathML/StirDOM crash [@ szone_free] [@ nsUnicodeRenderingToolkit::GetTextSegmentBoundingMetrics] [@ SetFontCharsetInfo] [@ OutlineMetrics] → MathML crash [@ szone_free] [@ nsUnicodeRenderingToolkit::GetTextSegmentBoundingMetrics] [@ SetFontCharsetInfo] [@ OutlineMetrics]
Whiteboard: [sg:critical?] Stir DOM testcase. See bug 366493 for 1.8 branch → [sg:critical?] See bug 366493 for 1.8 branch null deref
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...
Created attachment 259625 [details]
Real crash log from 2.0.0.3

Whoops. Wrong log. Here's the real one...
Attachment #259624 - Attachment is obsolete: true
Whiteboard: [sg:critical?] See bug 366493 for 1.8 branch null deref → [sg:critical?] See bug 366493 for 1.8 branch null deref, bug 365923 for GTK1 crash
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
(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.
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.4?
Flags: blocking1.8.0.12?
Flags: blocking1.8.1.4?
Flags: blocking1.8.1.4+
Flags: blocking1.8.0.12?
Flags: blocking1.8.0.12+
(Assignee)

Comment 30

11 years ago
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.
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.
Flags: blocking1.8.1.5+
Flags: blocking1.8.1.4+
Flags: blocking1.8.0.13+
Flags: blocking1.8.0.12+

Comment 32

11 years ago
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.
Attachment #262211 - Flags: review?(joshmoz)
(Assignee)

Comment 33

11 years ago
Comment on attachment 262211 [details] [diff] [review]
fix

Great!
Attachment #262211 - Flags: review?(joshmoz) → review+
(Assignee)

Updated

11 years ago
Flags: blocking1.8.1.4?
Attachment #262211 - Flags: superreview?(vladimir)

Updated

11 years ago
Attachment #262211 - Flags: superreview?(vladimir) → superreview+
Comment on attachment 262211 [details] [diff] [review]
fix

Requesting approval on this patch for the 1.8.0 and 1.8 branches.
Attachment #262211 - Flags: approval1.8.1.4?
Attachment #262211 - Flags: approval1.8.0.12?
Comment on attachment 262211 [details] [diff] [review]
fix

approved for 1.8.0.12 and 1.8.1.4, a=dveditz
Attachment #262211 - Flags: approval1.8.1.4?
Attachment #262211 - Flags: approval1.8.1.4+
Attachment #262211 - Flags: approval1.8.0.12?
Attachment #262211 - Flags: approval1.8.0.12+
Flags: blocking1.8.1.5+
Flags: blocking1.8.1.4?
Flags: blocking1.8.1.4+
Flags: blocking1.8.0.13+
Flags: blocking1.8.0.12+

Comment 36

11 years ago
Landed on MOZILLA_1_8_BRANCH and MOZILLA_1_8_0_BRANCH.
Keywords: fixed1.8.0.12, fixed1.8.1.4
(Assignee)

Comment 37

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

Comment 38

11 years ago
Landed on trunk.
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.
Keywords: fixed1.8.1.4 → verified1.8.1.4
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.
Keywords: fixed1.8.0.12 → verified1.8.0.12
Group: security
Flags: in-testsuite?

Comment 41

10 years ago
v.fixed with latest trunk nightly
Status: RESOLVED → VERIFIED
(Reporter)

Comment 42

10 years ago
Crashtest is in: gfx/src/mac/crashtests/306902-1.xml
Flags: in-testsuite? → in-testsuite+

Comment 43

10 years ago
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).
(Reporter)

Comment 44

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

Comment 45

10 years ago
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.
(Reporter)

Comment 46

10 years ago
Test moved.
Crash Signature: [@ szone_free] [@ nsUnicodeRenderingToolkit::GetTextSegmentBoundingMetrics] [@ SetFontCharsetInfo] [@ OutlineMetrics]
You need to log in before you can comment on or make changes to this bug.