Last Comment Bug 296134 - Crash on unicode "zero width non-joiner" sequence
: Crash on unicode "zero width non-joiner" sequence
Status: VERIFIED FIXED
[sg:fix]
: crash, fixed-aviary1.0.7, fixed1.7.12, testcase, verified1.8
Product: Core
Classification: Components
Component: Internationalization (show other bugs)
: Trunk
: All All
: -- critical (vote)
: ---
Assigned To: Mats Palmgren (:mats)
: Yuying Long
: Makoto Kato [:m_kato]
Mentors:
http://ehd.org/firefox-crash.html
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-05-31 15:40 PDT by David McWilliams
Modified: 2011-08-05 23:34 PDT (History)
15 users (show)
asa: blocking1.7.12+
asa: blocking‑aviary1.0.7+
mtschrep: blocking1.8b5+
bob: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Stack (5.62 KB, text/plain)
2005-08-09 23:14 PDT, Mats Palmgren (:mats)
no flags Details
Detailed trace of 'morkParser::ReadValue()' (3.73 KB, text/plain)
2005-08-09 23:31 PDT, Mats Palmgren (:mats)
no flags Details
Crash stack (1.49 KB, text/plain)
2005-08-10 13:17 PDT, Mats Palmgren (:mats)
no flags Details
Stack before we overwrite it (8.96 KB, text/plain)
2005-08-10 13:25 PDT, Mats Palmgren (:mats)
no flags Details
Patch rev. 1 (1.36 KB, patch)
2005-08-10 13:34 PDT, Mats Palmgren (:mats)
dbaron: review+
dbaron: superreview+
Details | Diff | Splinter Review
Patch rev. 2 (1.72 KB, patch)
2005-08-10 16:08 PDT, Mats Palmgren (:mats)
dbaron: review+
dbaron: superreview+
dbaron: approval‑aviary1.0.7+
dbaron: approval1.7.12+
shaver: approval1.8b4+
Details | Diff | Splinter Review
More checks (4.28 KB, patch)
2005-08-11 14:07 PDT, Mats Palmgren (:mats)
dbaron: review+
Details | Diff | Splinter Review
Patch rev. 1 + additional checks/assertions (3.72 KB, patch)
2005-08-30 18:10 PDT, Mats Palmgren (:mats)
dbaron: approval‑aviary1.0.7-
dbaron: approval1.7.12-
Details | Diff | Splinter Review

Description David McWilliams 2005-05-31 15:40:42 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.8) Gecko/20050511 Firefox/1.0.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.8) Gecko/20050511 Firefox/1.0.4

This sequence of 13 unicode characters and a carriage return crashes Firefox 1.04.

Reproducible: Always

Steps to Reproduce:
1.Load the linked page
2.
3.

Actual Results:  
Firefox immediately closed with no error message.

Expected Results:  
Displayed the 2 visible characters.

It also crashes in safe mode, so my extensions are not the issue.  I am using
the default theme.
Comment 1 timeless 2005-05-31 17:12:27 PDT
seems to happen w/ 1.8b2 Gecko/2005053105 too.
doesn't seem to happen w/ my cvs tree from before trunk got scary (that happened
about a month ago).
Comment 2 Simon Montagu :smontagu 2005-05-31 22:18:52 PDT
I haven't been able to reproduce this yet in either trunk or branch. 
Comment 3 Stephen Donner [:stephend] 2005-06-01 10:56:09 PDT
Loading http://ehd.org/firefox-crash.html with build 2005-06-01-05 completely
'cleanly' exits Mozilla -- that is, it doesn't bring up Talkback, it just exits
the app.  Windows XP.
Comment 4 Ronald Tilby 2005-06-10 20:06:13 PDT
Confirmed using Mozilla Trunk Nightly 2006061006 on Windows XP.
Mozilla just exits while loading the URL.  No crash, it just exits.
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2005-08-01 08:26:30 PDT
This crashes quite nicely on Linux too.  I have the following stack (in a
Firefox GTK2 XFT build):

#0  0xb5c991f5 in ConvertUnicharToUCS4 (aString=0xbfffc9a8, aLength=4294967293, 
    aOutBuffer=@0xbfff94b0, aOutLen=0xbfffc3a4)
    at ../../../../mozilla/gfx/src/gtk/nsFontMetricsXft.cpp:2385
#1  0xb5c965c0 in nsFontMetricsXft::EnumerateGlyphs (this=0x0, aString=0x3363,
aLen=0, 
    aCallback={__pfn = 0, __delta = 57968}, aCallbackData=0xe148)
    at ../../../../mozilla/gfx/src/gtk/nsFontMetricsXft.cpp:1467
#2  0x00000000 in ?? ()

I get a slightly more informative stack with a GTK1 Seamonkey build:

#0  0x4071042f in memcpy () from /lib/libc.so.6
#1  0x413ecfd6 in nsTextFrame::PrepareUnicodeText (this=0x4283a87c,
aTX=@0xbfffd880, 
    aIndexBuffer=0xbfffd9c0, aTextBuffer=0xbfffdb60, aTextLen=0xbfffd87c, 
    aForceArabicShaping=0, aJustifiableCharCount=0x0)
    at ../../../mozilla/layout/generic/nsTextFrame.cpp:1868
#2  0x413ee7d5 in nsTextFrame::PaintUnicodeText (this=0xdddddddd, 
    aPresContext=0xdddddddd, aRenderingContext=@0xdddddddd,
aStyleContext=0xdddddddd, 
    aTextStyle=@0xdddddddd, dx=-572662307, dy=-572662307)
    at ../../../mozilla/layout/generic/nsTextFrame.cpp:2517
#3  0xdddddddd in ?? ()
#4  0xdddddddd in ?? ()

So it looks like we're working with deleted frames and stomping all over memory
(including the stack).  Based on that, this may be exploitable; something we
should check on at least.

Comment 6 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-08-02 16:28:19 PDT
Works for me on Linux GTK2 x86-64.
Comment 7 Corinne Bordelon 2005-08-03 16:06:34 PDT
Not a blocker, but will consider an approved fix if the risk is low.
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2005-08-04 11:06:53 PDT
I feel strongly that this is something we need to fix for 1.8 final, even if we
don't do it for 1.8 beta 4.  How do I nominate this for 1.8 final?
Comment 9 David Baron :dbaron: ⌚️UTC-8 2005-08-04 11:17:15 PDT
renominating for 1.8b4
Comment 10 Brendan Eich [:brendan] 2005-08-04 12:03:26 PDT
Asa, we should have a blocking1.8 flag, shouldn't we?

/be
Comment 11 Mats Palmgren (:mats) 2005-08-09 23:12:12 PDT
The problem is an infinite loop in 'morkParser::ReadValue' and it causes calls
to 'morkBlob::GrowBlob' with increasing sizes until we crash...
Comment 12 Stephen Donner [:stephend] 2005-08-09 23:13:50 PDT
David: see https://bugzilla.mozilla.org/show_bug.cgi?id=296134#c11
Comment 13 Mats Palmgren (:mats) 2005-08-09 23:14:33 PDT
Created attachment 192170 [details]
Stack

We are stuck in frame #5
Comment 14 Mats Palmgren (:mats) 2005-08-09 23:31:12 PDT
Created attachment 192172 [details]
Detailed trace of 'morkParser::ReadValue()'

Notice the "unget $" at the beginning of the sequence, it only occurs there.
Comment 16 David :Bienvenu 2005-08-10 06:52:55 PDT
I tried an older seamonkey debug build (from 6/10) and didn't see a problem -
I'll update and see if it happens with a newer build. 

The scenario doesn't quite make sense - the problem occurs parsing the history
file, and going to a page just loads the previous contents of the history file.
So the page you load shouldn't be relevant. It's the pages/urls in the history
file from before you went to that page that should matter. If anyone has a
history.dat file that shows this problem, and can attach it here or send it to
me, I'll take a look.
Comment 17 David :Bienvenu 2005-08-10 08:00:50 PDT
Mats, I don't see how the first issue reported and its url could be related to
your stack trace.
Comment 18 Mats Palmgren (:mats) 2005-08-10 09:40:04 PDT
What I saw above was probably a variation of this bug that only occurs
with a specific "history.dat" file. The profile I used yesterday was tainted by
testing for bug 167315 (huge TITLEs). Unfortunately I didn't save the
"history.dat" that reproduced the infinite loop and today I can't
reproduce it (I now get the same crash as Boris in comment 5).

The odd thing is that I got a strong impression that the testcase in this
bug was the "trigger" since I could load other pages with the same profile
without crashing, but with the URL in this bug I got into this infinite loop
reproducibly.  I'll file a new bug if it occurs again.
Let me know if you still want the "history.dat" (14.1MB), I think you can
get an idea what it looks like by loading the testcases of bug 167315 though.

Sorry if I mislead you.
Comment 19 Mats Palmgren (:mats) 2005-08-10 13:17:30 PDT
Created attachment 192243 [details]
Crash stack

(using a clean profile this time ;-)

This definitely looks like we have overwritten the stack with data.
Could be exploitable.
Comment 20 Mats Palmgren (:mats) 2005-08-10 13:25:20 PDT
Created attachment 192245 [details]
Stack before we overwrite it

Note 'aLength=4294967295' in the frames at the top.
Comment 21 Mats Palmgren (:mats) 2005-08-10 13:30:10 PDT
A bit of ad-hoc tracing:

[0x8803970] MeasureText 5111 wordLen=14
[0xbfffc520] GetNextWord 859 wordLen=0
[0xbfffc520] GetNextWord 939 wordLen=14
[0xbfffc520] GetNextWord 942 wordLen=3
[0xbfffc520] GetNextWord 1092 before DoArabicShaping, wordLen=3
StripZeroWidthJoinControls+++ aTextLength=-1073758120 src=0xbfffc548 dest=0x401238d5
StripZeroWidthJoinControls *src=8204
 src[0+0] is CH_ZWNJ
 src[0+1] is CH_ZWNJ
StripZeroWidthJoinControls *src=0
StripZeroWidthJoinControls *src=8204
 src[2+0] is CH_ZWNJ
 src[2+1] is CH_ZWNJ
 stripped =4
StripZeroWidthJoinControls--- aTextLength=4
[0xbfffc520] GetNextWord 1094 after DoArabicShaping, wordLen=-1
[0xbfffc520] GetNextWord 1097 before DoNumericShaping, wordLen=-1
[0xbfffc520] GetNextWord 1099 after DoNumericShaping, wordLen=-1
[0xbfffc520] GetNextWord 1109 *aWordLenResult = wordLen=-1
[0x8803970] MeasureText 5116 wordLen=-1
[0x8803970] MeasureText 5292 wordLen=-1
nsFontMetricsXft.cpp:EnumerateGlyphs+++ aString=0xbfffc548 () aLen=-1
aCallbackData=0xbfffc450
Comment 22 Mats Palmgren (:mats) 2005-08-10 13:34:30 PDT
Created attachment 192247 [details] [diff] [review]
Patch rev. 1
Comment 23 Mats Palmgren (:mats) 2005-08-10 13:50:29 PDT
The "aTextLength" values in comment 21 was wrong, correction to that bit:

StripZeroWidthJoinControls+++ aTextLength=3 src=0xbfffc058 dest=0xbfffc548
StripZeroWidthJoinControls *src=8204
 src[0+0] is CH_ZWNJ
 src[0+1] is CH_ZWNJ
StripZeroWidthJoinControls *src=0
StripZeroWidthJoinControls *src=8204
 src[2+0] is CH_ZWNJ
 src[2+1] is CH_ZWNJ
 stripped =4
StripZeroWidthJoinControls--- aTextLength=-1
Comment 24 David Baron :dbaron: ⌚️UTC-8 2005-08-10 14:10:18 PDT
Comment on attachment 192247 [details] [diff] [review]
Patch rev. 1

r+sr=dbaron, although this might be made a little more robust against future
modification by:
 * assigning aTextLength = dest - aTarget at the end instead of subtracting
 * making the for-loop condition compare src against src_end (initially
assigned src + aTextLength)

However, for 1.7.x and 1.8 we should probably just do this.  I think you should
probably request 1.7.12 approval for this in addition to 1.8b4.
Comment 25 Mats Palmgren (:mats) 2005-08-10 16:08:31 PDT
Created attachment 192278 [details] [diff] [review]
Patch rev. 2

More robust version.
Comment 26 David Baron :dbaron: ⌚️UTC-8 2005-08-10 16:19:56 PDT
Comment on attachment 192278 [details] [diff] [review]
Patch rev. 2

r+sr=dbaron, although I'd probably prefer the smaller change for the 1.7
branch, at least
Comment 27 Daniel Veditz [:dveditz] 2005-08-11 02:51:40 PDT
Comment on attachment 192278 [details] [diff] [review]
Patch rev. 2

>+  NS_ASSERTION(aTextLength >= 0, "negative text length");

If there's even a slight chance aTextLength could be negative you need real
error-handling not a debug-only assertion.
Comment 28 Mike Shaver (:shaver -- probably not reading bugmail closely) 2005-08-11 11:09:00 PDT
Comment on attachment 192278 [details] [diff] [review]
Patch rev. 2

a=shaver, please commit to trunk today before branch.
Comment 29 Mats Palmgren (:mats) 2005-08-11 14:05:02 PDT
(In reply to comment #27)
> (From update of attachment 192278 [details] [diff] [review] [edit])
> >+  NS_ASSERTION(aTextLength >= 0, "negative text length");
> 
> If there's even a slight chance aTextLength could be negative you need real
> error-handling not a debug-only assertion.
> 

There is currently only one caller of 'StripZeroWidthJoinControls':
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsTextTransformer.cpp&rev=1.102&root=/cvsroot&mark=1492,1480,1487#1472

The only possibility of 'aTextLength' becoming negative is if
'ArabicShaping()' has a bug and assigns a value > 2GB through '&newLen'.
But then it seems more likely that is has a bug and assigns a value that
is greater than what was allocated by the nsAutoString.
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/base/nsBidiUtils.cpp&rev=3.14&root=/cvsroot&mark=275,365#275
Comment 30 Mats Palmgren (:mats) 2005-08-11 14:07:07 PDT
Created attachment 192415 [details] [diff] [review]
More checks

Let me know if you want any of these additional checks.
Comment 31 Stephen Donner [:stephend] 2005-08-28 15:16:28 PDT
Mats, can you land https://bugzilla.mozilla.org/attachment.cgi?id=192278?  I'm
still crashing with build 2005-08-28-06, so it's pretty clear this never landed.
Comment 32 Mats Palmgren (:mats) 2005-08-28 18:32:08 PDT
I'm waiting for dbaron to say if we need any of the extra checks I suggested
to address dveditz' concern in comment 27. (See comment 29 and comment 30)
Comment 33 David Baron :dbaron: ⌚️UTC-8 2005-08-29 17:52:37 PDT
Comment on attachment 192415 [details] [diff] [review]
More checks

It doesn't matter much to me either way (may as well put them in, since I doubt
it would affect performance), but the DoArabicShaping change should have an
assertion in it so it's clear that it's unexpected.
Comment 34 Mats Palmgren (:mats) 2005-08-30 18:03:14 PDT
Comment on attachment 192415 [details] [diff] [review]
More checks

Added the extra assertion dbaron suggested in comment 33 and
checked in to trunk and 1.8 branch at 2005-08-30 17:35 PDT.
Comment 35 Mats Palmgren (:mats) 2005-08-30 18:10:01 PDT
Created attachment 194399 [details] [diff] [review]
Patch rev. 1 + additional checks/assertions

Here's the smaller change (Patch rev. 1) + the extra checks/assertions.
Comment 36 Mats Palmgren (:mats) 2005-08-30 18:12:30 PDT
-> FIXED
Comment 37 Stephen Donner [:stephend] 2005-09-01 18:33:11 PDT
Verified FIXED on trunk using build 2005-09-01-05 SeaMonkey trunk on Windows XP.
Comment 38 Tracy Walker [:tracy] 2005-09-07 13:08:37 PDT
verified on Firefox 1.4 -mozilla1.8 branch- Win, Lin and Mac : 2005-09-07
Comment 39 David Baron :dbaron: ⌚️UTC-8 2005-09-12 16:40:18 PDT
Given the testing that's happened on trunk and 1.8 branch at this point, I'd
actually prefer to land what landed there  for 1.7 / aviary1.0 rather than
landing the slightly smaller (but less tested) change.  I'll do that shortly
unless somebody objects.
Comment 40 David Baron :dbaron: ⌚️UTC-8 2005-09-12 17:22:42 PDT
Checked in to MOZILLA_1_7_BRANCH and AVIARY_1_0_1_2005124_BRANCH.
Comment 41 Bob Clary [:bc:] 2009-03-31 12:36:40 PDT
test is 404.

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