1353313 - Intermittent PROCESS-CRASH | dom/media/test/test_streams_element_capture_createObjectURL.html | application crashed [@ mozilla::RemoteDataDecoder::Error]

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P1)

Description

[Carsten Book [:Tomcat]]

https://treeherder.mozilla.org/logviewer.html#?job_id=88432119&repo=mozilla-central

filing as sec-bug just in case for the Crash address: 0xe5e5e65d adress 

PROCESS-CRASH | dom/media/test/test_streams_element_capture_createObjectURL.html | application crashed [@ mozilla::RemoteDataDecoder::Error]
Crash dump filename: /tmp/tmpHR_n_R/15e693df-9e1c-42af-04f1-b363e8aa2529.dmp
Operating system: Android
                  0.0.0 Linux 3.10.40-gc16a3c6 #1 SMP PREEMPT Tue Jul 28 17:58:25 UTC 2015 armv7l
CPU: arm
     ARMv1 Qualcomm Krait features: swp,half,thumb,fastmult,vfpv2,edsp,neon,vfpv3,tls,vfpv4,idiva,idivt
     4 CPUs

GPU: UNKNOWN

Crash reason:  SIGSEGV
Crash address: 0xe5e5e65d
Process uptime: not available

Thread 21 (crashed)
 0  libxul.so!mozilla::RemoteDataDecoder::Error [RefPtr.h:b5d8b27a7537 : 284 + 0x0]
     r0 = 0xe5e5e5e5    r1 = 0x949df868    r2 = 0x00000009    r3 = 0x9e3c5ea8
     r4 = 0x83cfcb10    r5 = 0xe5e5e5e5    r6 = 0x9cdc34ad    r7 = 0x00000001
     r8 = 0x75c99628    r9 = 0xb4a20000   r10 = 0x00000000   r12 = 0x80000000
     fp = 0x00000001    sp = 0x949df840    lr = 0x9cdc189d    pc = 0x9cdc32a6
    Found by: given as instruction pointer in context
 1  libxul.so!mozilla::jni::NativeStub<mozilla::java::CodecProxy::NativeCallbacks::OnError_t, mozilla::JavaCallbacksSupport, mozilla::jni::Args<bool> >::Wrap<&mozilla::JavaCallbacksSupport::OnError> [JavaCallbacksSupport.h:b5d8b27a7537 : 61 + 0x5]
     r4 = 0x83cfcb10    r5 = 0x949df868    r6 = 0x9cdc34ad    r7 = 0x00000001
     r8 = 0x75c99628    r9 = 0xb4a20000   r10 = 0x00000000    fp = 0x00000001
     sp = 0x949df868    pc = 0x9cdc189d
    Found by: call frame info
 2  data@[e-mail][e-mail] + 0xd78063
     r4 = 0x00000055    r5 = 0x13139720    r6 = 0x130602e0    r7 = 0x00000001
     r8 = 0x75c99628    r9 = 0xb4a20000   r10 = 0x00000000    fp = 0x00000001
     sp = 0x949df890    pc = 0xa2bc7065
    Found by: call frame info
 3  dalvik-non moving space (deleted) + 0x154e
     sp = 0x949df894    pc = 0x75bf4550
    Found by: stack scanning
 4  dalvik-main space (deleted) + 0x53971e
     sp = 0x949df8a0    pc = 0x13139720
    Found by: stack scanning
 5  dalvik-main space (deleted) + 0x53971e
     sp = 0x949df8a8    pc = 0x13139720
    Found by: stack scanning
 6  dalvik-main space (deleted) + 0x4602de
     sp = 0x949df8ac    pc = 0x130602e0
    Found by: stack scanning
 7  dalvik-non moving space (deleted) + 0xa6626
     sp = 0x949df8b4    pc = 0x75c99628
    Found by: stack scanning
 8  data@[e-mail][e-mail] + 0xee6fc7
     sp = 0x949df8c0    pc = 0xa2d35fc9
    Found by: stack scanning
 9  dalvik-non moving space (deleted) + 0xa6626
     sp = 0x949df8c4    pc = 0x75c99628
    Found by: stack scanning
10  dalvik-main space (deleted) + 0x53971e
     sp = 0x949df8c8    pc = 0x13139720
    Found by: stack scanning
11  dalvik-main space (deleted) + 0x5d3fe
     sp = 0x949df8d0    pc = 0x12c5d400
    Found by: stack scanning
12  system@[e-mail] + 0x1e63c6
     sp = 0x949df8d4    pc = 0x70d013c8
    Found by: stack scanning
13  dalvik-main space (deleted) + 0x4602de
     sp = 0x949df8e0    pc = 0x130602e0
    Found by: stack scanning
14  dalvik-non moving space (deleted) + 0xa6696
     sp = 0x949df8e8    pc = 0x75c99698
    Found by: stack scanning
15  dalvik-main space (deleted) + 0x4602de
     sp = 0x949df8ec    pc = 0x130602e0
    Found by: stack scanning
16  data@[e-mail][e-mail] + 0xee702b
     sp = 0x949df8f0    pc = 0xa2d3602d
    Found by: stack scanning
17  dalvik-non moving space (deleted) + 0xa6696
     sp = 0x949df8f4    pc = 0x75c99698
    Found by: stack scanning
18  dalvik-zygote space (deleted) + 0x160f3ce
     sp = 0x949df8f8    pc = 0x75a883d0
    Found by: stack scanning
19  dalvik-non moving space (deleted) + 0xa681e
     sp = 0x949df8fc    pc = 0x75c99820
    Found by: stack scanning
20  dalvik-non moving space (deleted) + 0xa681e
     sp = 0x949df900    pc = 0x75c99820
    Found by: stack scanning
21  dalvik-non moving space (deleted) + 0xa681e
     sp = 0x949df904    pc = 0x75c99820
    Found by: stack scanning
22  dalvik-main space (deleted) + 0x5d3fe
     sp = 0x949df90c    pc = 0x12c5d400
    Found by: stack scanning
23  data@[e-mail][e-mail] + 0xee6d89
     sp = 0x949df910    pc = 0xa2d35d8b

Comment 1

[Carsten Book [:Tomcat]]

Attachment: more complete stack

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

dom/media/test/ is playback, not webrtc
gcp: any ideas what's going on here?  there's very little usable stack to work off of

Comment 3

[Jean-Yves Avenard [:jya]]

John, this is your area of expertise

Comment 4

[Gian-Carlo Pascutto [:gcp]]

Look for an owner for:

mozilla::java::CodecProxy
mozilla::RemoteDataDecoder

because that's all we have to go on.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Munro?  You've been touching RemoteDataDecoder.cpp and CodecProxy

Comment 6

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

After reverting all my patches in bug 1265755, the issue still occurs.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7499941336eecde0ff7d38f542e2bb8cf41bdc0b&selectedJob=89082877

Comment 7

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Attachment: Bug1353313-Use-RefPtr-to-hold-RemoteVideoDecoder.patch

Comment 8

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Attachment: Bug1353313-Use-RefPtr-to-hold-RemoteVideoDecoder.patch

Comment 9

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Attachment: Bug1353313-Add-synchronization-mechanism-to-JavaCallbacksSupport.patch

Comment 10

[John Lin [:jhlin][:jolin]]

Comment on attachment 8856790 [details] [diff] [review]
Bug1353313-Use-RefPtr-to-hold-RemoteVideoDecoder.patch

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

Using RefPtr here doesn't seem helpful to me. This extra ref count will be decreased when |mJavaCallbacks| goes away in |ProcessShutdown()| and won't extend the life of |RemoteDataDecoder|.

It looks like the issue is a race condition like the following:
- OnError() checks |mCancel|
- ProcessShutdown() preempts and resolves promise
- RemoteDataDecoder is destructed
- OnError() resumes
- UAF!

Comment 11

[John Lin [:jhlin][:jolin]]

Comment on attachment 8856791 [details] [diff] [review]
Bug1353313-Add-synchronization-mechanism-to-JavaCallbacksSupport.patch

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

::: dom/media/platforms/android/JavaCallbacksSupport.h
@@ +19,5 @@
>    using Base::AttachNative;
>    using Base::DisposeNative;
>  
> +  JavaCallbacksSupport() :
> +    mCanceled(false),

Coding style: use ':' and ',' to start a new line for initialization list.

@@ +77,5 @@
>      mCanceled = true;
>    }
>  
>  private:
>    Atomic<bool> mCanceled;

With |mMutex|, |Atomic<>| is no longer needed.

Comment 12

[Jean-Yves Avenard [:jya]]

Comment on attachment 8856791 [details] [diff] [review]
Bug1353313-Add-synchronization-mechanism-to-JavaCallbacksSupport.patch

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

There's a call to "Cancel" to ensure that the JavaCallBackSupport object is no longer called by the JNI.

Adding a mutex to ensure that the OnBlah function may help as it adds some delay, but it doesn't fundamentally resolve the problem that the JNI may call the callback object once it's been destructed.

There needs to be another mechanism to ensure that the callback object isn't used *at all*. Not that the functions of the callback do nothing

::: dom/media/platforms/android/JavaCallbacksSupport.h
@@ +25,4 @@
>  
> +  virtual ~JavaCallbacksSupport() {
> +    MutexAutoLock lock(mMutex);
> +    mCanceled = true;

this is either unnecessary or very wrong. After the destructor has been called, this is freed. and access to those members will lead to a UAF.

If you rely on the destructor holding on a mutex to make sure the remaining of the code access this safely that can't be, and this is an undefined behaviour.

Comment 13

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

(In reply to Jean-Yves Avenard [:jya] from comment #12)
> Comment on attachment 8856791 [details] [diff] [review]
> Bug1353313-Add-synchronization-mechanism-to-JavaCallbacksSupport.patch
> 
> Review of attachment 8856791 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> There's a call to "Cancel" to ensure that the JavaCallBackSupport object is
> no longer called by the JNI.
> 
> Adding a mutex to ensure that the OnBlah function may help as it adds some
> delay, but it doesn't fundamentally resolve the problem that the JNI may
> call the callback object once it's been destructed.
> 
> There needs to be another mechanism to ensure that the callback object isn't
> used *at all*. Not that the functions of the callback do nothing
> 
> ::: dom/media/platforms/android/JavaCallbacksSupport.h
> @@ +25,4 @@
> >  
> > +  virtual ~JavaCallbacksSupport() {
> > +    MutexAutoLock lock(mMutex);
> > +    mCanceled = true;
> 
> this is either unnecessary or very wrong. After the destructor has been
> called, this is freed. and access to those members will lead to a UAF.
> 
> If you rely on the destructor holding on a mutex to make sure the remaining
> of the code access this safely that can't be, and this is an undefined
> behaviour.

For the onBlah function call after CallBackSupport dtor, this would cause a UAF indeed. I will find out another way to ensure the callback object isn't used at all.

However, for the onBlah function call before dtor, I think this mutex is still needed to hold the CallBackSupport object before the onBlah function finishes.

Do you agree?

Comment 14

[Jean-Yves Avenard [:jya]]

There are two issues to solve:

1- Ensuring that the callback object isn't used after it's been destructed
2- Ensuring that all functions currently in use (in another thread) in the callback object have completed.

The mutex change allows for 2. And calling Cancel() is sufficient.

But 1 needs to be resolved too

Comment 15

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Attachment: Bug1353313-Add-synchronization-mechanism-to-CodecProxy.patch

mochitest-media,mochitest-media-e10s:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9be8e3a54f4a9b95ac6ae3f18dfc276370fbc81

autophone-mochitest-dom-media:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4ef5d0ab608eb36f6b4db73ac4813786b924ae7
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ed75aebb0483c1243ec016cd3146be6de8156ec
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d5ad62e041a510209cf3e64cdfe0683f1c8a0494

Comment 16

[Jean-Yves Avenard [:jya]]

Comment on attachment 8858185 [details] [diff] [review]
Bug1353313-Add-synchronization-mechanism-to-CodecProxy.patch

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

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/CodecProxy.java
@@ +101,5 @@
>              reportError(fatal);
>          }
>  
> +        private synchronized void reportError(boolean fatal) {
> +            if (!mCodecProxyReleased) {

shouldn't the same test be done for all callbacks?

Comment 17

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

(In reply to Jean-Yves Avenard [:jya] from comment #16)
> Comment on attachment 8858185 [details] [diff] [review]
> Bug1353313-Add-synchronization-mechanism-to-CodecProxy.patch
> 
> Review of attachment 8858185 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/CodecProxy.
> java
> @@ +101,5 @@
> > reportError(fatal);
> > }
> > 
> > + private synchronized void reportError(boolean fatal) {
> > + if (!mCodecProxyReleased) {
> 
> shouldn't the same test be done for all callbacks?

There are quite a few regression bugs if we add the test to all callbacks:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a43dd2aafcbc66380f8ba8bbe93e5bfca9ac8c88
https://treeherder.mozilla.org/#/jobs?repo=try&revision=55cb1e7df136fc0ad91b2beeec74eb332fb9e2a3

I will address these in a following bug.

Comment 18

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Attachment: Bug1353313-Add-synchronization-mechanism-to-CodecProxy.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb0edb2223a0b70ac999c157df11992c652632bd
https://treeherder.mozilla.org/#/jobs?repo=try&revision=74160af6ea9caf0c6f85d6419da2cfaf6f3fa5c3

Comment 19

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Attachment: Bug1353313-Add-synchronization-mechanism-to-CodecProxy.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8bc7fc7cf7634c51b33aeaa6d7520196d37933b6

Comment 20

[John Lin [:jhlin][:jolin]]

Comment on attachment 8859416 [details] [diff] [review]
Bug1353313-Add-synchronization-mechanism-to-CodecProxy.patch

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

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/CodecProxy.java
@@ +85,4 @@
>          }
>  
>          @Override
> +        public synchronized void onOutput(Sample sample) throws RemoteException {

Please dispose sample if proxy is dead.

Comment 21

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Attachment: Bug1353313-Add-synchronization-mechanism-to-CodecProxy.patch

carry r+ from John

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8624f5eea6b931f41018001e675f0e83c012fc34

Comment 22

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8859470 [details] [diff] [review]
Bug1353313-Add-synchronization-mechanism-to-CodecProxy.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Pretty hard

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No, just as the change we made, we made some function synchronized.

Which older supported branches are affected by this flaw?
back to 54

If not all supported branches, which bug introduced the flaw?
bug 1257777

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Will backport to beta

How likely is this patch to cause regressions; how much testing does it need?
Low risk to cause regression. We just solve a UAF bug.

Comment 23

[Al Billings [:abillings - ex-MoCo]]

sec-approval+. Please request approval on patches for backporting as well.

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab2aad34a96729279ca14caf4a7d72b7f4cd6506

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ab2aad34a967

Comment 28

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8859470 [details] [diff] [review]
Bug1353313-Add-synchronization-mechanism-to-CodecProxy.patch

Approval Request Comment
[Feature/Bug causing the regression]: bug 1257777
[User impact if declined]: intermittent crash
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: we just solved an UAF bug.
[String changes made/needed]: none

Comment 29

[Gerry Chang [:gchang]]

Comment on attachment 8859470 [details] [diff] [review]
Bug1353313-Add-synchronization-mechanism-to-CodecProxy.patch

Fix a sec-high. Beta54+. Should be in 54 beta 2.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1b0848711aae