894348 - MediaRecorder use-after-free crash [@mozilla::MediaStream::GraphImpl]

Status: VERIFIED FIXED

(Core :: Audio/Video: Recording, defect)

Description

[Christoph Diehl [:posidron]]

Attachment: testcase

alloc & free: dom/bindings/MediaRecorderBinding.cpp:356

  static bool
  start(JSContext* cx, JS::Handle<JSObject*> obj, mozilla::dom::MediaRecorder* self, const JSJitMethodCallArgs& args)
  {
    Optional<int32_t > arg0;
    if (0 < args.length()) {
      arg0.Construct();
      if (!ValueToPrimitive<int32_t, eDefault>(cx, args.handleAt(0), &arg0.Value())) {
        return false;
      }
    }
    ErrorResult rv;
*   self->Start(Constify(arg0), rv);
[...]


re-use: content/media/MediaStreamGraph.cpp:1495

  MediaStream::GraphImpl()
  {
*    return mGraph;
  }


Tested with http://hg.mozilla.org/integration/mozilla-inbound/rev/860f92c88a13

Comment 1

[Christoph Diehl [:posidron]]

Attachment: callstack

Comment 2

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Hi Christoph, 
It seems like a F5 reload stress test, right?

Comment 3

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: patch v1 with test case

oh, wrong usage of poniter....
BTW, Do you know how to test this behavior with mochitest?

Comment 4

[Christoph Diehl [:posidron]]

(In reply to Randy Lin [:rlin] from comment #3)
> BTW, Do you know how to test this behavior with mochitest?

Not really, let's Cc Jason Smith.

Comment 5

[Christoph Diehl [:posidron]]

(In reply to Randy Lin [:rlin] from comment #2)
> Hi Christoph, 
> It seems like a F5 reload stress test, right?

In my tries, only one reload was necessary.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 777631 [details] [diff] [review]
patch v1 with test case

Review of attachment 777631 [details] [diff] [review]:
-----------------------------------------------------------------

The code change is correct (ew!) but the test doesn't work since it will constantly reload.

You could try making the reference to the MediaRecorder be a local variable in a function that you call once. After the function is called, do SpecialPowers.gc() and that should destroy the ProcessedMediaStream too early. Hopefully that would make us crash most of the time.

Comment 7

[Jason Smith [:jsmith]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6)
> Comment on attachment 777631 [details] [diff] [review]
> patch v1 with test case
> 
> Review of attachment 777631 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The code change is correct (ew!) but the test doesn't work since it will
> constantly reload.
> 
> You could try making the reference to the MediaRecorder be a local variable
> in a function that you call once. After the function is called, do
> SpecialPowers.gc() and that should destroy the ProcessedMediaStream too
> early. Hopefully that would make us crash most of the time.

Another idea I have would be to do this with a crash test and local storage to re-run the same crashtest a few times on each reload.

See http://hg.mozilla.org/mozilla-central/file/16375716cc69/dom/media/tests/crashtests/801227.html as an example of a crashtest that demonstrated a way to do this.

Comment 8

[David Bolter [:davidb] (NeedInfo me for attention)]

Nice work. Does this affect any branches?

Comment 9

[Jason Smith [:jsmith]]

(In reply to David Bolter [:davidb] from comment #8)
> Nice work. Does this affect any branches?

Should only be affecting trunk. The Media Recorder API landed in the Firefox 25 timeframe.

Comment 10

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: patvh v2

Use for loop and force gc to capture this crash.

Comment 11

[Jason Smith [:jsmith]]

Comment on attachment 778248 [details] [diff] [review]
patvh v2

Review of attachment 778248 [details] [diff] [review]:
-----------------------------------------------------------------

Can kick off a try run with this? This is the first time we are checking in a MediaRecorder test into the tree, so there potentially could be other failures we might see on try.

::: content/media/test/test-MediaRecorder-reload.html
@@ +28,5 @@
> +    try { o1 = o0.mozCaptureStreamUntilEnded(); } catch(e) { }
> +    try { o2 = new MediaRecorder(o1) } catch(e) { }
> +    try { o2.start(2048) } catch(e) { }
> +    try { o2.start(0) } catch(e) { }
> +    SpecialPowers.gc();

Did you confirm this test invokes the crash without your fix applied?

@@ +30,5 @@
> +    try { o2.start(2048) } catch(e) { }
> +    try { o2.start(0) } catch(e) { }
> +    SpecialPowers.gc();
> +  }
> +  ok(true, "pass the crash test");

If you are intending to do a crashtest, why not actually use the framework here?

Comment 12

[Jason Smith [:jsmith]]

Comment on attachment 778248 [details] [diff] [review]
patvh v2

Ack. Bugzilla conflict. Sorry about that.

Comment 13

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

(In reply to Jason Smith [:jsmith] from comment #11)
> Comment on attachment 778248 [details] [diff] [review]
> patvh v2
> 
> Review of attachment 778248 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can kick off a try run with this? This is the first time we are checking in
> a MediaRecorder test into the tree, so there potentially could be other
> failures we might see on try.
> 
> ::: content/media/test/test-MediaRecorder-reload.html
> @@ +28,5 @@
> > +    try { o1 = o0.mozCaptureStreamUntilEnded(); } catch(e) { }
> > +    try { o2 = new MediaRecorder(o1) } catch(e) { }
> > +    try { o2.start(2048) } catch(e) { }
> > +    try { o2.start(0) } catch(e) { }
> > +    SpecialPowers.gc();
> 
> Did you confirm this test invokes the crash without your fix applied?
> 
> @@ +30,5 @@
> > +    try { o2.start(2048) } catch(e) { }
> > +    try { o2.start(0) } catch(e) { }
> > +    SpecialPowers.gc();
> > +  }
> > +  ok(true, "pass the crash test");
> 
> If you are intending to do a crashtest, why not actually use the framework
> here?

I have tested this before apply the fix and can capture this crash.

Comment 14

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Oh, more crash was found after run test case on different platform, I will fix it also.
https://tbpl.mozilla.org/?tree=Try&rev=919b234f6115

Comment 15

[Jason Smith [:jsmith]]

Thanks for doing that try run - good to see we found more crashes.

Crash info from try run:

PROCESS-CRASH | Shutdown | application crashed [@ nsTArray_Impl<nsAutoPtr<mozilla::DOMMediaStream::OnTracksAvailableCallback>, nsTArrayInfallibleAllocator>::Clear()]

03:52:59 WARNING - PROCESS-CRASH | Shutdown | application crashed [@ mozilla::DOMMediaStream::NotifyMediaStreamGraphShutdown()]

04:13:36 WARNING - PROCESS-CRASH | Shutdown | application crashed [@ nsAutoPtr<mozilla::DOMMediaStream::OnTracksAvailableCallback>::`scalar deleting destructor'(unsigned int) + 0xc] 

03:19:18 WARNING - PROCESS-CRASH | Shutdown | application crashed [@ 0x7b68404a]

03:50:37 WARNING - PROCESS-CRASH | Shutdown | application crashed [@ nsTArray_Impl<nsAutoPtr<mozilla::DOMMediaStream::OnTracksAvailableCallback>, nsTArrayInfallibleAllocator>::RemoveElementsAt(unsigned int, unsigned int)]

Comment 16

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: patch v3

Fix crash, also fix memory leaks problem.
try result.
https://tbpl.mozilla.org/?tree=Try&rev=9a3bea311de9

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 778862 [details] [diff] [review]
patch v3

Review of attachment 778862 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/MediaRecorder.cpp
@@ +115,5 @@
>  
>  MediaRecorder::~MediaRecorder()
>  {
> +  if (mTrackUnionStream)
> +    mTrackUnionStream->Destroy();

{}

Comment 18

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: check-in.patch

carry reviewer, fix nits.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

This needs sec-approval, no?

Comment 20

[Timothy B. Terriberry (:derf)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #19)
> This needs sec-approval, no?

MediaRecorder currently only exists on trunk. According to my understanding of <https://wiki.mozilla.org/Security/Bug_Approval_Process>, that means it does not need sec-approval. I'll leave it up to rlin if he wants to ask for it anyway.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Confirmed that with dveditz. I'll land next time I get a chance.

Comment 22

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Thanks Timothy, The mediaRecorder only affect the trunk right now.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2e6ca2dc655e

Comment 24

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2e6ca2dc655e

Comment 25

[Jason Smith [:jsmith]]

Verified via mochitest on landing.