Crash with event listeners on mozCaptureStream()

VERIFIED FIXED in Firefox 21

Status

()

defect
--
critical
VERIFIED FIXED
7 years ago
4 months ago

People

(Reporter: jruderman, Assigned: roc)

Tracking

(Blocks 1 bug, 4 keywords)

Trunk
mozilla22
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox19 unaffected, firefox20 unaffected, firefox21+ verified, firefox22+ verified, firefox-esr17 unaffected, b2g18 unaffected)

Details

Attachments

(2 attachments)

For the last day or so, I've been getting lots of assertions/crashes involving mozCaptureStream() and event listeners.  Here's one of them.
It might be related to bug 842079.
I don't understand this. mozCaptureStream returns nsIDOMMediaStream object, and that isn't
an EventTarget - as far as I see. Yet we try to handle it as an EventTarget?
> mozCaptureStream returns nsIDOMMediaStream object, and that isn't
> an EventTarget - as far as I see.

You may be looking in the wrong place (like mxr for mozilla-central).  It doesn't seem to show the code after bug 837034 happened.

The WebIDL checked in for that bug has:

>+interface MediaStream : EventTarget {

but the C++ class has:

>+class DOMMediaStream : public nsIDOMMediaStream,
>+                       public nsWrapperCache

which is very much not inheriting from EventTarget.  So the EventTarget things end up on the proto chain, reinterpret_cast to EventTarget, call virtual methods, and we get this security bug.
Blocks: 837034
Group: core-security
Our MDC page for MediaStream doesn't seem to have any useful spec links, so I can't tell whether it's correct that this is an EventTarget or not.  If it is, then it needs to actually do EventTarget bits...
Flags: needinfo?(rjesup)
Using the wrong vtable sounds bad, so marking sec-critical.
We don't implement any EventTarget stuff yet, so I guess we just need to remove EventTarget from MediaStream.webidl.
Or inherit from nsDOMEventTargetHelper get all the EventTarget bits working, yes.  For 21, changing the WebIDL is probably safer.
It seems to me that this kind of mistake should cause a build failure.
Comment on attachment 715004 [details] [diff] [review]
fix

r=me

> It seems to me that this kind of mistake should cause a build failure.

Yeah.

I might in fact be able to turn this into a build failure, and some related problems into runtime fatal asserts that will always trigger (we have some asserts along those lines, but they only trigger in certain limited cases which don't apply here).  I'll poke at that on Tuesday.
Attachment #715004 - Flags: review?(bzbarsky) → review+
Assignee: nobody → roc
OS: Mac OS X → All
Hardware: x86_64 → All
(In reply to Boris Zbarsky (:bz) from comment #3)
> You may be looking in the wrong place (like mxr for mozilla-central).  It
> doesn't seem to show the code after bug 837034 happened.

Uh, indeed. mxr hasn't still been updated...
> I might in fact be able to turn this into a build failure

Filed bug 842561.
Duplicate of this bug: 842721
Comment on attachment 715004 [details] [diff] [review]
fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 837034
User impact if declined: Probably-exploitable crashes
Testing completed (on m-c, etc.): Passes tests
Risk to taking this patch (and alternatives if risky): Very very low risk
String or UUID changes made by this patch: None
Attachment #715004 - Flags: review-
Attachment #715004 - Flags: review+
Attachment #715004 - Flags: approval-mozilla-aurora?
Flags: needinfo?(rjesup)
bz: did you mean to r- the patch you r+'d and also at the same time propose it for Aurora uplift?
Comment on attachment 715004 [details] [diff] [review]
fix

No, didn't mean to change the state of the "r".  ;)
Attachment #715004 - Flags: review- → review+
http://hg.mozilla.org/mozilla-central/rev/21d3ff83095f
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Attachment #715004 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flagging for verification in Firefox 21 and 22.
Keywords: verifyme
(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #20)
> Flagging for verification in Firefox 21 and 22.

Kamil, can you please help us with verifying this bug?
Reproduced the issue using the following build (M-C):
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-02-22-03-11-33-mozilla-central/

Reproduced the issue using the following build (Aurora):
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-02-22-11-12-54-mozilla-aurora/

Once I reproduced the problems, went through the following builds to make sure the issue has been fixed:

Went through the attached testcase using the latest M-C build (Passed):
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-03-12-03-10-46-mozilla-central/

Went through the attached testcase using the latest Aurora build (Passed):
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-03-12-04-20-13-mozilla-aurora/
Status: RESOLVED → VERIFIED
Thanks Kamil.
Flags: in-testsuite?
Keywords: verifyme
Group: core-security
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.