730441 - Crash in nsTreeColumns::RestoreNaturalOrder()

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Al Billings [:abillings - ex-MoCo]]

Attachment: PoC #1

This is derived from bug 708186. The reporter offered three additional PoCs that only affected 1.9.2. Since they were not SVG related, it was decided to open a new bug for the issue.

The first PoC crashes with bp-979112cd-d44b-45a3-81c1-aeb9c2120224 on my Mac.

The second PoC crashes at bp-1465ec42-6ccb-4703-91b4-569c72120224.

The third PoC crashes at bp-e0564e71-4740-407a-aa24-34eb22120224.

Comment 1

[Al Billings [:abillings - ex-MoCo]]

Attachment: PoC #2

Comment 2

[Al Billings [:abillings - ex-MoCo]]

Attachment: PoC #3

Comment 3

[Daniel Veditz [:dveditz]]

for PoC 1 and 2 I get
bp-b709db30-881f-4f46-9bbd-e25d12120224

For PoC 3 I get a crash at a slightly different place, nsTreeContentView::SetTree(nsITreeBoxObject*)
bp-6d2ee95e-24ed-443c-9a62-9c5ac2120224

In all three cases it's dying on a null tree, held by a nsCOMPtr so this is an unexploitable crash. Null means it's either never set or--as in this case--correctly cleaned up and isn't going to be some random pointer to something freed.

Comment 4

[Daniel Veditz [:dveditz]]

If you allow remote XUL (magic per-site permission setting) it still crashes nightly. bp-fe50ac32-c776-4f30-8688-c4f8d2120224

Not so much a user concern (no remote XUL in Firefox 4+) but means an add-on or Firefox code might end up with a stability problem.

Comment 5

[Timothy Nikkel (:tnikkel)]

PoC 1 and 2 can be fixed by programming defensively.

PoC 3 passes the javascript object {} to idl defined function setTree defined as

void setTree(in nsITreeBoxObject tree);

in layout/xul/base/src/tree/public/nsITreeView.idl. {} obviously doesn't implement the nsITreeBoxObject interface, but what is the proper way of determining that? Should this even be allowed at all?

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Timothy Nikkel (:tn) from comment #5)
> PoC 3 passes the javascript object {} to idl defined function setTree
> defined as
> 
> void setTree(in nsITreeBoxObject tree);
> 
> in layout/xul/base/src/tree/public/nsITreeView.idl. {} obviously doesn't
> implement the nsITreeBoxObject interface, but what is the proper way of
> determining that? Should this even be allowed at all?

Ah, but it does.  XPConnect is just insane here ... but I'm not sure that we can fix that.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Unless it's sane for nsITreeBoxObject to be implemented by JS, we should mark it builtinclass which will fix #3.

Comment 8

[Daniel Veditz [:dveditz]]

I don't think we need to worry about this one on the dying branch

Comment 9

[Mats Palmgren (inactive)]

Attachment: Fix+tests for #1 and #2

Comment 10

[Mats Palmgren (inactive)]

Attachment: Fix+test for #3

https://tbpl.mozilla.org/?tree=Try&rev=30cb7db0e35f

Comment 11

[Timothy Nikkel (:tnikkel)]

Comment on attachment 734247 [details] [diff] [review]
Fix+test for #3

>@@ -471,16 +471,20 @@ nsTreeContentView::SetTree(nsITreeBoxObj
>     nsCOMPtr<nsIBoxObject> boxObject = do_QueryInterface(mBoxObject);
>+    if (!boxObject) {
>+      mBoxObject = nullptr;
>+      return NS_ERROR_INVALID_ARG;
>+    }

This will ensure that it is at least a nsIBoxObject, but is it possible for the passed in object to be a nsIBoxObject but not a nsITreeBoxObject? I don't know the xpconnect magic that allows this fake nsITreeBoxObject so I'm not sure.

Comment 12

[Timothy Nikkel (:tnikkel)]

(In reply to Timothy Nikkel (:tn) from comment #11)
> Comment on attachment 734247 [details] [diff] [review]
> Fix+test for #3
> 
> >@@ -471,16 +471,20 @@ nsTreeContentView::SetTree(nsITreeBoxObj
> >     nsCOMPtr<nsIBoxObject> boxObject = do_QueryInterface(mBoxObject);
> >+    if (!boxObject) {
> >+      mBoxObject = nullptr;
> >+      return NS_ERROR_INVALID_ARG;
> >+    }
> 
> This will ensure that it is at least a nsIBoxObject, but is it possible for
> the passed in object to be a nsIBoxObject but not a nsITreeBoxObject? I
> don't know the xpconnect magic that allows this fake nsITreeBoxObject so I'm
> not sure.

QI'ing to nsISupports and then back nsITreeBoxObject should be enough to ensure we have a valid object, so we can do that if we don't get a better answer.

Comment 13

[Mats Palmgren (inactive)]

NS_IMETHODIMP
nsTreeContentView::SetTree(nsITreeBoxObject* aTree)

Hmm, I thought that the signature would guarantee that.
It's really a bug in the caller if a non-null aTree doesn't QI
to nsITreeBoxObject.

Anyway, I tested QI aTree to nsITreeBoxObject, both directly and via
nsISupports and both results in a pointer value === aTree.
I don't understand the XPConnect magic that makes that work either
but it seems like that extra check is pointless.

Comment 14

[Timothy Nikkel (:tnikkel)]

You're right. I thought QI'ing to nsISupports and then back nsITreeBoxObject wouldn't work.

The question is then what should we do about this?

The other thing I noticed is that nsITreeBoxObject doesn't inherirt from nsIBoxObject. So maybe we should set mBoxObject to null if aTree doesn't QI to nsIBoxObject?

Comment 15

[Mats Palmgren (inactive)]

The patch does set mBoxObject to null in that case (ClearRows() always
nulls out mRoot unconditionally); I don't think we can do more in SetTree.
This stuff isn't exposed to unprivileged content so I think we're fine.

I'll remove the !mRoot from the if-condition since it's always true.

Comment 16

[Timothy Nikkel (:tnikkel)]

Ok, that sounds good then.

Comment 17

[Timothy Nikkel (:tnikkel)]

Attachment: Fix+test for #3

This just does what comment 15 says. I got impatient. :)

Comment 18

[Mats Palmgren (inactive)]

Comment on attachment 740430 [details] [diff] [review]
Fix+test for #3

Thanks!

Comment 19

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/352ceffb0d9e
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6639d6743db

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/352ceffb0d9e
https://hg.mozilla.org/mozilla-central/rev/a6639d6743db