Closed
Bug 317334
(onload=window())
Opened 18 years ago
Closed 17 years ago
hang when long wrappable string is passed to prompt() [e.g. as used in the exploit for IE's <body onload=window()> bug]
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.8.1beta2
People
(Reporter: bernd_mozilla, Assigned: martijn.martijn)
References
(Depends on 1 open bug, )
Details
(Keywords: fixed1.8.1, hang, perf, Whiteboard: [reflow-refactor])
Attachments
(10 files, 4 obsolete files)
358 bytes,
text/html
|
Details | |
7.24 KB,
image/png
|
Details | |
4.29 KB,
image/png
|
Details | |
358 bytes,
text/html
|
Details | |
3.59 KB,
image/png
|
Details | |
338 bytes,
text/html
|
Details | |
1.09 KB,
patch
|
Details | Diff | Splinter Review | |
3.81 KB,
patch
|
mconnor
:
review+
darin.moz
:
approval1.8.1+
|
Details | Diff | Splinter Review |
3.70 KB,
patch
|
mconnor
:
review+
|
Details | Diff | Splinter Review |
2.75 KB,
patch
|
enndeakin
:
review+
beltzner
:
approval1.8.1+
|
Details | Diff | Splinter Review |
load the testcase and hit one selection it hanges ff see http://www.computerterrorism.com/research/ie/ct21-11-2005 for the details advertised at http://www.heise.de/newsticker/meldung/66480
Assignee | ||
Comment 1•18 years ago
|
||
It happens because this page is opened in an iframe in the new popup: http://www.computerterrorism.com/research/ie/fillmem.htm
Assignee | ||
Comment 2•18 years ago
|
||
Specifically, it's this part of the script at that page: // // Address called by the bug (also serves as slide code) // for (spearson=1 ; spearson <=500 ; spearson++) { eip = eip + unescape("%u7030%u4300") //eip = eip + unescape("%u4300") } // // Create a large chunk for memory saturation // for (spearson=1 ; spearson <=200; spearson++) { fillmem = fillmem + eip } This creates an enormous string, which makes Mozilla hang and apparently makes IE6 exploitable.
Comment 3•18 years ago
|
||
I can confrim that Firefox hangs up at: http://www.computerterrorism.com/research/ie/poc.htm and http://www.computerterrorism.com/research/ie/fillmem.htm but not with the heise testcases. Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8) Gecko/20051107 Firefox/1.5
Assignee | ||
Comment 4•18 years ago
|
||
*** Bug 317353 has been marked as a duplicate of this bug. ***
Comment 5•18 years ago
|
||
(In reply to comment #3) > I can confrim that Firefox hangs up at: NB: it's not a permanent hang, we do eventually recover (couple of minutes for me on WinXP) and display the prompt dialog.
Comment 6•18 years ago
|
||
if you interrupt the busy state in a debugger we're busy in layout trying to display the prompt(). Usually in some form of Reflow(), sometimes in font stuff, sometimes in Bidi (nsBidiPresUtils::RemoveBidiContinuation?).
Comment 7•18 years ago
|
||
*** Bug 317358 has been marked as a duplicate of this bug. ***
![]() |
||
Comment 8•18 years ago
|
||
Note that with a GTK1 build (which is what my profiling build is) all our time is just spent messing with fonts... I doubt that's the issue on Windows, though.
![]() |
||
Comment 9•18 years ago
|
||
OK, with a GTK2 build I get something more like: Total hit count: 340722 Count %Total Function Name 151410 44.4 nsBlockFrame::DoRemoveFrame(nsIFrame*, int) 13795 4.0 SearchTable 7655 2.2 FcCharSetDestroy 7410 2.2 nsBlockFrame::TryAllLines(nsLineList_iterator*, nsLineList_iterator*, int*) 4656 1.4 PL_DHashTableOperate with the calls to DoRemoveFrame coming mostly from nsBidiPresUtils::RemoveBidiContinuation, called from nsBidiPresUtils::Resolve. About 40% of total time is spent under Resolve (most in DoRemoveFrame). I broke in Resolve, and the 3rd or 4th time it's called I get: 200 mSuccess = InitLogicalArray(aPresContext, aFirstChild, nsnull, PR_TRUE); (gdb) next p202 if (text->mUnicodeBidi == NS_STYLE_UNICODE_BIDI_OVERRIDE) { (gdb) p mLogicalFrames.Count() $26 = 66709 Going forward some more: 238 mSuccess = mBidiEngine->CountRuns(&runCount); (gdb) next 239 if (NS_FAILED(mSuccess) ) { (gdb) p runCount $27 = 5 So I'm guessing we have that many frames because of line-breaking or something? Seems like a lot for a 200111-char string (which is what we have). In any case, I bet that whatever order we're removing them in makes all this O(N^2) in the number of frames or something like that...
![]() |
||
Comment 10•18 years ago
|
||
For what it's worth, when we call nsBidiPresUtils::RemoveBidiContinuation, we call it as: Breakpoint 3, nsBidiPresUtils::RemoveBidiContinuation (this=0x8abcc28, aPresContext=0x88b2a80, aFrame=0xa1c6934, aFirstIndex=4, aLastIndex=66708, aOffset=@0xbfffb084) at ../../../mozilla/layout/base/nsBidiPresUtils.cpp:774 Note aLastIndex and aFirstIndex. Then we proceed to remove those frames from the block, starting at the end (why at the end?). That's the Really Slow way to do it given the linked-list storage for frames, no? Is there a reason not to change the iteration order?
![]() |
||
Comment 11•18 years ago
|
||
So yeah, switching the iteration order makes the profile look like: Total hit count: 220757 Count %Total Function Name 17914 8.1 SearchTable 8595 3.9 FcCharSetDestroy 5983 2.7 PL_DHashTableOperate 5227 2.4 nsStyleContext::GetStyleData(nsStyleStructID) 4858 2.2 nsTextFrame::MeasureText(nsPresContext*, nsHTMLReflowState const&, nsTextTransformer&, nsTextStyle&, nsTextFrame::TextReflowData&) Too bad I crash at some point before that dialog can come up... ;) But that may be because I screwed up the order-reversing, of course.
Assignee | ||
Comment 12•18 years ago
|
||
*** Bug 317373 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 13•18 years ago
|
||
*** Bug 317394 has been marked as a duplicate of this bug. ***
Comment 14•18 years ago
|
||
BTW: On a related note, in earlier builds it only took ~20 seconds to display the prompt, now it takes ~90 seconds here (also the memory usage is much higher, 30MB vs. 70MB). I narrowed that one down to the time range 2005-10-15 to 2005-10-16. Bonsai link would be http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=MozillaTinderboxAll&branch=HEAD&branchtype=match&filetype=match&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-01-15+04%3A00%3A00&maxdate=2005-01-16+07%3A00%3A00&cvsroot=%2Fcvsroot might be related here (i mean 20 seconds to display this would still be much better than 90 seconds ;).
Comment 15•18 years ago
|
||
I meant 2005-01-15 and 2005-01-16 of course (it's correct in the bonsai link).
![]() |
||
Comment 16•18 years ago
|
||
I'll bet on the SizeToContent fix in that range; if you want to test by local backout, please do. That said, 1) The real thing to do is to fix the perf issues 2) Displaying a 200,000-character string is never going to get particularly snappy.
Updated•18 years ago
|
Alias: onload=window()
Updated•18 years ago
|
Summary: hang with IE exploit → hang with IE exploit [body onload=window()]
Comment 17•18 years ago
|
||
(In reply to comment #11) > Too bad I crash at some point before that dialog can come up... ;) But that > may be because I screwed up the order-reversing, of course. As the code stands, just switching the iteration order is going to crash because RemoveFrame() deletes all of the frame's next-in-flows, so as we go on iterating through mLogicalFrames we get pointers to frames that have already been deleted.
![]() |
||
Comment 18•18 years ago
|
||
OK. So what's actually going on here? I assume not all the frames in mLogicalFrames are in-flows of each other, but some are? Can we just leave things that are not first in flow out of mLogicalFrames here? Or filter them out, or something? Alternately, why are we ending up with 60000-some frames here anyway? Should we be?
Comment 19•18 years ago
|
||
I'll look into this some more tomorrow. As far as I can tell so far, bidi resolving isn't creating the 6000+ frames, but I'm not sure who is. Line breaking?
![]() |
||
Comment 20•18 years ago
|
||
Line breaking is possible, yes... But why are we line-breaking every 3 chars, then?
Reporter | ||
Comment 21•18 years ago
|
||
Reporter | ||
Comment 22•18 years ago
|
||
Reporter | ||
Comment 23•18 years ago
|
||
so we reflow the text once it has these chars to death, already one char is 6 times reflown if it needs to break it reflows the whole thing multiple times
Reporter | ||
Comment 24•18 years ago
|
||
Reporter | ||
Comment 25•18 years ago
|
||
so it looks that the range of the unichar defines whether we wrap the text and start that "insane" linebreaking.
Comment 26•18 years ago
|
||
(In reply to comment #16) > I'll bet on the SizeToContent fix in that range; if you want to test by local > backout, please do. Yeah, that was it. Without that patch, it takes 15 seconds.
![]() |
||
Comment 27•18 years ago
|
||
Without that fix I bet we never do that second reflow (the one where we have 60000 frames).
Comment 28•18 years ago
|
||
FF 1.5 OSX eats all available CPU and hangs with a Spinning Beachball of Doom.
Severity: normal → major
OS: Windows XP → All
Hardware: PC → All
Updated•18 years ago
|
Severity: major → normal
Hardware: All → PC
Reporter | ||
Comment 29•18 years ago
|
||
the MEW is differently computed for both testcases, so we think we can wrap the text. area 0320D108 r=0 a=0,UC c=0,0 cnt=6246 text 0320D1AC r=0 a=UC,UC c=UC,UC cnt=6247 text 0320D1AC d=150000,195 me=75 <------- causes hang text 0320D1AC r=2 a=0,UC c=UC,UC cnt=6248 area 0323FFE8 r=0 a=0,UC c=0,0 cnt=6182 text 0324008C r=0 a=UC,UC c=UC,UC cnt=6183 text 0324008C d=300000,195 me=300000 text 0324008C r=2 a=0,UC c=UC,UC cnt=6184 text 0324008C d=300000,195 me=300000 area 0323FFE8 d=300000,0 me=300000 m=300000 o=(0,0) 300000 x 195
Reporter | ||
Comment 30•18 years ago
|
||
this testcases causes as much reflows as the first one
Reporter | ||
Comment 31•18 years ago
|
||
I think we hit the usual xul reflow is inefficient bug (bug 260191 for instance) in order to get the prefsize we reflow like http://lxr.mozilla.org/seamonkey/source/layout/xul/base/src/nsBoxFrame.cpp#1004 with a zero width as the reflow log shows to get the minimum width. Then the text needs to wrap. Somehow the initial text seems to be wrappable, but the attachment 203965 [details] shows that, if the text is definetely wrappable we will wrap the text at every possible place to come as close as possible to the 0 available width. Looks like [reflow refactor].
Reporter | ||
Comment 32•18 years ago
|
||
or in other words: we hit http://lxr.mozilla.org/seamonkey/source/layout/generic/nsFrame.cpp#4748 // 3) Minimum size. This is a toughy. We can pass the block a flag asking for the max element // size. That would give us the width. Unfortunately you can only ask for a maxElementSize // during an incremental reflow. So on other reflows we will just have to use 0. // The min height on the other hand is fairly easy we need to get the largest // line height. This can be done with the line iterator. I believe this is wrong
![]() |
||
Comment 33•18 years ago
|
||
So perhaps we should start filing separate bugs blocking this? I'd still like to see what we can do about the bidi stuff here, for example...
Whiteboard: [reflow-refactor]
Updated•18 years ago
|
Keywords: perf
Summary: hang with IE exploit [body onload=window()] → hang when long wrappable string is passed to prompt() [e.g. as used in the exploit for IE's <body onload=window()> bug]
Comment 34•18 years ago
|
||
(In reply to comment #31) > Somehow the initial text seems to be wrappable I'm not sure if there is a doubtful tone here, but the initial text is legitimately wrappable between any pair of ideographic characters, i.e. just about everywhere.
Comment 35•18 years ago
|
||
(In reply to comment #18) > Can we just leave > things that are not first in flow out of mLogicalFrames here? Or filter them > out, or something? It's possible that parent->RemoveFrame() isn't the ideal way to do this anyway. We might be better off writing something more purpose-built that just goes through reconnecting the bidi continuations. > Alternately, why are we ending up with 60000-some frames here anyway? Should > we be? It would also be interesting to try a similar testcase that creates a huge string of alternate left-to-right and right-to-left characters.
Comment 36•18 years ago
|
||
*** Bug 317573 has been marked as a duplicate of this bug. ***
Comment 37•18 years ago
|
||
*** Bug 317578 has been marked as a duplicate of this bug. ***
Comment 38•18 years ago
|
||
*** Bug 317587 has been marked as a duplicate of this bug. ***
Reporter | ||
Comment 39•18 years ago
|
||
Comment 40•18 years ago
|
||
That patch improved it by 32% :) (before it was ~95 seconds, with the patch it's ~65 seconds).
Comment 41•18 years ago
|
||
*** Bug 317799 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 42•18 years ago
|
||
*** Bug 318109 has been marked as a duplicate of this bug. ***
Comment 43•18 years ago
|
||
*** Bug 318147 has been marked as a duplicate of this bug. ***
Comment 44•18 years ago
|
||
*** Bug 318144 has been marked as a duplicate of this bug. ***
Comment 45•18 years ago
|
||
*** Bug 318668 has been marked as a duplicate of this bug. ***
Comment 46•18 years ago
|
||
Has this dropped through the cracks? Exploits based on the CounterTerrorism PoC have shown up in the wild (on warez sites and the like).
Flags: blocking1.8.0.1?
Reporter | ||
Comment 47•18 years ago
|
||
I can't say what is the plan for the bidi stuff, but the analysis, that I have done points to refactoring of the xul reflow. This is exactly what the reflow branch is supposed to accomplish. This is what the status whiteboards means. It also defines a timetable which does not match 1.8.0.1. My interpretation is that xul reflow triggers repeatedly a weakness of bidiframes. This can be fixed at two levels: xul reflow or bidi frames. XUL reflow is close to an f-word for me, I don't understand what all the voodoo here does but from the reflow logs its easy to see that it is bad. So while I prefer the small tweaks, I can't say what is right and wrong for xul. Neither do I know how to test xul reflow properly other than to hork the UI and get feedback. This is not an option for 1.8.0.1.
Comment 48•18 years ago
|
||
*** Bug 320760 has been marked as a duplicate of this bug. ***
Comment 49•18 years ago
|
||
*** Bug 320348 has been marked as a duplicate of this bug. ***
![]() |
||
Comment 50•18 years ago
|
||
Daniel, this bug is basically about 1.9-type performance work. It's not a security bug as far as I can see; just another performance issue (no more or less serious than the existing performance issues we have). I see no reason this should block 1.8.anything, nor do I see a way to "fix" any of this that would be acceptable for the 1.8 branch. Of course see comment 33. If we break this down into subproblems and come up with safe fixes to some of them, great.
Comment 51•18 years ago
|
||
(In reply to comment #50) > Daniel, this bug is basically about 1.9-type performance work. Maybe now. It started with a particular IE exploit that "hangs" Firefox/Mozilla (temporarily) and which is now found in the wild. Because real people are stumbling across actual instances of this any partial fix that helps would be welcome. But I'll bow to reality and remove the 1.8.0.1 nomination.
Flags: blocking1.8.0.1? → blocking1.8.1?
![]() |
||
Comment 52•18 years ago
|
||
OK, but 319930 covers the Bidi performance issues. bernd, do you want to file a bug blocking this one on the wrapping and min-width issue?
![]() |
||
Comment 53•18 years ago
|
||
I meant bug 319930
Comment 54•18 years ago
|
||
Would limiting the amount of text we allow in a prompt() call to something reasonable make the performance acceptable? Is enough text such that the prompt is unusable needed to trigger the bad performance problem?
Assignee | ||
Comment 55•18 years ago
|
||
So something like this? This creates a limit of 10000 chars for alerts, prompts, etc.
Comment 56•18 years ago
|
||
Let's try to get Martijn's patch reviewed, 10k chars in a prompt is a sane limit.
Assignee: nobody → martijn.martijn
Flags: blocking1.8.1? → blocking1.8.1+
Comment 57•18 years ago
|
||
Comment on attachment 226446 [details] [diff] [review] limit of 10000 chars Martijn, can you land this ASAP on trunk?
Attachment #226446 -
Flags: review+
Assignee | ||
Comment 58•18 years ago
|
||
Checking in toolkit/content/commonDialog.xul; /cvsroot/mozilla/toolkit/content/commonDialog.xul,v <-- commonDialog.xul new revision: 1.8; previous revision: 1.7 done Checking in toolkit/content/xul.css; /cvsroot/mozilla/toolkit/content/xul.css,v <-- xul.css new revision: 1.77; previous revision: 1.76 done Ok, I've checked the "limit of 10000 chars" in the trunk. But I'm not marking this bug fixed, because it's more a workaround for this bug than anything else.
Comment 59•18 years ago
|
||
Comment on attachment 226446 [details] [diff] [review] limit of 10000 chars Checked in on trunk since June 28 with no regressions pointing to it. I think we want wider (read: beta) testing on this one ...
Attachment #226446 -
Flags: approval1.8.1?
Updated•18 years ago
|
Target Milestone: --- → mozilla1.8.1beta1
Comment 60•18 years ago
|
||
The patch contains the following change: -<?xml version="1.0"?> +<?xml version="1.0"?> What's that all about?
Updated•18 years ago
|
Whiteboard: [reflow-refactor] → [reflow-refactor][checkin needed]
Comment 61•18 years ago
|
||
Comment on attachment 226446 [details] [diff] [review] limit of 10000 chars a=darin on behalf of drivers (sans-garbage in patch)
Attachment #226446 -
Flags: approval1.8.1? → approval1.8.1+
Assignee | ||
Comment 62•18 years ago
|
||
Argh! I can see that garbage in the trunk too. It needs to be removed. Sorry about that.
Assignee | ||
Comment 63•18 years ago
|
||
Checking in commonDialog.js; /cvsroot/mozilla/toolkit/content/commonDialog.js,v <-- commonDialog.js new revision: 1.9.2.4; previous revision: 1.9.2.3 done Checking in commonDialog.xul; /cvsroot/mozilla/toolkit/content/commonDialog.xul,v <-- commonDialog.xul new revision: 1.6.2.2; previous revision: 1.6.2.1 done Checking in xul.css; /cvsroot/mozilla/toolkit/content/xul.css,v <-- xul.css new revision: 1.61.2.14; previous revision: 1.61.2.13 done Checked "limit of 10000 chars" patch in on 1.8.1 branch (without the garbage). And I removed the garbage from trunk.
Keywords: fixed1.8.1
Whiteboard: [reflow-refactor][checkin needed] → [reflow-refactor]
Comment 64•18 years ago
|
||
The checkins in 1.8_Branch leads to a regression: Bug 344045
Updated•18 years ago
|
Assignee | ||
Comment 65•18 years ago
|
||
I've backed out my patch on the 1.8.1 branch, so bug 344021 and bug 344045 should be fixed now. There is clearly an issue with patch, which wasn't happening with the trunk build, because I somehow forgot to check in the commonDialog.js part of the patch into trunk somehow :( See comment 58.
Assignee | ||
Updated•18 years ago
|
Keywords: fixed1.8.1
Assignee | ||
Comment 66•18 years ago
|
||
Ok, this still needs to be checked in on trunk to have the workaround working. Instead of adding the rule in xul.css, maybe it is better to add the css rule as an inline style in commonDialog.xul?
Assignee | ||
Updated•18 years ago
|
Attachment #228621 -
Flags: review?(mconnor)
Assignee | ||
Comment 67•18 years ago
|
||
Gavin and I just agreed that it is better to use an inline style here.
Attachment #228622 -
Flags: review?(mconnor)
Assignee | ||
Comment 68•18 years ago
|
||
And now without the unnecessary garbage that I keep getting for some reason. Sorry for all the spam.
Attachment #228621 -
Attachment is obsolete: true
Attachment #228622 -
Attachment is obsolete: true
Attachment #228629 -
Flags: review?(mconnor)
Attachment #228621 -
Flags: review?(mconnor)
Attachment #228622 -
Flags: review?(mconnor)
Updated•18 years ago
|
Target Milestone: mozilla1.8.1beta1 → mozilla1.8.1beta2
Comment 69•17 years ago
|
||
Actually the perf issue has already been fixed on the reflow branch...
Assignee | ||
Comment 70•17 years ago
|
||
Ok, this patch has the following comment added: + // XXX the substr(0, 10000) part is a workaround for bug 317334 So it makes it clear that that part could be backed out when this bug is fixed.
Attachment #228629 -
Attachment is obsolete: true
Attachment #229797 -
Flags: review?(mconnor)
Attachment #228629 -
Flags: review?(mconnor)
Updated•17 years ago
|
Attachment #229797 -
Flags: review?(mconnor) → review+
Assignee | ||
Comment 71•17 years ago
|
||
"rest of patch that needs to be on trunk" checked in on trunk.
Assignee | ||
Comment 72•17 years ago
|
||
This is equivalent of what went into trunk.
Attachment #232113 -
Flags: review?(mconnor)
Comment 73•17 years ago
|
||
Comment on attachment 232113 [details] [diff] [review] branch patch >+<?xml version="1.0"?> Yick, this won't work out so well. Can you fix the patch to get rid of this goo?
Assignee | ||
Comment 74•17 years ago
|
||
> Yick, this won't work out so well. Can you fix the patch to get rid of this
> goo?
Sorry about that :(
Attachment #232113 -
Attachment is obsolete: true
Attachment #233979 -
Flags: review?(mconnor)
Attachment #232113 -
Flags: review?(mconnor)
Comment 75•17 years ago
|
||
Comment on attachment 233979 [details] [diff] [review] branch patch Looks OK
Attachment #233979 -
Flags: review?(mconnor) → review+
Assignee | ||
Updated•17 years ago
|
Attachment #233979 -
Flags: approval1.8.1?
Comment 76•17 years ago
|
||
Comment on attachment 233979 [details] [diff] [review] branch patch a=beltzner on behalf of drivers, for landing on the MOZILLA_1_8_BRANCH. Please mark fixed1.8.1 when it lands.
Attachment #233979 -
Flags: approval1.8.1? → approval1.8.1+
Updated•17 years ago
|
Whiteboard: [reflow-refactor] → [reflow-refactor][checkin needed (1.8.1 branch)]
Comment 77•17 years ago
|
||
Checked in to 1.8 branch on behalf of author.
Status: NEW → RESOLVED
Closed: 17 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Whiteboard: [reflow-refactor][checkin needed (1.8.1 branch)] → [reflow-refactor]
You need to log in
before you can comment on or make changes to this bug.
Description
•