Closed
Bug 296134
Opened 19 years ago
Closed 19 years ago
Crash on unicode "zero width non-joiner" sequence
Categories
(Core :: Internationalization, defect)
Core
Internationalization
Tracking
()
VERIFIED
FIXED
People
(Reporter: davemcw, Assigned: MatsPalmgren_bugz)
References
()
Details
(5 keywords, Whiteboard: [sg:fix])
Attachments
(4 files, 4 obsolete files)
1.49 KB,
text/plain
|
Details | |
8.96 KB,
text/plain
|
Details | |
4.28 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
3.72 KB,
patch
|
dbaron
:
approval-aviary1.0.7-
dbaron
:
approval1.7.12-
|
Details | Diff | Splinter Review |
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•19 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
Comment 2•19 years ago
|
||
I haven't been able to reproduce this yet in either trunk or branch.
Whiteboard: stackwanted
Comment 3•19 years ago
|
||
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•19 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•19 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•19 years ago
|
||
Not a blocker, but will consider an approved fix if the risk is low.
Flags: blocking1.8b4? → blocking1.8b4-
Comment 8•19 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?
Comment 10•19 years ago
|
||
Asa, we should have a blocking1.8 flag, shouldn't we? /be
Updated•19 years ago
|
Flags: blocking1.8b4? → blocking1.8b4+
Assignee | ||
Comment 11•19 years ago
|
||
The problem is an infinite loop in 'morkParser::ReadValue' and it causes calls to 'morkBlob::GrowBlob' with increasing sizes until we crash...
Assignee | ||
Comment 13•19 years ago
|
||
We are stuck in frame #5
Assignee | ||
Comment 14•19 years ago
|
||
Notice the "unget $" at the beginning of the sequence, it only occurs there.
Assignee | ||
Comment 15•19 years ago
|
||
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•19 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•19 years ago
|
||
Mats, I don't see how the first issue reported and its url could be related to your stack trace.
Assignee | ||
Comment 18•19 years ago
|
||
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
Assignee | ||
Comment 19•19 years ago
|
||
(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 | ||
Updated•19 years ago
|
Assignee: smontagu → mats.palmgren
Whiteboard: stackwanted
Assignee | ||
Comment 20•19 years ago
|
||
Note 'aLength=4294967295' in the frames at the top.
Assignee | ||
Comment 21•19 years ago
|
||
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
Assignee | ||
Comment 22•19 years ago
|
||
Attachment #192247 -
Flags: superreview?(dbaron)
Attachment #192247 -
Flags: review?(dbaron)
Assignee | ||
Comment 23•19 years ago
|
||
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?
Assignee | ||
Comment 25•19 years ago
|
||
More robust version.
Attachment #192247 -
Attachment is obsolete: true
Assignee | ||
Updated•19 years ago
|
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+
Assignee | ||
Updated•19 years ago
|
Attachment #192278 -
Flags: approval1.8b4?
Assignee | ||
Updated•19 years ago
|
Attachment #192247 -
Attachment is obsolete: false
Attachment #192247 -
Flags: approval1.7.12?
Updated•19 years ago
|
Flags: blocking-aviary1.0.7?
Whiteboard: [sg:fix]
Updated•19 years ago
|
Group: security
Comment 27•19 years ago
|
||
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+
Assignee | ||
Comment 29•19 years ago
|
||
(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
Assignee | ||
Comment 30•19 years ago
|
||
Let me know if you want any of these additional checks.
Assignee | ||
Updated•19 years ago
|
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.
Assignee | ||
Comment 32•19 years ago
|
||
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+
Assignee | ||
Comment 34•19 years ago
|
||
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.
Assignee | ||
Updated•19 years ago
|
Attachment #192278 -
Attachment is obsolete: true
Assignee | ||
Comment 35•19 years ago
|
||
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?
Assignee | ||
Updated•19 years ago
|
Attachment #192247 -
Flags: approval1.7.12?
Assignee | ||
Comment 36•19 years ago
|
||
-> FIXED
Verified FIXED on trunk using build 2005-09-01-05 SeaMonkey trunk on Windows XP.
Status: RESOLVED → VERIFIED
Comment 38•19 years ago
|
||
verified on Firefox 1.4 -mozilla1.8 branch- Win, Lin and Mac : 2005-09-07
Keywords: fixed1.8 → verified1.8
Updated•19 years ago
|
Flags: blocking1.7.12?
Flags: blocking1.7.12+
Flags: blocking-aviary1.0.7?
Flags: blocking-aviary1.0.7+
Updated•19 years ago
|
Flags: blocking-aviary1.0.7+
Updated•19 years ago
|
Flags: blocking-aviary1.0.7+ → blocking-aviary1.0.7?
Flags: blocking1.7.12?
Updated•19 years ago
|
Attachment #194399 -
Flags: approval1.7.13?
Attachment #194399 -
Flags: approval-aviary1.0.8?
Attachment #194399 -
Flags: approval-aviary1.0.7+
Updated•19 years ago
|
Flags: blocking1.7.12?
Flags: blocking1.7.12+
Flags: blocking-aviary1.0.7?
Flags: blocking-aviary1.0.7+
Updated•19 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+
Updated•19 years ago
|
Flags: blocking1.7.13+
Flags: blocking-aviary1.0.8+
Updated•19 years ago
|
Group: security
You need to log in
before you can comment on or make changes to this bug.
Description
•