Infinite recursion crash in frame construction involving XBL

VERIFIED FIXED

Status

()

Core
XBL
--
critical
VERIFIED FIXED
13 years ago
10 years ago

People

(Reporter: Jesse Ruderman, Assigned: sicking)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
crash, fixed1.8.1, testcase, verified1.8.0.1
Points:
---
Bug Flags:
blocking1.8b5 -
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:fix])

Attachments

(2 attachments)

(Reporter)

Description

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

Comment 1

13 years ago
Created attachment 195719 [details]
simple testcase
(Reporter)

Comment 2

13 years ago
Same crash on Windows trunk: TB9265863H
OS: MacOS X → All
Hardware: Macintosh → All
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

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

Comment 6

13 years ago
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].
Flags: blocking1.8b5?
Whiteboard: [sg:fix]
The idea in comment 3 would break the use of document fragments in bindings,
unfortunately...

Updated

13 years ago
Flags: blocking1.8b5? → blocking1.8b5-

Comment 8

13 years ago
bz, can you or neil dig further into this? 
Assignee: general → 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...
Assignee: bzbarsky → general
I'll take this one
Assignee: general → bugmail
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.
Status: NEW → ASSIGNED
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...
Created attachment 204400 [details] [diff] [review]
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.
Attachment #204400 - Flags: superreview?(bryner)
Attachment #204400 - Flags: review?
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.
Attachment #204400 - Flags: superreview?(bryner) → superreview+
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.
Attachment #204400 - Flags: review? → review?(bzbarsky)
Attachment #204400 - Flags: review?(bzbarsky) → review+
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Flags: blocking1.8.1?
Resolution: --- → FIXED
Comment on attachment 204400 [details] [diff] [review]
Branch safe patch

Er.. sure there is.
Attachment #204400 - Flags: approval1.8.0.1?
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.
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
Status: RESOLVED → VERIFIED

Comment 20

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

Please land on 1.8 and 1.8.1 branches.
Attachment #204400 - Flags: approval1.8.1+
Attachment #204400 - Flags: approval1.8.0.1?
Attachment #204400 - Flags: approval1.8.0.1+
*** 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
Keywords: fixed1.8.0.1, fixed1.8.1
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.
Keywords: verified1.8.0.1

Updated

12 years ago
Keywords: fixed1.8.0.1
Flags: blocking1.8.1?
(Reporter)

Comment 23

10 years ago
Crashtest checked in.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.