Closed Bug 317554 Opened 15 years ago Closed 15 years ago

Crash with evil display: table-footer-group testcase [@ nsCellMap::GetCellInfoAt]

Categories

(Core :: Layout: Tables, defect)

defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: martijn.martijn, Assigned: bernd_mozilla)

References

Details

(5 keywords, Whiteboard: [sg:critical][rft-dl])

Crash Data

Attachments

(3 files, 2 obsolete files)

See upcoming testcase, it crashes for me when hovering over the text.
It also crashes in Mozilla1.7, so no recent regression.

Talkback ID: TB12167662G
Attached file testcase
crashes for me, suggest changing OS and hardware to ALL

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US;
rv:1.8) Gecko/20051111 Firefox/1.5
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US;
rv:1.8) Gecko/20051111 Firefox/1.5
Crashes for me in Camino as well.

-> All/All
OS: Windows XP → All
Hardware: PC → All
nsCellMap::GetCellInfoAt(nsTableCellMap & {...}, int 2, int 0, int * 0x0012c4f4, int * 0x0012c4f8) line 2406 + 16 bytes
nsTableCellMap::GetCellInfoAt(int 2, int 0, int * 0x0012c4f4, int * 0x0012c4f8) line 762 + 23 bytes
nsTableFrame::GetCellInfoAt(int 2, int 0, int * 0x0012c4f4, int * 0x0012c4f8) line 4479
BasicTableLayoutStrategy::AssignNonPctColumnWidths(int 23760, const nsHTMLReflowState & {...}) line 1036 + 28 bytes
BasicTableLayoutStrategy::Initialize(const nsHTMLReflowState & {...}) line 143 + 17 bytes
nsTableFrame::ReflowTable(nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, int 1073741824, nsReflowReason eReflowReason_Resize, nsIFrame * & 0x00000000, int & 0, unsigned int & 0) line 2121
and so on with the the usual suspects, the cellframe that is referred by the cellmap has been deleted before.
Summary: Crash with evil display: table-footer-group testcase → Crash with evil display: table-footer-group testcase [@ nsCellMap::GetCellInfoAt]
***START TABLE DUMP***
mColWidths=-1
row(3)=033E7BCC cell(0)=033E7D7C
row(1)=033F05A4
***** START TABLE CELL MAP DUMP ***** 033EFEF0
cols array orig/span-> 033EFEF00=1/0  cols in cache 1
  ***** START GROUP CELL MAP DUMP ***** 033F1570
  mapRowCount=0 tableRowCount=0
  ***** END GROUP CELL MAP DUMP *****

  ***** START GROUP CELL MAP DUMP ***** 033F10C0
  mapRowCount=0 tableRowCount=0
  ***** END GROUP CELL MAP DUMP *****

  ***** START GROUP CELL MAP DUMP ***** 033F1128
  mapRowCount=3 tableRowCount=3
  row 0 :
  row 1 :
  row 2 : C2,0


  C2,0=033E7D7C(0)
  ***** END GROUP CELL MAP DUMP *****

  ***** START GROUP CELL MAP DUMP ***** 033F15D8
  mapRowCount=0 tableRowCount=0
  ***** END GROUP CELL MAP DUMP *****

  ***** START GROUP CELL MAP DUMP ***** 033F1640
  mapRowCount=1 tableRowCount=1
  row 0 :

  ***** END GROUP CELL MAP DUMP *****
***** END TABLE CELL MAP DUMP *****

The table cellmap without hovering is already wrong, there should not be 3 rows to wrap the text in a single anonymous table cell.
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → bernd_mozilla
Status: NEW → ASSIGNED
Attached patch patchSplinter Review
Robert this is the patch that I asked you on IRC.

So what goes wrong here:
First we have for every rowgroup a cellmap. Rowgroups are not necessarily layout as the follow in content because the can be header or footer. In order to deal with this we have OrderRowGroups. The cellmaps should be in the same order as the rowgroups after OrderRowGroups. This does not happen without the patch if we initialize multiple cellmaps at once. If we initialize a cellmap we append after its precessor. If we now start to add multiple cellmaps in the wrong order we first append the last cellmap for instance, its precessor has not been created yet so its null and we will insert the cellmap at the beginning instead of the end. Once the cellmap is out of sync with the row group frames, its only a matter of dom transitions till we crash.
Attachment #205545 - Attachment is obsolete: true
Attachment #205742 - Flags: superreview?(roc)
Attachment #205742 - Flags: review?(roc)
Attachment #205742 - Flags: superreview?(roc)
Attachment #205742 - Flags: superreview+
Attachment #205742 - Flags: review?(roc)
Attachment #205742 - Flags: review+
fixed on trunk
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051217 Firefox/1.6a1
Status: RESOLVED → VERIFIED
Comment on attachment 205742 [details] [diff] [review]
patch

seems to be pretty safe
Attachment #205742 - Flags: approval1.8.1?
Attachment #205742 - Flags: approval1.8.0.1?
Comment on attachment 205742 [details] [diff] [review]
patch

a-=schrep for drivers.  Given this is not a topcrash or security issue and the size of the changes we should land on the 1.8 branch for 1.8.1 release.
Attachment #205742 - Flags: approval1.8.0.1? → approval1.8.0.1-
Comment on attachment 205742 [details] [diff] [review]
patch

a-=schrep for drivers.  Given this is not a topcrash or security issue or regression and the size of the changes we should land on the 1.8 branch for 1.8.1 release.
Attachment #205742 - Flags: approval1.8.1? → branch-1.8.1?(roc)
Attachment #205742 - Flags: approval-branch-1.8.1?(roc) → approval-branch-1.8.1+
Comment on attachment 205742 [details] [diff] [review]
patch

there are some rumors that this is security relevant (bug 328937)
Attachment #205742 - Flags: approval1.8.0.2?
Attached patch patch against 1.7 (obsolete) — Splinter Review
Attachment #213619 - Flags: approval1.7.13?
Attachment #213619 - Flags: approval-aviary1.0.9?
Attachment #213619 - Flags: approval-aviary1.0.8?
Group: security
Flags: blocking1.8.0.2+
Flags: blocking1.7.13+
Flags: blocking-aviary1.0.8+
Whiteboard: [sg:critical]
Comment on attachment 205742 [details] [diff] [review]
patch

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #205742 - Flags: approval1.8.0.2? → approval1.8.0.2+
fix checked into 1.8.1 and 1.8.0.2
Marking [rft-dl] (ready for testing in Firefox 1.5.0.2 release candidates)
Whiteboard: [sg:critical] → [sg:critical][rft-dl]
Comment on attachment 213619 [details] [diff] [review]
patch against 1.7

>+            InsertRows(*rgFrame, rows, rowIndex, PR_TRUE);

This patch doesn't compile on moz17/aviary101 unless this is changed to
              InsertRows(aPresContext, *rgFrame, rows, rowIndex, PR_TRUE);

Apart from that, once applied this stops the exploit in FF1.0.x. Should I worry that the above isn't the only problem with the patch?
Attachment #213619 - Attachment is obsolete: true
Attachment #213694 - Flags: review?
Attachment #213694 - Flags: approval1.7.13?
Attachment #213694 - Flags: approval-aviary1.0.8?
Attachment #213619 - Flags: approval1.7.13?
Attachment #213619 - Flags: approval-aviary1.0.9?
Attachment #213619 - Flags: approval-aviary1.0.8?
Attachment #213694 - Flags: review? → review?(bernd_mozilla)
Comment on attachment 213694 [details] [diff] [review]
udpated moz1.7 patch

got r= from bz in #content
approved for aviary/moz17 branches per drivers
Attachment #213694 - Flags: review?(bernd_mozilla)
Attachment #213694 - Flags: review+
Attachment #213694 - Flags: approval1.7.13?
Attachment #213694 - Flags: approval1.7.13+
Attachment #213694 - Flags: approval-aviary1.0.8?
Attachment #213694 - Flags: approval-aviary1.0.8+
fix checked in into aviary and 1.7
verified fixed on the 1.x branch using 2006-03-02-11-aviary1.0.1 bits (Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7.13) Gecko/20060302 Firefox/1.0.8). Adding relevant keyword.
aslo verified on Windows Fx 1.0.8, Moz 1.7.13, Fx 1.5.0.2 from 03/02
Group: security
Crash Signature: [@ nsCellMap::GetCellInfoAt]
You need to log in before you can comment on or make changes to this bug.