The default bug view has changed. See this FAQ.

dom cellmap crash

RESOLVED FIXED in mozilla1.8.1

Status

()

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

People

(Reporter: Bernd, Assigned: Bernd)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
mozilla1.8.1
x86
Windows XP
crash, testcase, verified1.8.0.7, verified1.8.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9a1 +
blocking1.8.1 +
blocking1.8.0.7 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical] heap buffer underflow. fix rolled into 346980)

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

11 years ago
the testcase tries to access foo[-1] or foo[0xffffffff] marking sensitive.
(Assignee)

Comment 1

11 years ago
Created attachment 228578 [details]
testcase
Assignee: nobody → bernd_mozilla
Status: NEW → ASSIGNED
(Assignee)

Updated

11 years ago
Blocks: 339128
(Assignee)

Comment 2

11 years ago
Created attachment 228602 [details] [diff] [review]
patch
Affects the 1.8 branches also, nominating.

Why do these routines use signed indexes? Could they be changed to use unsigned ints instead?
Flags: blocking1.9a1+
Flags: blocking1.8.1?
Flags: blocking1.8.0.6?
Whiteboard: [sg:critical] heap buffer underflow
Comment on attachment 228602 [details] [diff] [review]
patch

>-
>+  NS_ASSERTION(aFirstRowIndex >= 0, "nsCellMap::InsertRows called with negative rowIndex");

If you don't change the API's to use unsigned should we do more than assert? Add the assertion to other routines as well?

>   PRInt32 numRows = mRows.Count();
>   PRInt32 numCols = aMap.GetColCount();
> 
>   if (aFirstRowIndex >= numRows) {
>+    mRowCount -= aNumRowsToRemove;
>     return;

seems dodgy here, can mRowCount be made negative this way? Or has something else already incremented mRowCount by exactly the amount aFirstRowIndex is larger than numRows.

What if aFirstRowIndex is ever negative here? We didn't expect it could happen in the other place either.

>     if (aEndRowIndex < mRowCount - 1) {
>       cellData = GetDataAt(aMap, aEndRowIndex + 1, colX, PR_TRUE);
>       if ((cellData) && (cellData->IsRowSpan())) {
>         return PR_TRUE; // there is a row span out of the region
>       }
>     }
>+    else {
>+      // this cell might be the cause of a dead row which should be removed
>+      return PR_TRUE;
>+    }

"might be"? what happens if it isn't?
(Assignee)

Comment 5

11 years ago
>Why do these routines use signed indexes?
Because Chris wrote them. -1 is very common as a hidden boolean parameter to toggle function behaviour.

I haven't really analyzed the code what would happen with unsigned indices. I am currently more in panic mode trying to get the whole thing survive bug 339128. The upcoming patch does exactly this at least on my machine.

>seems dodgy here, can mRowCount be made negative this way? Or has something
>else already incremented mRowCount by exactly the amount aFirstRowIndex is
>larger than numRows.
Every row that we have increases mRowCount. Every row that we remove has to reduce it. We have a scenario where the latter does not happen without the patch.

>What if aFirstRowIndex is ever negative here? We didn't expect it could happen
>in the other place either.

If the routine returns PR_TRUE we will call a more expensive routine thats not a issue. 
(Assignee)

Comment 6

11 years ago
Created attachment 229215 [details] [diff] [review]
patch
Attachment #228602 - Attachment is obsolete: true
Attachment #229215 - Flags: superreview?(bzbarsky)
Attachment #229215 - Flags: review?(bzbarsky)

Updated

11 years ago
Flags: blocking1.8.1? → blocking1.8.1+
I'd really like an explanation of what the various parts of the patch do to be able to review this...  For example, if aEndRowIndex >= mRowCount - 1, why is it ok to call GetDataAt(aMap, aEndRowIndex, colX, PR_TRUE)?  Why is the numOrigRows check needed?   That sort of thing.
Flags: blocking1.8.0.6? → blocking1.8.0.6+
Whiteboard: [sg:critical] heap buffer underflow → [sg:critical] [needs review bzbarsky] heap buffer underflow
(Assignee)

Comment 8

11 years ago
Created attachment 231583 [details] [diff] [review]
revised patch
Attachment #231583 - Flags: superreview?(bzbarsky)
Attachment #231583 - Flags: review?(bzbarsky)
(Assignee)

Updated

11 years ago
Attachment #229215 - Flags: superreview?(bzbarsky)
Attachment #229215 - Flags: review?(bzbarsky)
Comment on attachment 231583 [details] [diff] [review]
revised patch

>--- nsCellMap.cpp	2006-08-01 16:26:40.296875000 +0200
>+++ nsCellMap.local	2006-08-01 16:33:42.140625000 +0200
>@@ -1498,30 +1502,51 @@ PRBool nsCellMap::CellsSpanOut(nsVoidArr
>+                                   // current cellmap extension.

s/extension/extent/ ?

>@@ -2023,18 +2048,23 @@ nsCellMap::RebuildConsideringRows(nsTabl

>+  PRInt32 copyEndRowIndex = PR_MIN(numOrigRows, aStartRowIndex);
>+
>   // put back the rows before the affected ones just as before
>-  for (rowX = 0; rowX < aStartRowIndex; rowX++) {
>+  // aStartRowIndex might be after all existing rows so we should limit the
>+  // copy to the amount of exisiting rows

Wouldn't this comment make more sense before you declare copyEndRowIndex and do the PR_MIN thing?

With that, looks ok.  Thanks for the comments!
Attachment #231583 - Flags: superreview?(bzbarsky)
Attachment #231583 - Flags: superreview+
Attachment #231583 - Flags: review?(bzbarsky)
Attachment #231583 - Flags: review+
(Assignee)

Comment 10

11 years ago
fixed on trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Updated

11 years ago
Blocks: 346980

Comment 11

11 years ago
Is this patch suitable for 1.8 branch?

Updated

11 years ago
Target Milestone: --- → mozilla1.8.1
(Assignee)

Comment 12

11 years ago
fixed on branch by the patch in bug 346980
Keywords: fixed1.8.1
(Assignee)

Comment 13

11 years ago
the only bug that should block 1.8.0.7 is bug 346980 which is blocked by the
regression 347725

asking for 1.8.0.7- on this

Comment 14

11 years ago
https://bugzilla.mozilla.org/attachment.cgi?id=228578&action=view
ff2b2 debug windows/linux no crash, no assert, nightly windows/linux no crash
verified fixed 1.8
Keywords: fixed1.8.1 → verified1.8.1
Whiteboard: [sg:critical] [needs review bzbarsky] heap buffer underflow → [sg:critical] heap buffer underflow. fix rolled into 346980
Moving to 1.8.0.8 at Bernd's request (regression worries)
Flags: blocking1.8.0.8+
Flags: blocking1.8.0.7-
Flags: blocking1.8.0.7+
(Assignee)

Comment 16

11 years ago
fixed on branches by the cellmap branch patch
Keywords: fixed1.8.0.7
Can I get some clarification on this bug's status?  It seems to have been minus-ed for 1807, but it is marked 'fixed1807'.  Has the patch been checked in?  Has this issue been fixed by another patch?  Or are we planning on checking in this patch to the 1.8.0 branch later, even though this particular crash has been fixed?

This did land in 1.8.0.7 after all. It wasn't "blocking", but was still approved.
Flags: blocking1.8.0.8+
Flags: blocking1.8.0.7-
Flags: blocking1.8.0.7+
https://bugzilla.mozilla.org/attachment.cgi?id=228578&action=view should not crash

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.7) Gecko/20060906 Firefox/1.5.0.7

verified 1.8.0.7
Keywords: fixed1.8.0.7 → verified1.8.0.7
Group: security
Flags: in-testsuite?

Comment 20

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