Last Comment Bug 338391 - Out of bounds and crash [@ nsAttrAndChildArray::RemoveChildAt] removing a node during a mutation event for its removal
: Out of bounds and crash [@ nsAttrAndChildArray::RemoveChildAt] removing a nod...
Status: RESOLVED FIXED
[sg:critical?]
: crash, testcase, verified1.8.0.5, verified1.8.1
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: PowerPC Mac OS X
: -- normal (vote)
: ---
Assigned To: Jonas Sicking (:sicking) PTO Until July 5th
: Hixie (not reading bugmail)
Mentors:
Depends on:
Blocks: 325861 329982
  Show dependency treegraph
 
Reported: 2006-05-17 23:58 PDT by Jesse Ruderman
Modified: 2007-12-13 20:09 PST (History)
6 users (show)
dveditz: blocking1.9a1+
dveditz: blocking1.8.1+
dveditz: blocking1.8.0.5+
jruderman: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (crashes Firefox) (539 bytes, application/xhtml+xml)
2006-05-17 23:59 PDT, Jesse Ruderman
no flags Details
patch (894 bytes, patch)
2006-05-19 13:09 PDT, Daniel Veditz [:dveditz]
jonas: review+
jonas: superreview+
jonas: approval‑branch‑1.8.1+
dveditz: approval1.8.0.5+
Details | Diff | Review

Description Jesse Ruderman 2006-05-17 23:58:08 PDT
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 2006-05-17 23:59:01 PDT
Created attachment 222462 [details]
testcase (crashes Firefox)
Comment 2 Daniel Veditz [:dveditz] 2006-05-19 13:05:33 PDT
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] 2006-05-19 13:09:39 PDT
Created attachment 222662 [details] [diff] [review]
patch
Comment 4 Jesse Ruderman 2006-05-19 14:10:47 PDT
I'm guessing most of our compilers issued a warning that the comparison was useless, but nobody noticed? :/
Comment 5 Jonas Sicking (:sicking) PTO Until July 5th 2006-05-19 15:12:32 PDT
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] 2006-05-19 15:57:01 PDT
(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] 2006-05-19 16:03:22 PDT
> 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 2006-05-20 05:34:29 PDT
gcc doesn't complain about anything in this file (with the default options for a debug Firefox build).  Weird.
Comment 9 Daniel Veditz [:dveditz] 2006-06-19 11:56:09 PDT
Comment on attachment 222662 [details] [diff] [review]
patch

approved for 1.8.0 branch, a=dveditz for drivers
Comment 10 Jay Patel [:jay] 2006-06-26 16:11:19 PDT
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 2006-07-26 01:30:42 PDT
dveditz, has this been fixed on trunk?
Comment 12 Daniel Veditz [:dveditz] 2006-07-26 02:54:28 PDT
Yes, fixed on June 22, rev=3.488 of nsGenericElement.cpp
Comment 13 Bob Clary [:bc:] 2006-08-22 00:31:20 PDT
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 2007-12-13 20:09:34 PST
Crashtest checked in.

Note You need to log in before you can comment on or make changes to this bug.