Evil xul testcase, using display:-moz-grid-group causes crash [@ nsGridRow::IsCollapsed]

VERIFIED FIXED in mozilla1.8rc1

Status

()

Core
Layout
P1
critical
VERIFIED FIXED
12 years ago
8 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: bz)

Tracking

({fixed1.8, testcase, verified1.7.13})

Trunk
mozilla1.8rc1
fixed1.8, testcase, verified1.7.13
Points:
---
Bug Flags:
blocking1.7.13 +
blocking-aviary1.0.8 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical])

Attachments

(2 attachments)

(Reporter)

Description

12 years ago
See upcoming testcase, which causes a crash when clicking on the button.
Mozilla1.7 is also crashing with the testcase.

Talkback ID: TB10399249G
nsGridRow::IsCollapsed 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/xul/base/src/grid/nsGridRow.cpp,
line 102]
nsGrid::GetPrefRowHeight 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/xul/base/src/grid/nsGrid.cpp,
line 905]
nsGridRowGroupLayout::GetPrefSize 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/xul/base/src/grid/nsGridRowGroupLayout.cpp,
line 128]
nsBoxFrame::GetPrefSize 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/xul/base/src/nsBoxFrame.cpp,
line 948]
nsSprocketLayout::GetPrefSize 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/xul/base/src/nsSprocketLayout.cpp,
line 1346]
nsGridRowGroupLayout::GetPrefSize 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/xul/base/src/grid/nsGridRowGroupLayout.cpp,
line 113]
nsBoxFrame::GetPrefSize 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/xul/base/src/nsBoxFrame.cpp,
line 948]
nsStackLayout::GetPrefSize 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/xul/base/src/nsStackLayout.cpp,
line 95]
nsGridLayout2::GetPrefSize 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/xul/base/src/grid/nsGridLayout2.cpp,
line 151]
nsBoxFrame::GetPrefSize 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/xul/base/src/nsBoxFrame.cpp,
line 948]
nsSprocketLayout::PopulateBoxSizes 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/xul/base/src/nsSprocketLayout.cpp,
line 822]
nsSprocketLayout::Layout 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/xul/base/src/nsSprocketLayout.cpp,
line 265]
nsBoxFrame::DoLayout 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/xul/base/src/nsBoxFrame.cpp,
line 1106]
nsBoxFrame::DoLayout 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/xul/base/src/nsBoxFrame.cpp,
line 1106]
nsRootBoxFrame::Reflow 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/xul/base/src/nsRootBoxFrame.cpp,
line 226]
nsContainerFrame::ReflowChild 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsContainerFrame.cpp,
line 891]
ViewportFrame::Reflow 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsViewportFrame.cpp,
line 239]
IncrementalReflow::Dispatch 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/base/nsPresShell.cpp,
line 859]
PresShell::ProcessReflowCommands 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/base/nsPresShell.cpp,
line 6492]
PresShell::WillPaint 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/base/nsPresShell.cpp,
line 6136]
SHELL32.dll + 0x520c24 (0x778b0c24)
(Reporter)

Comment 1

12 years ago
Created attachment 198952 [details]
testcase
Created attachment 199020 [details] [diff] [review]
Proposed fix

Looks like a logic bug in grid to me.  And the assert change is to just make
them fire when they should (like this case).
Attachment #199020 - Flags: superreview?(roc)
Attachment #199020 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #199020 - Flags: superreview?(roc) → superreview+

Comment 3

12 years ago
Comment on attachment 199020 [details] [diff] [review]
Proposed fix

Quickly plussing this before my eyes glaze over again ;-)
Attachment #199020 - Flags: review?(neil.parkwaycc.co.uk) → review+
Assignee: nobody → bzbarsky
Fixed on trunk.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
OS: Windows XP → All
Priority: -- → P1
Hardware: PC → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.8rc1
Group: security
Comment on attachment 199020 [details] [diff] [review]
Proposed fix

We should probably take this on branch.  The reason we crash here is an
out-of-bounds access on an array, followed by a method call on what we think is
the object there.  So this is probably exploitable...

The fix is quite safe -- the only affected case is when we go from having more
columns than we have explicitly defined to having exactly the right number, and
without this patch this case is guaranteed to lead us to dereference -1 on an
array....  So the only case affected is the case when we have a problem.
Attachment #199020 - Flags: approval1.8rc1?
Flags: blocking1.8rc1?
Flags: blocking1.7.13+
Flags: blocking-aviary1.0.8+
Whiteboard: [sg:critical]
Daniel, should I request 1.7/aviary branch approval?

Updated

12 years ago
Attachment #199020 - Flags: approval1.8rc1? → approval1.8rc1+
Verified FIXED using a self-built SeaMonkey trunk as of PDT 5:00pm this evening
on testcase: https://bugzilla.mozilla.org/attachment.cgi?id=198952 with Windows XP.
Status: RESOLVED → VERIFIED
Fixed on branch
Keywords: fixed1.8

Updated

12 years ago
Flags: blocking1.8rc1?

Updated

12 years ago
Flags: testcase+
Comment on attachment 199020 [details] [diff] [review]
Proposed fix

aviary101/moz17 landing approval: a=dveditz for drivers. Please add the fixed1.7.13 and fixed-aviary1.0.8 keywords when landed.
Attachment #199020 - Flags: approval1.7.13+
Attachment #199020 - Flags: approval-aviary1.0.8+
*** Committing to MOZILLA_1_7_BRANCH... 
new revision: 1.14.50.1; previous revision: 1.14

*** Committing layout/xul/base/src/grid/nsGrid.cpp on AVIARY_1_0_1_20050124_BRANCH
new revision: 1.14.64.1; previous revision: 1.14
Keywords: fixed-aviary1.0.8, fixed1.7.13

Comment 11

12 years ago
v.fixed on 1.0.1 Aviary branch with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.12) Gecko/20060209 Firefox/1.0.7
Keywords: fixed-aviary1.0.8 → verified-aviary1.0.8
verified fixed on 1.7 branch using Mozilla 1.7.13 Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7.13) Gecko/20060309. No crash with testcase.
Keywords: fixed1.7.13 → verified1.7.13
Group: security

Updated

10 years ago
Flags: in-testsuite+ → in-testsuite?

Comment 13

8 years ago
crash test landed
http://hg.mozilla.org/mozilla-central/rev/e7bb6f9cd175
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.