308120 - Infinite recursion crash in frame construction involving XBL

Status: VERIFIED FIXED

(Core :: XBL, defect)

Description

[Jesse Ruderman]

Clicking the button in the testcase crashes Firefox.  The stack consists mostly
of four functions repeating:

nsCSSFrameConstructor::ConstructXULFrame()
nsCSSFrameConstructor::ConstructFrameInternal()
nsCSSFrameConstructor::ConstructFrame()
nsCSSFrameConstructor::ProcessChildren()
nsCSSFrameConstructor::ConstructXULFrame()

Comment 1

[Jesse Ruderman]

Attachment: simple testcase

Comment 2

[Jesse Ruderman]

Same crash on Windows trunk: TB9265863H

Comment 3

[Boris Zbarsky [:bzbarsky]]

Ah, yeah.  We end up thinking that the anon <hbox> is a child of itself because
the binding looks like this:

184       <xul:hbox class="box-inherit button-box"
xbl:inherits="align,dir,pack,orient"
185                 align="center" pack="center" flex="1">
186         <children>

So we stick that <hbox> in the real kids of the button, which makes it appear to
be an insertion parent for itself.

I think we should just throw any time an element with a binding parent is moved
outside that binding's scope (so appended to any element with a different
binding parent) via DOM methods.  Anyone have any objections to that approach?

Comment 4

[[e-mail]]

What about the similar cases of removing an anonymous node containing an
insertion point before, or after, appending a node that would have been
appended to that insertion point? (You might need to have nested anonymous
nodes so that the removed node would have a parent to be removed from).

Comment 5

[Boris Zbarsky [:bzbarsky]]

Yeah, that probably breaks too.  XBL in general does not deal at all well with
mutations, especially "unapproved" ones.

I don't think we can disable access to XBL anonymous content for non-chrome,
though...

Comment 6

[Jesse Ruderman]

This particular crash is infinite recursion and so it isn't exploitable, but
based on the results of bc's tests I think there are other XBL crashes that are
exploitable.  bz's solution in comment 3 might fix them, so I'm marking this bug
as [sg:fix].

Comment 7

[Boris Zbarsky [:bzbarsky]]

The idea in comment 3 would break the use of document fragments in bindings,
unfortunately...

Comment 8

[Asa Dotzler [:asa]]

bz, can you or neil dig further into this? 

Comment 9

[Boris Zbarsky [:bzbarsky]]

Not in time for 1.8, I don't think.  We would need to define exactly what
operations should throw (I'm not even close to being able to do that), and then
probably do a lot of XBL/content surgery to fix.

In any case, I don't think this bug is a priority for 1.8...

Comment 10

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

I'll take this one

Comment 11

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

Throwing is the wrong thing to do here IMHO. The operation is perfectly legal, we just deal with it the wrong way. My guess is that we don't unbind the node from the tree first since we don't see a parent.

Comment 12

[Boris Zbarsky [:bzbarsky]]

I suspect we unbind the node fine, but we don't change the insertion point table, so there is an insertion point that points to that node...

Comment 13

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

Attachment: Branch safe patch

This makes us throw if someone tries to move that is the 'top level' anonymous node elsewhere in the tree.

This is what we should do for the 1.8 branch so lets do it on the trunk too for now. Ultimatly I want to make the operation work rather then throw, but i'll create a separate trunk-only patch for that.

Comment 14

[Brian Ryner (not reading)]

Comment on attachment 204400 [details] [diff] [review]
Branch safe patch

>--- base/src/nsGenericElement.cpp	18 Nov 2005 13:39:41 -0000	3.425
>+++ base/src/nsGenericElement.cpp	29 Nov 2005 01:37:04 -0000
>@@ -3429,29 +3429,32 @@ nsGenericElement::doInsertBefore(nsIDOMN
> 
>     if (fragmentObs) {
>       NS_ASSERTION(count && !do_notify, "Unexpected state");
>       fragmentObs->Finish();
>     }
> 
>     doc_fragment->DropChildReferences();
>   } else {
>-    nsCOMPtr<nsIDOMNode> oldParent;
>-    res = aNewChild->GetParentNode(getter_AddRefs(oldParent));
>-
>-    if (NS_FAILED(res)) {
>-      return res;
>+    nsIContent* bindingParent = newContent->GetBindingParent();
>+    if (bindingParent == newContent ||
>+        (bindingParent && bindingParent == newContent->GetParent())) {
>+      // We can't deal with this so just bail
>+      return NS_ERROR_DOM_NOT_SUPPORTED_ERR;

Does this only need to check newContent's parent or does it also need to check other ancestors up the tree?  I'm not sure of the answer, so as long as you are, sr=me.

Comment 15

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

We do not need to check anscestors. Moving a node that is 'deep' in the anonymous tree should work fine since then we manage to unbind the node fine and we don't need to update any anonymous-children lists.

Comment 16

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

Filed bug 318159 on getting things to work 'right'.

Marking this fixed though it still needs to be checked in to the 1.8 branch. There is still no flag to request branch approval though.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 204400 [details] [diff] [review]
Branch safe patch

Er.. sure there is.

Comment 18

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

I was actually just going to land it on the 1.8.1 branch. I'll talk with the security folks here about where we want it.

Comment 19

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using trunk SeaMonkey build 2005-11-30-10 on Windows XP with testcase at: https://bugzilla.mozilla.org/attachment.cgi?id=195719&action=view

Comment 20

[Mike Schroepfer]

Comment on attachment 204400 [details] [diff] [review]
Branch safe patch

Please land on 1.8 and 1.8.1 branches.

Comment 21

[Boris Zbarsky [:bzbarsky]]

*** Committing to MOZILLA_1_8_BRANCH... 
new revision: 3.395.2.7; previous revision: 3.395.2.6

*** Committing content/base/src/nsGenericElement.cpp on MOZILLA_1_8_0_BRANCH... 
new revision: 3.395.2.6.2.1; previous revision: 3.395.2.6

Comment 22

[Marcia Knous [:marcia]]

verified fixed on the branch using Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.1) Gecko/20060110 Firefox/1.5.0.1 (using Jesse's testcase in Comment 1) Adding verified1.8.0.1 keyword.

Comment 23

[Jesse Ruderman]

Crashtest checked in.