296134 - Crash on unicode "zero width non-joiner" sequence

Status: VERIFIED FIXED

(Core :: Internationalization, defect)

Description

[David McWilliams]

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]

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]

I haven't been able to reproduce this yet in either trunk or branch. 

Comment 3

[Stephen Donner [:stephend] Not actively reading bugmail]

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]

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 [:bzbarsky]]

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

Works for me on Linux GTK2 x86-64.

Comment 7

[Corinne Bordelon]

Not a blocker, but will consider an approved fix if the risk is low.

Comment 8

[Boris Zbarsky [:bzbarsky]]

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-4, no longer working on Mozilla)]

renominating for 1.8b4

Comment 10

[Brendan Eich [:brendan]]

Asa, we should have a blocking1.8 flag, shouldn't we?

/be

Comment 11

[Mats Palmgren (inactive)]

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] Not actively reading bugmail]

David: see https://bugzilla.mozilla.org/show_bug.cgi?id=296134#c11

Comment 13

[Mats Palmgren (inactive)]

Attachment: Stack

We are stuck in frame #5

Comment 14

[Mats Palmgren (inactive)]

Attachment: Detailed trace of 'morkParser::ReadValue()'

Notice the "unget $" at the beginning of the sequence, it only occurs there.

Comment 15

[Mats Palmgren (inactive)]

This is the spots that I traced:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/db/mork/src/morkParser.cpp&rev=1.23&root=/cvsroot&mark=907,909,914,929#890

Comment 16

[David :Bienvenu]

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]

Mats, I don't see how the first issue reported and its url could be related to
your stack trace.

Comment 18

[Mats Palmgren (inactive)]

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

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

Attachment: Stack before we overwrite it

Note 'aLength=4294967295' in the frames at the top.

Comment 21

[Mats Palmgren (inactive)]

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

Attachment: Patch rev. 1

Comment 23

[Mats Palmgren (inactive)]

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-4, no longer working on Mozilla)]

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

Attachment: Patch rev. 2

More robust version.

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

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

Comment on attachment 192278 [details] [diff] [review]
Patch rev. 2

a=shaver, please commit to trunk today before branch.

Comment 29

[Mats Palmgren (inactive)]

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

Attachment: More checks

Let me know if you want any of these additional checks.

Comment 31

[Stephen Donner [:stephend] Not actively reading bugmail]

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

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-4, no longer working on Mozilla)]

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

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

Attachment: Patch rev. 1 + additional checks/assertions

Here's the smaller change (Patch rev. 1) + the extra checks/assertions.

Comment 36

[Mats Palmgren (inactive)]

-> FIXED

Comment 37

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED on trunk using build 2005-09-01-05 SeaMonkey trunk on Windows XP.

Comment 38

[Tracy Walker [:tracy]]

verified on Firefox 1.4 -mozilla1.8 branch- Win, Lin and Mac : 2005-09-07

Comment 39

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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-4, no longer working on Mozilla)]

Checked in to MOZILLA_1_7_BRANCH and AVIARY_1_0_1_2005124_BRANCH.

Comment 41

[Bob Clary [:bc] (inactive)]

test is 404.