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)
:
:
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 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] (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 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 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 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 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] (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 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 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 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 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 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 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 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 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.