Loading the testcase triggers an assertion and a crash. ###!!! ASSERTION: Must only be called on reflowed lines: '!(GetStateBits() & NS_FRAME_IS_DIRTY)', file /Users/jruderman/central/layout/generic/nsFrame.cpp, line 4071 Crash at one of the following: * [@ nsFrameManager::CaptureFrameStateFor] * [@ nsContainerFrame::PositionChildViews] * [@ nsFrameList::CheckForLoops] Some of the crashes look exploitable.

The assert is coming from a CachedIsEmpty call. The frame tree is: Canvas(html)(-1)@0x142d184 [view=0xcba5570] {0,0,0,0} [state=00002403] [content=0xcacdf30] [sc=0x144c864] pst=:-moz-scrolled-canvas< Area(html)(-1)@0x144cb94 {0,0,0,0} [state=00d01401] sc=0x144c98c(i=0,b=1)< line 0x144cf90: count=1 state=block,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x4048] {0,0,0,0} < Block(body)(1)@0x144cf38 {0,0,0,0} [state=00101401] sc=0x144cae8(i=1,b=0)< line 0x144da04: count=3 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0xc000] {0,0,0,0} < Text(0)@0x144cfdc[0,2,T] next=0x144d2a8 {0,0,0,0} [state=00200000] [content=0xcba5450] sc=0x144ce98 pst=:-moz-non-element< "\n\n" > Placeholder(div)(1)@0x144d2a8 {0,0,0,0} [state=00000403] outOfFlowFrame=ColumnSet(div)(1)@0x144d174 Text(2)@0x144d9c4[0,2,T] {0,0,0,0} [state=08000402] [content=0xcba5e90] sc=0x144ce98 pst=:-moz-non-element< "\n\n" > > Float-list< ColumnSet(div)(1)@0x144d174 {0,0,0,0} [state=00000523] [content=0xcba49e0] [sc=0x144cee8]< Area(div)(1)@0x144d0c0 next=0x136a238 next-in-flow=0x136a238 {0,0,960,1560} [state=08d00409] [overflow=0,0,960,1920] sc=0x144d1fc(i=0,b=1) pst=:-moz-column-content< line 0x144d99c: count=1 state=block,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x4008] {0,0,960,1560} ca={0,0,960,1920} < Block(div)(0)@0x144d330 next-in-flow=0x144ddb8 {0,0,960,1560} [state=08100409] [overflow=0,0,960,1920] sc=0x144d2e0(i=2,b=1)< line 0x144d924: count=2 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x8340] {960,0,0,0} ca={0,960,960,960} < Placeholder(div)(0)@0x144d500 {960,0,0,0} outOfFlowFrame=Area(div)(0)@0x144d458 Placeholder(div)(1)@0x144d7b0 {960,0,0,0} outOfFlowFrame=Area(div)(1)@0x144d758 > floats < placeholder@0x144d500 Area(div)(0) region={0,0,960,2880} placeholder@0x144d7b0 Area(div)(1) region={960,0,0,1920} > line 0x144d94c: count=1 state=block,clean,prevmargindirty,not impacted,not wrapped,before:leftbr+rightbr,after:nobr[0x4c0a] {0,0,0,0} < Frame(br)(2)@0x144d8b8 {0,0,0,0} [state=00000400] [content=0xcba5d00] > line 0x144d974: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x4200] {0,0,0,0} < Text(3)@0x144d8e4[0,1,T] {0,0,0,0} [state=08600000] [content=0xcba5dc0] sc=0x144d4b0 pst=:-moz-non-element< " " > > Float-list< Area(div)(0)@0x144d458 next=0x144d758 {0,960,960,960} [state=00d00100] sc=0x144d3ac(i=1,b=0)< line 0x144d660: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x4100] {0,0,960,960} < Block(span)(0)@0x144d608 {0,0,960,960} [state=00d00000] sc=0x144d55c(i=0,b=0)< > > > Area(div)(1)@0x144d758 {960,960,0,0} [state=00d00100] sc=0x144d6ac(i=0,b=0)< > > > > > Area(div)(1)@0x136a238 prev-in-flow=0x144d0c0 {1920,0,960,1920} [state=10d00004] sc=0x144d1fc(i=0,b=1) pst=:-moz-column-content< line 0x136a210: count=1 state=block,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x4108] {0,0,960,0} < Block(div)(0)@0x144ddb8 prev-in-flow=0x144d330 {0,0,960,0} [state=10100004] sc=0x144d2e0(i=0,b=0)< > > > > The |this| in CachedIsEmpty() is 0x144d8b8 (the single Frame(br) around). We then crash in CheckForLoops. The reason we crash is that we're at nsBlockFrame::SplitLine and aFrame->GetNextSibling() is a deleted frame. aFrame is the nsPlaceholderFrame at 0x144d7b0. Its GetNextSibling() pointer points to 0x144d8e4 which is gone from the frame tree by this point.

It looks like we do some frame-destroying here from nsBlockFrame::DeleteNextInFlowChild. The place where we destroy the nsTextFrame is under nsLineBox::DeleteLineList, called from nsBlockFrame::Destroy, called from nsBlockFrame::DeleteNextInFlowChild.

And yes, this looks exploitable.

The testcase crashes my Linux nightly build from today, too. Build ID: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b1pre) Gecko/20081001 Minefield/3.1b1pre Crash report: 8a2f2682-900b-11dd-a741-001cc45a2c28 OS/Platform -->All/All

The testcase also crashes Firefox 3.0.3 (after hanging it for ~15 seconds). Adding 'wanted1.9.0.x?' Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.3) Gecko/2008092515 Ubuntu/8.10 (intrepid) Firefox/3.0.3 No crash on Firefox 2, though. Adding 'regression' keyword. Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.17) Gecko/20080829 Firefox/2.0.0.17

This testcase has it structure simplified a bit, and the style separated out into a <style> block. I also substituted divs for the original testcase's <span> and <br> (which had "display: inline-block" and "display: inherit" --> "display: block")

Just for fun, this testcase has 'position: absolute' on the outermost element instead of 'float: left'. It still crashes. (If I make that substitution for the inner floated elements, the crash goes away, though.)

This is the frametree at the time of crash (using the same debugging session from the previously-attached backtrace).

So, I've traced this back a little ways. Basically, we get into a situation where the first part of the frame tree looks like the diagram below: ColumnSet(div)< Block(div) < line < Block < line < Placeholder(div)(0) Placeholder(div)(1) -- my mNextSibling points to Text(3), below > line < Block(div)(2) > > line 0xb04ac290: Text(3) < " " > > In the diagram above, notice that there's a line between the Placeholder and its mNextSibling. It contains an empty block, so it looks harmless, but it's actually a problem. Specifically, it causes problems at some later point when we move the third line into the Overflow-lines list via "PushLine". That function clears out the mNextSibling pointer in the _previous line's final frame_. In this case, that's the wrong mNextSibling pointer to clear -- we'd need to go back *two* lines to get the appropriate mNextSibling pointer. So, I'm assuming that the situation I've diagrammed above should never happen (i.e. a frame and its mNextSibling should never have an unrelated line separating them). I'm now trying to figure out how we get in that situation.

(In reply to comment #10) > In the diagram above, notice that there's a line between the Placeholder and > its mNextSibling. It contains an empty block, so it looks harmless, but it's > actually a problem. If it's not clear: By "It" in "It contains an empty block", I was referring to the second line -- the line that separates the placeholder from its sibling.

Also, if it's not clear: the crash ends up happening because: - PushLine doesn't clear the "correct" mNextSibling pointer, as I described above. - This leaves that pointer dangling. - We dereference this mNextSibling later on, after the text-frame it points to has died.

So I'm not 100% sure this is correct, but it fixes the problem described in comment 10, and it fixes the crash on all 3 testcases. Basically, what was happening is this: Context: We're inside a "Pull data from a next-in-flow" loop @ line 2073. 1. We pull the first line, and we... 1a. add its first child as our last child's nextSibling 1b. append it to our lines list 2. We pull the second line, and we... 2a. add its first child as our last child's nextSibling 2b. append it to our lines list Step 2a effectively snips the first added line's contents out of the "mNextSibling" chain, while leaving it in the line list, separating sibling frames. This gives us the situation described in comment 10. This patch prevents that by updating the aState.mPrevChild pointer, so that in step 2a, we are *append* to the sibling list, rather than *snipping out* the last element as we were doing before.

Comment on attachment 341398 [details] [diff] [review] patch v1 >+ // Add line to our line list, and set its last child as our new prev-child > if (aState.mPrevChild) { > aState.mPrevChild->SetNextSibling(toMove->mFirstChild); >+ aState.mPrevChild = toMove->LastChild(); > } Two questions / thoughts: 1. Should the "aState.mPrevChild = toMove->LastChild();" line be moved below the "if" clause, so it happens unconditionally? I tend to think it should, but I'm not sure if there's anything extra I'd need to do in those situations. (i.e. are there any other variables I'd need to set or tweak when initializing mPrevChild from null to a real value?) 2. Is it possible that toMove is an empty line (meaning LastChild() returns null)? If so, I definitely wouldn't want to put that null value into aState.mPrevChild. If that's a possibility, I'd want to store LastChild() in a temporary variable and null-check it before overwriting mPrevChild.

1. Yes, that should be enough. 2. Up above we checked toMove->GetChildCount() > 0, so it can't be empty here.

(In reply to comment #14) > 1. Should the "aState.mPrevChild = toMove->LastChild();" line be moved below > the "if" clause, so it happens unconditionally? (In reply to comment #15) > 1. Yes, that should be enough. Ok, thanks -- that's fixed in this version. And thanks for the assurance on #2 -- I figured we were probably already checking for that, but I hadn't verified it yet.

BTW: I'm not including crashtests in the patch so as not to give away sample exploit code while users are vulnerable. /me toggles "in-testsuite?" as a reminder to land tests down the line.

Comment on attachment 341409 [details] [diff] [review] patch v1a Patch applies cleanly on 1.9.0.x branch -- requesting approval to land there.

Comment on attachment 341409 [details] [diff] [review] patch v1a Actually, canceling 1.9.0.x approval request, until after this patch lands on trunk (... after it gets post-1.9.1b1 approval, after such a flag is added to bugzilla. :))

'patch v1a' pushed to mozilla-central as changeset 0f35e3ef60f1.

Comment on attachment 341409 [details] [diff] [review] patch v1a Requesting approval to land on branch, now that this is on trunk.

Comment on attachment 341409 [details] [diff] [review] patch v1a Approved for 1.9.0.4, a=dveditz for release-drivers

Here are crashtests for testcases 1-3, to land after this has been released on branch.

'patch v1a' landed on 1.9.0.x branch. Checking in layout/generic/nsBlockFrame.cpp; /cvsroot/mozilla/layout/generic/nsBlockFrame.cpp,v <-- nsBlockFrame.cpp new revision: 3.957; previous revision: 3.956 done

Verified crashing with Firefox 3.0.3 and the fix with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.4pre) Gecko/2008102104 GranParadiso/3.0.4pre.

Verified fixed on the 1.9.1 branch using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090225 Firefox and : Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090225 Shiretoko/3.1b3pre. No crahes with testcase. Verified fixed on the trunk using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090225 Minefield/3.2a1pre and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090225 Minefield/3.2a1pre.