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 (:bholley) (busy with Stylo)
:
: Andrew Overholt [:overholt]
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 (:bholley) (busy with Stylo)
khuey: review+
Details | Diff | Splinter Review

Description User image 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 User image Jesse Ruderman 2011-11-28 13:59:38 PST
Created attachment 577367 [details]
crash stack
Comment 2 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 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 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 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 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-28 14:19:08 PST
I'm going to guess this is fallout from Bug 692342.
Comment 5 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-28 14:20:59 PST
aIID is a null reference.
Comment 6 User image Jesse Ruderman 2011-11-29 16:01:28 PST
Created attachment 577782 [details]
testcase 2

Thanks for the reduction, Kyle.
Comment 7 User image Jesse Ruderman 2011-11-29 16:03:15 PST
Will the fix for bug 605271 (or new-dom-bindings) prevent this QI?
Comment 8 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-30 04:03:49 PST
Yes, but even with that there's still an actual XPConnect bug here.
Comment 9 User image Bobby Holley (:bholley) (busy with Stylo) 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 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 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 User image Bobby Holley (:bholley) (busy with Stylo) 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 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 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 User image Bobby Holley (:bholley) (busy with Stylo) 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 User image Bobby Holley (:bholley) (busy with Stylo) 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 User image Bobby Holley (:bholley) (busy with Stylo) 2011-12-01 18:27:26 PST
Looks good. Pushed to mozilla-inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/59fe691e50d3
Comment 16 User image 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.