Last Comment Bug 705875 - XPConnect is passing null references into xpcom interfaces
: XPConnect is passing null references into xpcom interfaces
Status: RESOLVED FIXED
: crash, regression, testcase
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla11
Assigned To: Bobby Holley (busy)
:
Mentors:
Depends on:
Blocks: 326633 692342
  Show dependency treegraph
 
Reported: 2011-11-28 13:59 PST by Jesse Ruderman
Modified: 2011-12-02 03:25 PST (History)
6 users (show)
bobbyholley: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (crashes Firefox when loaded) (requires extension for CC) (310 bytes, text/html)
2011-11-28 13:59 PST, Jesse Ruderman
no flags Details
crash stack (8.33 KB, text/plain)
2011-11-28 13:59 PST, Jesse Ruderman
no flags Details
testcase 2 (132 bytes, text/html)
2011-11-29 16:01 PST, Jesse Ruderman
no flags Details
Bug 705875 - Check for null IID pointers and references in XPCConvert. v1 (2.01 KB, patch)
2011-12-01 10:31 PST, Bobby Holley (busy)
khuey: review+
Details | Diff | Review

Description Jesse Ruderman 2011-11-28 13:59:04 PST
Created attachment 577365 [details]
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 2011-11-28 13:59:38 PST
Created attachment 577367 [details]
crash stack
Comment 2 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-11-28 14:01:34 PST
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 [:khuey] (khuey@mozilla.com) 2011-11-28 14:14:22 PST
  window.QueryInterface(Components.interfaces.nsIInterfaceRequestor);
  window.getInterface(null);

in an unprivileged context is enough to crash my build.
Comment 4 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-11-28 14:19:08 PST
I'm going to guess this is fallout from Bug 692342.
Comment 5 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-11-28 14:20:59 PST
aIID is a null reference.
Comment 6 Jesse Ruderman 2011-11-29 16:01:28 PST
Created attachment 577782 [details]
testcase 2

Thanks for the reduction, Kyle.
Comment 7 Jesse Ruderman 2011-11-29 16:03:15 PST
Will the fix for bug 605271 (or new-dom-bindings) prevent this QI?
Comment 8 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-11-30 04:03:49 PST
Yes, but even with that there's still an actual XPConnect bug here.
Comment 9 Bobby Holley (busy) 2011-12-01 10:31:52 PST
Created attachment 578305 [details] [diff] [review]
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 [:khuey] (khuey@mozilla.com) 2011-12-01 10:36:43 PST
(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 (busy) 2011-12-01 11:20:22 PST
(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?
Comment 12 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-12-01 11:29:43 PST
(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 13 Bobby Holley (busy) 2011-12-01 11:52:58 PST
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 (busy) 2011-12-01 14:31:15 PST
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 (busy) 2011-12-01 18:27:26 PST
Looks good. Pushed to mozilla-inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/59fe691e50d3
Comment 16 Marco Bonardo [::mak] 2011-12-02 03:25:37 PST
https://hg.mozilla.org/mozilla-central/rev/59fe691e50d3

Note You need to log in before you can comment on or make changes to this bug.