Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Crash on unicode "zero width non-joiner" sequence

VERIFIED FIXED

Status

()

Core
Internationalization
--
critical
VERIFIED FIXED
12 years ago
6 years ago

People

(Reporter: David McWilliams, Assigned: Mats Palmgren (vacation - back in August))

Tracking

(5 keywords)

Trunk
crash, fixed-aviary1.0.7, fixed1.7.12, testcase, verified1.8
Points:
---
Bug Flags:
blocking1.7.12 +
blocking-aviary1.0.7 +
blocking1.8b5 +
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:fix], URL)

Attachments

(4 attachments, 4 obsolete attachments)

(Reporter)

Description

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

Updated

12 years ago

Comment 1

12 years ago
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).
Assignee: nobody → smontagu
Component: General → Internationalization
Product: Firefox → Core
QA Contact: general → amyy
Version: unspecified → Trunk
I haven't been able to reproduce this yet in either trunk or branch. 
Whiteboard: stackwanted
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

12 years ago
Confirmed using Mozilla Trunk Nightly 2006061006 on Windows XP.
Mozilla just exits while loading the URL.  No crash, it just exits.

Comment 5

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

Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.8b4?
OS: Windows XP → All
Hardware: PC → All
Works for me on Linux GTK2 x86-64.

Comment 7

12 years ago
Not a blocker, but will consider an approved fix if the risk is low.
Flags: blocking1.8b4? → blocking1.8b4-

Comment 8

12 years ago
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?
renominating for 1.8b4
Flags: blocking1.8b4- → blocking1.8b4?
Asa, we should have a blocking1.8 flag, shouldn't we?

/be

Updated

12 years ago
Flags: blocking1.8b4? → blocking1.8b4+
The problem is an infinite loop in 'morkParser::ReadValue' and it causes calls
to 'morkBlob::GrowBlob' with increasing sizes until we crash...
David: see https://bugzilla.mozilla.org/show_bug.cgi?id=296134#c11
Created attachment 192170 [details]
Stack

We are stuck in frame #5
Created attachment 192172 [details]
Detailed trace of 'morkParser::ReadValue()'

Notice the "unget $" at the beginning of the sequence, it only occurs there.
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

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

12 years ago
Mats, I don't see how the first issue reported and its url could be related to
your stack trace.
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.
Keywords: testcase
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.
Attachment #192170 - Attachment is obsolete: true
Attachment #192172 - Attachment is obsolete: true
Assignee: smontagu → mats.palmgren
Whiteboard: stackwanted
Created attachment 192245 [details]
Stack before we overwrite it

Note 'aLength=4294967295' in the frames at the top.
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
Created attachment 192247 [details] [diff] [review]
Patch rev. 1
Attachment #192247 - Flags: superreview?(dbaron)
Attachment #192247 - Flags: review?(dbaron)
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 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.
Attachment #192247 - Flags: superreview?(dbaron)
Attachment #192247 - Flags: superreview+
Attachment #192247 - Flags: review?(dbaron)
Attachment #192247 - Flags: review+
Flags: blocking1.7.12?
Created attachment 192278 [details] [diff] [review]
Patch rev. 2

More robust version.
Attachment #192247 - Attachment is obsolete: true
Attachment #192278 - Flags: superreview?(dbaron)
Attachment #192278 - Flags: review?(dbaron)
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
Attachment #192278 - Flags: superreview?(dbaron)
Attachment #192278 - Flags: superreview+
Attachment #192278 - Flags: review?(dbaron)
Attachment #192278 - Flags: review+
Attachment #192278 - Flags: approval1.8b4?
Attachment #192247 - Attachment is obsolete: false
Attachment #192247 - Flags: approval1.7.12?
Flags: blocking-aviary1.0.7?
Whiteboard: [sg:fix]
Group: security
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 on attachment 192278 [details] [diff] [review]
Patch rev. 2

a=shaver, please commit to trunk today before branch.
Attachment #192278 - Flags: approval1.8b4? → approval1.8b4+
(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
Created attachment 192415 [details] [diff] [review]
More checks

Let me know if you want any of these additional checks.
Attachment #192415 - Flags: review?(dbaron)
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.
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 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.
Attachment #192415 - Flags: review?(dbaron) → review+
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.
Attachment #192278 - Attachment is obsolete: true
Created attachment 194399 [details] [diff] [review]
Patch rev. 1 + additional checks/assertions

Here's the smaller change (Patch rev. 1) + the extra checks/assertions.
Attachment #192247 - Attachment is obsolete: true
Attachment #194399 - Flags: approval1.7.12?
Attachment #194399 - Flags: approval-aviary1.0.7?
Attachment #192247 - Flags: approval1.7.12?
-> FIXED
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Keywords: fixed1.8
Resolution: --- → FIXED
Keywords: crash
Verified FIXED on trunk using build 2005-09-01-05 SeaMonkey trunk on Windows XP.
Status: RESOLVED → VERIFIED
verified on Firefox 1.4 -mozilla1.8 branch- Win, Lin and Mac : 2005-09-07
Keywords: fixed1.8 → verified1.8
Flags: blocking1.7.12?
Flags: blocking1.7.12+
Flags: blocking-aviary1.0.7?
Flags: blocking-aviary1.0.7+

Updated

12 years ago
Flags: blocking-aviary1.0.7+

Updated

12 years ago
Flags: blocking-aviary1.0.7+ → blocking-aviary1.0.7?
Flags: blocking1.7.12?

Updated

12 years ago
Attachment #194399 - Flags: approval1.7.13?
Attachment #194399 - Flags: approval-aviary1.0.8?
Attachment #194399 - Flags: approval-aviary1.0.7+

Updated

12 years ago
Flags: blocking1.7.12?
Flags: blocking1.7.12+
Flags: blocking-aviary1.0.7?
Flags: blocking-aviary1.0.7+

Updated

12 years ago
Attachment #194399 - Flags: approval1.7.12+
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.
Checked in to MOZILLA_1_7_BRANCH and AVIARY_1_0_1_2005124_BRANCH.
Keywords: fixed-aviary1.0.7, fixed1.7.12
Attachment #192278 - Flags: approval1.7.12+
Attachment #192278 - Flags: approval-aviary1.0.7+
Attachment #194399 - Flags: approval1.7.12-
Attachment #194399 - Flags: approval1.7.12+
Attachment #194399 - Flags: approval-aviary1.0.7-
Attachment #194399 - Flags: approval-aviary1.0.7+
Flags: blocking1.7.13+
Flags: blocking-aviary1.0.8+
Group: security
test is 404.
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.