705875 - XPConnect is passing null references into xpcom interfaces

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded) (requires extension for CC)

1. Install https://www.squarefree.com/extensions/domFuzzLite3.xpi
2. Load the testcase

(typeof window.getInterface) changes from "undefined" to "function" when the cycle collector runs.  And calling the function crashes [@ nsGlobalWindow::GetInterface].

Maybe a regression from recent cycle collector changes?

Comment 1

[Jesse Ruderman]

Attachment: crash stack

Comment 2

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

I doubt this has anything to do with cycle collection, and everything to do with what fuzzPriv.CC does to get at dom window utils.

Comment 3

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

  window.QueryInterface(Components.interfaces.nsIInterfaceRequestor);
  window.getInterface(null);

in an unprivileged context is enough to crash my build.

Comment 4

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

I'm going to guess this is fallout from Bug 692342.

Comment 5

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

aIID is a null reference.

Comment 6

[Jesse Ruderman]

Attachment: testcase 2

Thanks for the reduction, Kyle.

Comment 7

[Jesse Ruderman]

Will the fix for bug 605271 (or new-dom-bindings) prevent this QI?

Comment 8

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

Yes, but even with that there's still an actual XPConnect bug here.

Comment 9

[Bobby Holley (:bholley)]

Attachment: Bug 705875 - Check for null IID pointers and references in XPCConvert. v1

This is a regression from http://hg.mozilla.org/mozilla-central/rev/c428312abbc7 . The rest of the changes in that patch should be fine.

Pushed this to try: https://tbpl.mozilla.org/?tree=Try&rev=898f757866f3

When it goes green I'll flag for review. Hopefully we don't rely on passing null IIDs anywhere.

Comment 10

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

(In reply to Bobby Holley (:bholley) from comment #9)
> When it goes green I'll flag for review. Hopefully we don't rely on passing
> null IIDs anywhere.

We'd better not, because that's not valid C++!

Comment 11

[Bobby Holley (:bholley)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #10)
> (In reply to Bobby Holley (:bholley) from comment #9)
> > When it goes green I'll flag for review. Hopefully we don't rely on passing
> > null IIDs anywhere.
> 
> We'd better not, because that's not valid C++!

No, I mean in the case where we pass an IID pointer rather than an IID reference. I think we support both?

Comment 12

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

(In reply to Bobby Holley (:bholley) from comment #11)
> (In reply to Kyle Huey [:khuey] ([e-mail]) from comment #10)
> > (In reply to Bobby Holley (:bholley) from comment #9)
> > > When it goes green I'll flag for review. Hopefully we don't rely on passing
> > > null IIDs anywhere.
> > 
> > We'd better not, because that's not valid C++!
> 
> No, I mean in the case where we pass an IID pointer rather than an IID
> reference. I think we support both?

Oh, idk.

Comment 13

[Bobby Holley (:bholley)]

Comment on attachment 578305 [details] [diff] [review]
Bug 705875 - Check for null IID pointers and references in XPCConvert. v1

All the tests pass on linux, which is good enough for me.

Flagging khuey for review.

Comment 14

[Bobby Holley (:bholley)]

This ended up with a bunch of oranges on mochitest-oth, which all seem to be related to some stuff already on inbound. But I rebased and am doing another try push (for mochitest-o only) to be sure:

https://tbpl.mozilla.org/?tree=Try&rev=8a115e35ad03

Comment 15

[Bobby Holley (:bholley)]

Looks good. Pushed to mozilla-inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/59fe691e50d3

Comment 16

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/59fe691e50d3