149436 - Mozilla crashed on a Tru64 UNIX machine

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Shanmugavelu Shanmuganathan (gone)]

Mozilla 1.0 crahsed while trying to access www.tamilbeat.com
I loaded this page and clicked on the second item on the right.
Mozilla crashed.
It didn't crash if when I clicked the first item (or any oter item).
Upon debugging I found that the following. 
In ./layout/html/table/src/nsTableFrame.cpp
the line 5918 reads like this
 for (PRInt32 rowX = info.rowIndex; rowX <= cellEndRowIndex; rowX++) {
nsTableRowFrame* rowFrame = (rowX == info.rowIndex) ? info.topRow
:rowFrame->GetNextRow();
....
 }
Here rowFrame's scope is inside the for loop. Tru64 UNIX uses
the memory that is freed and there is no gaurantee that the address
will still hold the same value after a while.  So once mozilla steps out
of this loop the memory will be used. So when the next time this 
value is accessed it get the garbage value and crashes. To avoid this 
it would be safe to declare rowFrame outside of the loop. 

  nsTableRowFrame* rowFrame;
      for (PRInt32 rowX = info.rowIndex; rowX <= cellEndRowIndex; rowX++) {
        rowFrame = (rowX == info.rowIndex) ? info.topRow : rowFrame->GetNextRow();
        ....
      }
I made these changes and it worked fine and didn't crash. 

I found the same exact code in another place in the same  file.
I will attach the fix soon.

Comment 1

[Shanmugavelu Shanmuganathan (gone)]

Attachment: Fix

Comment 2

[Matthias Versen [:Matti]]

-> NEW

Reporter: Please ask for reviews for your patch (see www.mozilla.org/hacking)

Comment 3

[Shanmugavelu Shanmuganathan (gone)]

Can I have a r= and sr= for this patch please.

Comment 4

[Kevin McCluskey (gone)]

-> Karnaze

Comment 5

[karnaze (gone)]

Comment on attachment 86501 [details] [diff] [review]
Fix

r=karnaze

Comment 6

[kinmoz]

Comment on attachment 86501 [details] [diff] [review]
Fix

sr=[e-mail]

Comment 7

[Shanmugavelu Shanmuganathan (gone)]

I don't have permissions to commit my changes in to the repository.
may I ask some one in this list to commit these changes.

Comment 8

[karnaze (gone)]

The patch is in.

Comment 9

[Aleksey Nogin]

This commit have added two "may be used uninitialized" warnings:

+layout/html/table/src/nsTableFrame.cpp:5917
+ `class nsTableRowFrame * rowFrame' might be used uninitialized in this function
+
+layout/html/table/src/nsTableFrame.cpp:5950
+ `class nsTableRowFrame * rowFrame' might be used uninitialized in this function

See also bug 59652 and bug 59675

Comment 10

[karnaze (gone)]

shanmu? - this is your patch

Comment 11

[Shanmugavelu Shanmuganathan (gone)]

These two warnings always showed up. I just had a glimpse of the build log
from one of the clobber build (before the fix was checked in), where I see this.

Warning 156: "nsTableFrame.cpp", line 5918 # The variable 'rowFrame' may not be
initialized on all possible paths from start of the function.
    bleRowFrame* rowFrame = (rowX == info.rowIndex) ? info.topRow :
rowFrame->GetNextRow();
                                                                    ^^^^^^^^   
           
Warning 156: "nsTableFrame.cpp", line 5950 # The variable 'rowFrame' may not be
initialized on all possible paths from start of the function.
    bleRowFrame* rowFrame = (rowX == info.rowIndex) ? info.topRow :
rowFrame->GetNextRow();


                                                                   

Comment 12

[Jim Dunn]

Yes, this has always shown up.  If you look at the logic...
yes it is "ok" because the first time thru the loop
you don't access rowframe, only on subsequent times thru
the loop.

I had suggested a change to remove these warnings because
in this loop this check is unnecessary (and wasteful).
But never checked it in...

Comment 13

[Colin Blake]

*** Bug 136769 has been marked as a duplicate of this bug. ***

Comment 14

[chris hofmann]

Comment on attachment 86501 [details] [diff] [review]
Fix

a=chofmann for the 1.01 branch. add fixed1.01 after checking in.  thanks

Comment 15

[Syd Logan]

added adt1.0.1+

Comment 16

[karnaze (gone)]

fixed1.0.1

Comment 17

[Matthias Versen [:Matti]]

*** Bug 155661 has been marked as a duplicate of this bug. ***

Comment 18

[Matthias Versen [:Matti]]

*** Bug 159010 has been marked as a duplicate of this bug. ***

Comment 19

[Moied]

Verified with branch build 20020731 on winxp, adding KW:verified1.0.1