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

RESOLVED FIXED

Status

()

Core
DOM
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: Jesse Ruderman, Assigned: sicking)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
PowerPC
Mac OS X
crash, testcase, verified1.8.0.5, verified1.8.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9a1 +
blocking1.8.1 +
blocking1.8.0.5 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?], crash signature)

Attachments

(2 attachments)

(Reporter)

Description

11 years ago
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.
(Reporter)

Comment 1

11 years ago
Created attachment 222462 [details]
testcase (crashes Firefox)
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?]
Created attachment 222662 [details] [diff] [review]
patch
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?
(Reporter)

Comment 4

11 years ago
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.
(Reporter)

Comment 8

11 years ago
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+
Keywords: fixed1.8.0.5, fixed1.8.1

Comment 10

11 years ago
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.
Keywords: fixed1.8.0.5 → verified1.8.0.5
Whiteboard: [sg:moderate?] → [sg:critical?]
(Reporter)

Comment 11

11 years ago
dveditz, has this been fixed on trunk?
Yes, fixed on June 22, rev=3.488 of nsGenericElement.cpp
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 13

11 years ago
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

Keywords: fixed1.8.1 → verified1.8.1
Group: security
Flags: in-testsuite?
(Reporter)

Comment 14

10 years ago
Crashtest checked in.
Flags: in-testsuite? → in-testsuite+
Crash Signature: [@ nsAttrAndChildArray::RemoveChildAt]
You need to log in before you can comment on or make changes to this bug.