338391 - Out of bounds and crash [@ nsAttrAndChildArray::RemoveChildAt] removing a node during a mutation event for its removal

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jesse Ruderman]

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20060517 Minefield/3.0a1

Loading the testcase causes a crash [@ nsAttrAndChildArray::RemoveChildAt].

Security-sensitive because I see an "out-of-bounds" assertion at nsAttrAndChildArray.cpp line 212 along with manual pointer arithmetic (!) in that function.

Comment 1

[Jesse Ruderman]

Attachment: testcase (crashes Firefox)

Comment 2

[Daniel Veditz [:dveditz]]

nsGenericElement::RemoveChildAt() is called with aIndex of 1. But by time doRemoveChildAt() calls nsAttrAndChildArray::RemoveChildAt aIndex has morphed into 0xffffffff.

culprit appears to be here, 
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/base/src/nsGenericElement.cpp&rev=3.478&mark=2331-2332#2327

This was to prevent the array bounds problem in bug 329982. IndexOf returns -1 to signal it's not a child, but aIndex is unsigned so it's never < 0 and we don't bail out.

Comment 3

[Daniel Veditz [:dveditz]]

Attachment: patch

Comment 4

[Jesse Ruderman]

I'm guessing most of our compilers issued a warning that the comparison was useless, but nobody noticed? :/

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 222662 [details] [diff] [review]
patch

We used to have tinderboxes that showed warnings, but that doesn't seem to be the case any more :(

>   if (guard.Mutated(0)) {
>     aIndex = container.IndexOf(aKid);
>-    if (aIndex < 0) {
>+    if (aIndex == (PRUint32)(-1)) {

Please add a cast to the line above as well since I'm guessing that warns about signed/unsigned missmatch.

Comment 6

[Daniel Veditz [:dveditz]]

(In reply to comment #4)
> I'm guessing most of our compilers issued a warning that the comparison was
> useless, but nobody noticed? :/

The vc7.1 compiler doesn't (on our default -W3 setting). It warns about a '<' mismatch on line 4116 and 4137, but not for this one. With -W4 I get about 32 screens of warnings for just this one file, mostly from our templatized headers.

Comment 7

[Daniel Veditz [:dveditz]]

> With -W4 I get about 32 screens of warnings for just this one file,

And yet it still doesn't catch this error.

Comment 8

[Jesse Ruderman]

gcc doesn't complain about anything in this file (with the default options for a debug Firefox build).  Weird.

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 222662 [details] [diff] [review]
patch

approved for 1.8.0 branch, a=dveditz for drivers

Comment 10

[Jay Patel [:jay]]

v.fixed on 1.8.0 branch with Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US;
rv:1.8.0.5) Gecko/20060626 Firefox/1.5.0.5, no crash with testcase.

Comment 11

[Jesse Ruderman]

dveditz, has this been fixed on trunk?

Comment 12

[Daniel Veditz [:dveditz]]

Yes, fixed on June 22, rev=3.488 of nsGenericElement.cpp

Comment 13

[Bob Clary [:bc] (inactive)]

https://bugzilla.mozilla.org/attachment.cgi?id=222462
ff2b2 windows, linux, macppc no crash, macppc shows a blank url bar after the test loads.
ff2b2 debug windows, linux no crash
verified fixed 1.8

Comment 14

[Jesse Ruderman]

Crashtest checked in.