Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Heap corruption in nsTableRowGroupFrame::CalculateRowHeights

RESOLVED FIXED

Status

()

Core
Layout: Tables
--
critical
RESOLVED FIXED
11 years ago
8 years ago

People

(Reporter: Eli Friedman, Assigned: Bernd)

Tracking

(4 keywords)

Trunk
crash, fixed1.8.0.12, fixed1.8.1.4, testcase
Points:
---
Bug Flags:
blocking1.8.1.4 +
wanted1.8.1.x +
blocking1.8.0.12 +
wanted1.8.0.x +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical], URL)

Attachments

(4 attachments)

(Reporter)

Description

11 years ago
When loading http://www.w3.org/TR/css3-page/ in print preview, I get a C runtime assertion complaining about heap corruption.

(If it doesn't crash for you, try turning up your scaling level.)

Trace:
WARNING: Stack unwind information not available. Following frames may be wrong.
MSVCR80D!free_dbg+0x2df
MSVCR80D!free_dbg+0x4e
MSVCR80D!operator delete+0xb7
MSVCR80D!operator delete[]+0xc
gklayout!nsTableRowGroupFrame::CalculateRowHeights(class nsPresContext * aPresContext = 0x04a54db0, struct nsHTMLReflowMetrics * aDesiredSize = 0x0012b28c, struct nsHTMLReflowState * aReflowState = 0x0000b388)+0x7ce [c:\mozilla\mozilla\layout\tables\nstablerowgroupframe.cpp @ 821]
gklayout!nsTableRowGroupFrame::ReflowChildren(class nsPresContext * aPresContext = 0x04a54db0, struct nsHTMLReflowMetrics * aDesiredSize = 0x0012b28c, struct nsRowGroupReflowState * aReflowState = 0x00000001, unsigned int * aStatus = 0x0012ba10, int * aPageBreakBeforeEnd = 0x0012b0ec)+0x345 [c:\mozilla\mozilla\layout\tables\nstablerowgroupframe.cpp @ 486]
gklayout!nsTableRowGroupFrame::Reflow(class nsPresContext * aPresContext = 0x04a54db0, struct nsHTMLReflowMetrics * aDesiredSize = 0x049db8a0, struct nsHTMLReflowState * aReflowState = 0x00000000, unsigned int * aStatus = 0x0012ba10)+0xbc [c:\mozilla\mozilla\layout\tables\nstablerowgroupframe.cpp @ 1280]
gklayout!nsContainerFrame::ReflowChild(class nsIFrame * aKidFrame = 0x04c25a14, class nsPresContext * aPresContext = 0x04a54db0, struct nsHTMLReflowMetrics * aDesiredSize = 0x0012b28c, struct nsHTMLReflowState * aReflowState = 0x0012b1e0, int aX = 120, int aY = 120, unsigned int aFlags = 0, unsigned int * aStatus = 0x0012ba10)+0x74 [c:\mozilla\mozilla\layout\generic\nscontainerframe.cpp @ 954]
gklayout!nsTableFrame::ReflowChildren(struct nsTableReflowState * aReflowState = 0x0012b38c, unsigned int * aStatus = 0x0012ba10, class nsIFrame ** aLastChildReflowed = 0x0012b430, struct nsRect * aOverflowArea = 0x0012b654)+0x2b6 [c:\mozilla\mozilla\layout\tables\nstableframe.cpp @ 2828]
gklayout!nsTableFrame::ReflowTable(struct nsHTMLReflowMetrics * aDesiredSize = 0x0012b62c, struct nsHTMLReflowState * aReflowState = 0x0012b4d4, int aAvailHeight = 27840, class nsIFrame ** aLastChildReflowed = 0x0012b430, unsigned int * aStatus = 0x0012ba10)+0x60 [c:\mozilla\mozilla\layout\tables\nstableframe.cpp @ 2040]
gklayout!nsTableFrame::Reflow(class nsPresContext * aPresContext = 0x00000000, struct nsHTMLReflowMetrics * aDesiredSize = 0x0012b62c, struct nsHTMLReflowState * aReflowState = 0x00000000, unsigned int * aStatus = 0x0012ba10)+0x161 [c:\mozilla\mozilla\layout\tables\nstableframe.cpp @ 1933]
gklayout!nsContainerFrame::ReflowChild(class nsIFrame * aKidFrame = 0x049db8a0, class nsPresContext * aPresContext = 0x04a54db0, struct nsHTMLReflowMetrics * aDesiredSize = 0x0012b62c, struct nsHTMLReflowState * aReflowState = 0x0012b4d4, int aX = 0, int aY = 0, unsigned int aFlags = 3, unsigned int * aStatus = 0x0012ba10)+0x74 [c:\mozilla\mozilla\layout\generic\nscontainerframe.cpp @ 954]
gklayout!nsTableOuterFrame::OuterReflowChild(class nsPresContext * aPresContext = 0x04a54db0, class nsIFrame * aChildFrame = 0x049db8a0, struct nsHTMLReflowState * aOuterRS = 0x00006cc0, void * aChildRSSpace = 0x0012b4d4, struct nsHTMLReflowMetrics * aMetrics = 0x0012b62c, int aAvailWidth = 15060, struct nsSize * aDesiredSize = 0x0012b6cc, struct nsMargin * aMargin = 0x0012b6bc, unsigned int * aStatus = 0x0012ba10)+0x12b [c:\mozilla\mozilla\layout\tables\nstableouterframe.cpp @ 1112]
gklayout!nsTableOuterFrame::Reflow(class nsPresContext * aPresContext = 0x04a54db0, struct nsHTMLReflowMetrics * aDesiredSize = 0x0012b858, struct nsHTMLReflowState * aOuterRS = 0x00000000, unsigned int * aStatus = 0x0012ba10)+0x1a8 [c:\mozilla\mozilla\layout\tables\nstableouterframe.cpp @ 1217]
gklayout!nsBlockReflowContext::ReflowBlock(struct nsRect * aSpace = 0x0012b9dc, int aApplyTopMargin = 0, struct nsCollapsingMargin * aPrevMargin = 0x0012bc98, int aClearance = 0, int aIsAdjacentWithTop = 1, struct nsMargin * aComputedOffsets = 0x00000000, struct nsHTMLReflowState * aFrameRS = 0x0012b89c, unsigned int * aFrameReflowStatus = 0x0012ba10)+0x1e0 [c:\mozilla\mozilla\layout\generic\nsblockreflowcontext.cpp @ 372]
gklayout!nsBlockFrame::ReflowBlockFrame(class nsBlockReflowState * aState = 0x00000000, class nsLineList_iterator aLine = class nsLineList_iterator, int * aKeepReflowGoing = 0x0012bba0)+0x3cb [c:\mozilla\mozilla\layout\generic\nsblockframe.cpp @ 2873]
gklayout!nsBlockFrame::ReflowLine(class nsBlockReflowState * aState = 0x0012bc2c, class nsLineList_iterator aLine = class nsLineList_iterator, int * aKeepReflowGoing = 0x0012bba0)+0xb6 [c:\mozilla\mozilla\layout\generic\nsblockframe.cpp @ 2115]
gklayout!nsBlockFrame::ReflowDirtyLines(class nsBlockReflowState * aState = 0x00000000)+0x376 [c:\mozilla\mozilla\layout\generic\nsblockframe.cpp @ 1779]
gklayout!nsBlockFrame::Reflow(class nsPresContext * aPresContext = 0x04a54db0, struct nsHTMLReflowMetrics * aMetrics = 0x0012c0f8, struct nsHTMLReflowState * aReflowState = 0x0012c13c, unsigned int * aStatus = 0x0012c2b0)+0x21c [c:\mozilla\mozilla\layout\generic\nsblockframe.cpp @ 903]
gklayout!nsBlockReflowContext::ReflowBlock(struct nsRect * aSpace = 0x0012c27c, int aApplyTopMargin = 0, struct nsCollapsingMargin * aPrevMargin = 0x0012c538, int aClearance = 0, int aIsAdjacentWithTop = 1, struct nsMargin * aComputedOffsets = 0x00000870, struct nsHTMLReflowState * aFrameRS = 0x0012c13c, unsigned int * aFrameReflowStatus = 0x0012c2b0)+0x1e0 [c:\mozilla\mozilla\layout\generic\nsblockreflowcontext.cpp @ 372]

I've also gotten all sorts of wonderful assertions.

Not reduced, at least at the moment.
(Reporter)

Comment 1

11 years ago
Created attachment 255070 [details]
Reduced testcase

There, reduced it.
(Assignee)

Comment 2

11 years ago
rowinfo is they key we work past array bounds
Assignee: nobody → bernd_mozilla
Severity: normal → critical

Updated

11 years ago
Keywords: crash, testcase
(Assignee)

Comment 3

11 years ago
Created attachment 255305 [details] [diff] [review]
fix for the assert
(Assignee)

Comment 4

11 years ago
Comment on attachment 255305 [details] [diff] [review]
fix for the assert

The patch fixes the assert and cells will not use negative heights, but then we crash seemingly randomly (my bet is painting). It might be that the assert makes us crash in a single place rather than randomly
Attachment #255305 - Flags: superreview?(bzbarsky)
Attachment #255305 - Flags: review?(bzbarsky)
(Reporter)

Comment 5

11 years ago
Created attachment 255384 [details] [diff] [review]
Patch to make the issue completely obvious

The issue should become very clear if you apply this patch and try the testcase.  It should have been clear anyway from the trace and the title of the bug, but here you go.

Comment 6

11 years ago
Comment on attachment 255305 [details] [diff] [review]
fix for the assert

So this makes NS_FRAME_SET_TRUNCATION do the right thing, basically?
Attachment #255305 - Flags: superreview?(bzbarsky)
Attachment #255305 - Flags: superreview+
Attachment #255305 - Flags: review?(bzbarsky)
Attachment #255305 - Flags: review+
(Assignee)

Comment 7

11 years ago
exactly
(Assignee)

Comment 8

11 years ago
Created attachment 255455 [details] [diff] [review]
revised patch

The patch includes the fixup that is already reviewed, switches the rowinfo to Tarray and fixes the access beyond the array boundaries.
Attachment #255455 - Flags: superreview?(bzbarsky)
Attachment #255455 - Flags: review?(bzbarsky)
(Assignee)

Updated

11 years ago
Attachment #255455 - Attachment is patch: true

Comment 9

11 years ago
Comment on attachment 255455 [details] [diff] [review]
revised patch

>Index: nsTableRowGroupFrame.cpp

>+  nsTArray<RowInfo> rowInfo;
>+  rowInfo.AppendElements(numRows);

Bail out if AppendElements fails?

r+sr=bzbarsky with that
Attachment #255455 - Flags: superreview?(bzbarsky)
Attachment #255455 - Flags: superreview+
Attachment #255455 - Flags: review?(bzbarsky)
Attachment #255455 - Flags: review+
(Assignee)

Comment 10

11 years ago
fix checked in
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Comment 11

11 years ago
Comment on attachment 255455 [details] [diff] [review]
revised patch

at least +          if ((rowIndex + rowSpan) > numRows) {
+            // there might be rows pushed already to the nextInFlow
+            rowSpan = numRows - rowIndex;
+          }

should be go branches

I would prefer to have the rest too minus the Tarray part.
Attachment #255455 - Flags: approval1.8.1.3?
Attachment #255455 - Flags: approval1.8.0.11?
Comment on attachment 255455 [details] [diff] [review]
revised patch

approved for 1.8.0.12 and 1.8.1.4, a=dveditz for release-drivers
Attachment #255455 - Flags: approval1.8.1.4?
Attachment #255455 - Flags: approval1.8.1.4+
Attachment #255455 - Flags: approval1.8.0.12?
Attachment #255455 - Flags: approval1.8.0.12+
Group: security
Flags: blocking1.8.1.4+
Flags: blocking1.8.0.12+
Whiteboard: [sg:critical]
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
(Assignee)

Comment 13

10 years ago
I checked in the part from comment 11 the other stuff does not apply anyway.
Keywords: fixed1.8.0.12, fixed1.8.1.4
Group: security
Flags: in-testsuite?
crash test landed
http://hg.mozilla.org/mozilla-central/rev/8dc547dc339c
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.