468771 - (CVE-2009-3981) "ASSERTION: the pointer to this sibling will be overwritten" with -moz-column, table

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: the pointer to this sibling will be overwritten: '!aNewFrame->GetNextSibling()', file /Users/jruderman/central/layout/generic/nsFrameList.cpp, line 176

###!!! ASSERTION: Some objects allocated with AllocateFrame were not freed: 'mFrameCount == 0', file /Users/jruderman/central/layout/base/nsPresShell.cpp, line 675

Likely exploitable.  The testcase doesn't cause a crash, but a variant (created with the usual trick) makes Firefox dereference 0xdadadaf6.

Comment 1

[Bernd]

The usual suspects, the block reflow reports incompleteness, the table starts to split, but as it is not in pagination mode, this goes wrong.

Comment 2

[Bernd]

the crash is preceeded by:
###!!! ASSERTION: Shouldn't be incomplete if availableHeight is UNCONSTRAINED.:
'aReflowState.availableHeight != NS_UNCONSTRAINEDSIZE', file d:/moz_src/src/layo
ut/generic/nsBlockFrame.cpp, line 1413

Comment 3

[Bernd]

Attachment: testcase for the incomplete assert

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Should be fairly easy to fix once we figure out where the incompleteness is being triggered.

Comment 5

[Bernd]

we trigger it by asking to reisze to a very small width of the column frame

ColumnSet(div)(0) 05CCE828 Reflow a=60,UC c=60,UC h-resize v-resize cnt=10999
            block 05CCE774 Reflow a=60,1200 c=60,UC dirty v-resize nif=05CCF8A4 cnt=11000
             text 05CCE9F0 Reflow a=60,UC c=UC,UC dirty v-resize nif=05CCF838 cnt=11001
             text 05CCE9F0 Reflow d=600,1140 status=0x1
             text 05CCF838 Reflow a=60,UC c=UC,UC dirty v-resize pif=05CCE9F0 cnt=11002
             text 05CCF838 Reflow d=660,1140
            block 05CCE774 Reflow d=60,1200 status=0x3o=(0,0) 600 x 1200sto=(0,0) 600 x 1200
           ColumnSet(div)(0) 05CCE828 Reflow d=60,1200 status=0x3o=(0,0) 600 x 1200sto=(0,0) 600 x 1200
###!!! ASSERTION: Shouldn't be incomplete if availableHeight is UNCONSTRAINED.:'aReflowState.availableHeight != NS_UNCONSTRAINEDSIZE', file d:/moz_src/src/layout/generic/nsBlockFrame.cpp, line 1413

Comment 6

[Daniel Holbert [:dholbert]]

FWIW: I hit the assertions from comment 0 and comment 2 on the first testcase, and the incomplete assert on the second testcase, using my linux mozilla-central debug build (updated yesterday).

Platform --> ALL/ALL

Comment 7

[Daniel Holbert [:dholbert]]

FWIW, the testcase on bug 435664 triggers this bug's assertion ("the pointer to this sibling will be overwritten") when printed / print-previewed.  (It also crashes, which is what that bug is primarily about.)  See bug 435664 comment 13.  Possibly related to this bug?

Comment 8

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

I think the problem here is that the column set is propagating incompleteness that's ok inside of it to outside of it... although maybe incompleteness isn't ok inside the last column of a column set.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

The comment should help explain what's going on.

Basically we're just changing the number of columns from 2 to 1. This should mean that the first column pulls all the content out of the second column, but that doesn't happen because we skip reflowing the first column. We need to tweak the incremental reflow test so that we always reflow the last column if we're giving it unbounded height; it will want to pull content from its next-sibling.

Comment 10

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

Comment on attachment 361492 [details] [diff] [review]
fix

r+sr=dbaron

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This fixes bug 476579 too.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Pushed http://hg.mozilla.org/mozilla-central/rev/c0dbc2a40bb1. I withheld the testcase.

Comment 14

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

Comment on attachment 361492 [details] [diff] [review]
fix

a1.9.1=dbaron

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/6f907ad90647

Comment 16

[Daniel Veditz [:dveditz]]

Although the assertion doesn't occur on the 1.9.0 branch the code being patched looks the same. Do we need this fix on the 1.9.0 branch as well?

Comment 17

[Daniel Veditz [:dveditz]]

Blocking 1.9.0 on the assumption the answer to comment 16 is "yes, we do".

Comment 18

[Daniel Veditz [:dveditz]]

Code-freeze for 1.9.0.16 is next Tuesday. If this patch works as-is please request approval, else we need a back-port patch. Thanks.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 361492 [details] [diff] [review]
fix

Patch applies as-is.

Comment 20

[Daniel Veditz [:dveditz]]

Comment on attachment 361492 [details] [diff] [review]
fix

Approved for 1.9.0.16, a=dveditz for release-drivers

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Checked into 1.9.0.

Comment 22

[Al Billings [:abillings - ex-MoCo]]

Neither of the two testcases trigger the assert in my debug 1.9.0.15 build on OS X. Have we seen these asserts on the 1.9.0 branch?

Comment 23

[Jesse Ruderman]

Crashtest added: http://hg.mozilla.org/mozilla-central/rev/d3e7c18f58fc