Closed Bug 705875 Opened 8 years ago Closed 8 years ago

XPConnect is passing null references into xpcom interfaces

Categories

(Core :: XPConnect, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla11

People

(Reporter: jruderman, Assigned: bholley)

References

(Blocks 1 open bug)

Details

(Keywords: crash, regression, testcase)

Crash Data

Attachments

(3 files, 1 obsolete file)

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?
Blocks: 326633
Attached file crash stack
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.
  window.QueryInterface(Components.interfaces.nsIInterfaceRequestor);
  window.getInterface(null);

in an unprivileged context is enough to crash my build.
I'm going to guess this is fallout from Bug 692342.
Assignee: nobody → bobbyholley+bmo
Blocks: 692342
Component: DOM → XPConnect
OS: Mac OS X → All
QA Contact: general → xpconnect
Hardware: x86_64 → All
Summary: Crash with cycle collector, window.getInterface → XPConnect is passing null references into xpcom interfaces
Attached file testcase 2
Thanks for the reduction, Kyle.
Attachment #577365 - Attachment is obsolete: true
Will the fix for bug 605271 (or new-dom-bindings) prevent this QI?
Yes, but even with that there's still an actual XPConnect bug here.
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.
(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++!
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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?
(In reply to Bobby Holley (:bholley) from comment #11)
> (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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 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.
Attachment #578305 - Flags: review?(khuey)
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
Looks good. Pushed to mozilla-inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/59fe691e50d3
Flags: in-testsuite+
Target Milestone: --- → mozilla11
https://hg.mozilla.org/mozilla-central/rev/59fe691e50d3
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.