33.65 KB, text/plain
24.83 KB, text/plain
7.43 KB, patch
|Details | Diff | Splinter Review|
4.68 KB, application/xml
25.37 KB, text/plain
809 bytes, patch
Josh Aas: review+
Simon Fraser: superreview+
|Details | Diff | Splinter Review|
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.
Same crash on Gecko 1.8 branch.
Still crashes with the same signature with an opt trunk build from a few hours ago.
The crash also occurs in a GTK1 debug build, but not GTK2 (neither with MathML).
Created attachment 197272 [details] [diff] [review] Add null-checks on 'mCCMap' This fixes my crash.
Just switch to nsFontGTK::SupportsChar(). It already has the null-check.
I still see the Mac crash (comment 0) with yesterday's trunk nightly. It happens while the status bar counter says 2400 now, though.
You might have to reload the testcase to trigger the crash. The GTK patch should probably be moved into another bug.
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.
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.
Created attachment 209775 [details] simple testcase for the Mac crash Same testcase without CDATA markers.
Do we need to get someone from Apple to look at the simple testcase and crash?
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.
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.
Josh, can you take this bug?
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.
(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)
The original unreduced testcase still crashes both FF188.8.131.52pre and FF184.108.40.206pre. Filed bug 366493 since the crash is nothing like the one fixed in this bug.
The reduced testcase still crashes Fx 220.127.116.11 (and I'd guess 18.104.22.168). 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.
Created attachment 259624 [details] Crash log from Fx 22.214.171.124 Here's the log I got using 126.96.36.199 with the simplified testcase. They look very similar to me as the initial crash log above...
Created attachment 259625 [details] Real crash log from 188.8.131.52 Whoops. Wrong log. Here's the real one...
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.
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.
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 on attachment 262211 [details] [diff] [review] fix Great!
Comment on attachment 262211 [details] [diff] [review] fix Requesting approval on this patch for the 1.8.0 and 1.8 branches.
Comment on attachment 262211 [details] [diff] [review] fix approved for 184.108.40.206 and 220.127.116.11, a=dveditz
Landed on MOZILLA_1_8_BRANCH and MOZILLA_1_8_0_BRANCH.
Stuart - can you please land this on trunk even though it isn't used there?
Landed on trunk.
verified on the 1.8 branch using Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:18.104.22.168pre) Gecko/2007050804 BonEcho/22.214.171.124pre. 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.
verified on the 1.8.0 branch using Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:126.96.36.199pre) Gecko/20070508 Firefox/188.8.131.52pre. 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.
v.fixed with latest trunk nightly
Crashtest is in: gfx/src/mac/crashtests/306902-1.xml
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).
That's where the patch was. Want me to move the test to gfx/src/thebes/ or something?
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.