339315 - Crash [@ nsCachedStyleData::GetStyleData] called within nsTableColFrame::GetStyleWidth

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Jesse Ruderman]

Loading the testcase in a ***debug*** build of Firefox (on Mac) causes a crash in nsCachedStyleData.  The crash involves accessing memory at address 0xddddddfd, so this is probably a security hole.

Comment 1

[Jesse Ruderman]

Attachment: testcase

Comment 2

[Jesse Ruderman]

Attachment: stack trace

Comment 3

[Jesse Ruderman]

Thread 0 Crashed:
nsCachedStyleData::GetStyleData(nsStyleStructID const&)
nsStyleContext::GetStyleData(nsStyleStructID)
nsIFrame::GetStyleData(nsStyleStructID) const
nsIFrame::GetStylePosition() const
nsTableColFrame::GetStyleWidth() const
BasicTableLayoutStrategy::AssignNonPctColumnWidths
BasicTableLayoutStrategy::Initialize
nsTableFrame::ReflowTable
nsTableFrame::Reflow
...

Comment 4

[Jesse Ruderman]

A nightly (non-debug) build crashes on a slightly testcase in TableBackgroundPainter::PaintCell.  So bug 339165 is probably the same as this one.

Comment 5

[Bernd]

Attachment: patch

Comment 6

[Bernd]

Attachment: expanded version

this is functional identical to attachment 224595 [details] [diff] [review] but it is less a hack.

Comment 7

[Boris Zbarsky [:bzbarsky]]

So what does this patch actually change?

Comment 8

[Bernd]

Every column has its column index, which indicates where it is in table. Every column group has a start col index which shows the first index a column inside this colgroup should have. If we add or remove a col we need to adjust the colindizes of the following columns. There are two cases we need to shift the index from a given column and all following cols in that colgroup or we need to shift all columns in the colgroup. The problem that we encounter without the patch is that regardless whether we pass a column where we need to start or not we treat the first colgroup special. This is not a issue if we insert cols but hits if remove cols. The patch has one function if we adjust a whole rowgroup (2 args) and one function if we shift inside a rowgroup (additional col frame arg where to start).

Comment 9

[Boris Zbarsky [:bzbarsky]]

I guess what I really don't understand are:

1)  Why are we no longer caling SetStartColumnIndex() on any colgroups?  Is this
    what actually fixes the bug?
2)  Since the two functions are basically identical except for what the first
    value of colFrame is, why are they separate functions instead of a single
    function with a null-check?

Comment 10

[Bernd]

Comment on attachment 224595 [details] [diff] [review]
patch

asking for review on the short version.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 224595 [details] [diff] [review]
patch

Fix the comment to explain what we're doing and why, ok?

Comment 12

[Bernd]

fix checked in

Comment 13

[Daniel Veditz [:dveditz]]

Affects 1.8 branches also

Comment 14

[Bernd]

Attachment: patch against MOZILLA_1_8_BRANCH

Comment 15

[Bernd]

Comment on attachment 231420 [details] [diff] [review]
patch against MOZILLA_1_8_BRANCH

thats the patch with comments that got checked into trunk

Comment 16

[Bernd]

fixed on 1.8.1

Comment 17

[Daniel Veditz [:dveditz]]

Comment on attachment 231420 [details] [diff] [review]
patch against MOZILLA_1_8_BRANCH

approved for 1.8.0 branch, a=dveditz for drivers

Comment 18

[Bernd]

fix checked into 1.8.0 branch

Comment 19

[Bob Clary [:bc] (inactive)]

https://bugzilla.mozilla.org/attachment.cgi?id=223427
ff2b2 release|debug windows/linux/macppc no crash, 
verified fixed 1.8

Comment 20

[Jay Patel [:jay]]

v.fixed on both branches with 8/24 nightly builds, no crash with testcase:
Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.7pre) Gecko/20060824 Firefox/1.5.0.7pre
Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1b2) Gecko/20060824 BonEcho/2.0b2

Comment 21

[Alexander Sack]

attachment 231420 [details] [diff] [review] alone does not fix this testcase nor the testcase from bug 339170 on 1.0.x branch.

I found that the cellmap patch (bug 346980) subsumes this patch, thus backported it completely, but without success.

Bernd et al, any idea what other dependency I might be missing here? The backtrace still looks similar:

#5  0xa5d69990 in nsCachedStyleData::GetStyleData (this=0xddddddf9, aSID=@0xafe59a04)
    at nsRuleNode.h:211
#6  0xa5d8082b in nsStyleContext::GetStyleData (this=0xdddddddd,
    aSID=eStyleStruct_Position) at nsStyleContext.cpp:248
#7  0xa5af191e in nsIFrame::GetStyleData (this=0x8945530, aSID=eStyleStruct_Position)
    at nsIFrame.h:600
#8  0xa5b027f9 in nsIFrame::GetStylePosition (this=0x8945530) at nsStyleStructList.h:82
#9  0xa5c28dab in nsTableColFrame::GetStyleWidth (this=0x8945530)
    at nsTableColFrame.cpp:110
#10 0xa5c193ba in BasicTableLayoutStrategy::AssignNonPctColumnWidths (this=0x8990808,
    aMaxWidth=18960, aReflowState=@0xafe59e10) at BasicTableLayoutStrategy.cpp:1051
#11 0xa5c16d2d in BasicTableLayoutStrategy::Initialize (this=0x8990808,
    aReflowState=@0xafe59e10) at BasicTableLayoutStrategy.cpp:127
#12 0xa5c30165 in nsTableFrame::ReflowTable (this=0x89404a4, aPresContext=0x890f6a0,
    aDesiredSize=@0xafe5a060, aReflowState=@0xafe59e10, aAvailHeight=1073741824,
    aReason=eReflowReason_Resize, aLastChildReflowed=@0xafe59ca4,
    aDoCollapse=@0xafe59d00, aDidBalance=@0xafe59cac, aStatus=@0xafe5a58c)
    at nsTableFrame.cpp:2218
#13 0xa5c2fa4e in nsTableFrame::Reflow (this=0x89404a4, aPresContext=0x890f6a0,
    aDesiredSize=@0xafe5a060, aReflowState=@0xafe59e10, aStatus=@0xafe5a58c)
    at nsTableFrame.cpp:2076
#14 0xa5b0fd0b in nsContainerFrame::ReflowChild (this=0x894034c, aKidFrame=0x89404a4,
    aPresContext=0x890f6a0, aDesiredSize=@0xafe5a060, aReflowState=@0xafe59e10, aX=0,
    aY=0, aFlags=3, aStatus=@0xafe5a58c) at nsContainerFrame.cpp:966
#15 0xa5c451ea in nsTableOuterFrame::OuterReflowChild (this=0x894034c,
    aPresContext=0x890f6a0, aChildFrame=0x89404a4, aOuterRS=@0xafe5a5c0,
    aMetrics=@0xafe5a060, aAvailWidth=18960, aDesiredSize=@0xafe5a0e0,
    aMargin=@0xafe5a0d0, aMarginNoAuto=@0xafe5a0c0, aPadding=@0xafe5a0b0,
    aReflowReason=eReflowReason_Incremental, aStatus=@0xafe5a58c,
    aNeedToReflowCaption=0xafe5a05c) at nsTableOuterFrame.cpp:1333

Comment 22

[Jesse Ruderman]

Crashtest checked in.