Closed Bug 338391 Opened 16 years ago Closed 16 years ago

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

Categories

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

PowerPC
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jruderman, Assigned: sicking)

References

Details

(4 keywords, Whiteboard: [sg:critical?])

Crash Data

Attachments

(2 files)

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.
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.
Assignee: general → dveditz
Blocks: 329982
Flags: blocking1.9a1+
Flags: blocking1.8.1+
Flags: blocking1.8.0.5+
Whiteboard: [sg:moderate?]
Attached patch patchSplinter Review
Attachment #222662 - Flags: superreview?
Attachment #222662 - Flags: review?
Attachment #222662 - Flags: approval-branch-1.8.1?
Attachment #222662 - Flags: superreview?(bugmail)
Attachment #222662 - Flags: superreview?
Attachment #222662 - Flags: review?(bugmail)
Attachment #222662 - Flags: review?
Attachment #222662 - Flags: approval-branch-1.8.1?(bugmail)
Attachment #222662 - Flags: approval-branch-1.8.1?
I'm guessing most of our compilers issued a warning that the comparison was useless, but nobody noticed? :/
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.
Attachment #222662 - Flags: superreview?(bugmail)
Attachment #222662 - Flags: superreview+
Attachment #222662 - Flags: review?(bugmail)
Attachment #222662 - Flags: review+
Attachment #222662 - Flags: approval-branch-1.8.1?(bugmail)
Attachment #222662 - Flags: approval-branch-1.8.1+
(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.

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

And yet it still doesn't catch this error.
gcc doesn't complain about anything in this file (with the default options for a debug Firefox build).  Weird.
Assignee: dveditz → bugmail
Comment on attachment 222662 [details] [diff] [review]
patch

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #222662 - Flags: approval1.8.0.5+
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.
Whiteboard: [sg:moderate?] → [sg:critical?]
dveditz, has this been fixed on trunk?
Yes, fixed on June 22, rev=3.488 of nsGenericElement.cpp
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
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

Group: security
Flags: in-testsuite?
Crashtest checked in.
Flags: in-testsuite? → in-testsuite+
Crash Signature: [@ nsAttrAndChildArray::RemoveChildAt]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.