1188590 - jsjni_GetGlobalClassRef goes reentrant on main

Status: RESOLVED FIXED

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

Description

[Byron Campen [:bwc]]

This has only been observed on android 4.0. (And only on mozilla-beta, since the android 4.0 tests are disabled on aurora and central) The patch from bug 1182289 detected this problem.

Comment 1

[Byron Campen [:bwc]]

Ok, diagnostics are in:

http://ftp.mozilla.org/pub/mozilla.org/mobile/try-builds/[e-mail]-9a9bdd9b7c40/try-android-api-11-debug/try_panda_android-debug_test-mochitest-5-bm89-tests1-panda-build1491.txt.gz

We are going reentrant in jsjni_GetGlobalClassRef, which is definitely android specific.

https://dxr.mozilla.org/mozilla-central/source/widget/android/AndroidJNIWrapper.cpp#65

Comment 2

[Byron Campen [:bwc]]

This might work:

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

Comment 3

[Byron Campen [:bwc]]

Ok, fixed the compile bug, looks good (there's a known perma-orange on android, but that's about it).

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b584fd6ab3f

Comment 4

[Byron Campen [:bwc]]

Attachment: Don't sync dispatch if we're already on main

Comment 5

[Byron Campen [:bwc]]

Attachment: (Beta only) Reenable mochitests for bug 1013809

Comment 6

[Byron Campen [:bwc]]

Comment on attachment 8640503 [details] [diff] [review]
(Beta only) Reenable mochitests for bug 1013809

Approval Request Comment

Just re-enabling some tests that were disabled to allow bug 1182289 to land, now that we've found the cause of the mystery reentrancy.

Comment 7

[Byron Campen [:bwc]]

Comment on attachment 8640498 [details] [diff] [review]
Don't sync dispatch if we're already on main

Approval Request Comment for aurora, beta, and esr38
[Feature/regressing bug #]:

   Bug 839836

[User impact if declined]:

   Content will probably be able to cause UAFs in webrtc code (and probably other code too) on android.

[Describe test coverage new/current, TreeHerder]:

   The patches from bug 1182289 uncovered this bug, because there are assertions that are tripped when main goes reentrant if CreateOrUpdateMediaPipeline is on the stack. These asserts do not cover all of the API in PeerConnection, nor do they cover other code that cannot tolerate main going reentrant.


[Risks and why]: 

   This patch alters timing (GetGlobalClassRef will finish earlier than stuff in main's event queue), so it is possible it could uncover other problems. However, I'm not seeing evidence of this on try.

[String/UUID change made/needed]:

   None.

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

   Since our mochitests for bug 1013809 hit this every time on android 4.0, it would seem pretty easy to hit this reliably. There are probably other ways to cause this bug too. It is hard to say how easy it would be to actually exploit.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

   The patch for re-enabling the tests for bug 1013809 on beta would provide a pretty good hint at how to trigger this bug, but the fix itself does not paint this kind of bullseye. We may wish to open a separate bug for the test patch, so the two are harder to link together.

Which older supported branches are affected by this flaw?

   All.

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

   I'm pretty sure the patch will apply cleanly. If not, it will be a trivial backport.

How likely is this patch to cause regressions; how much testing does it need?

   This patch alters timing (GetGlobalClassRef will finish earlier than stuff in main's event queue), so it is possible it could uncover other problems. However, I'm not seeing evidence of this on try.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8640498 [details] [diff] [review]
Don't sync dispatch if we're already on main

Android isn't supported on esr38

Comment 9

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

Comment on attachment 8640498 [details] [diff] [review]
Don't sync dispatch if we're already on main

Approvals given!

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8640503 [details] [diff] [review]
(Beta only) Reenable mochitests for bug 1013809

Test-only changes can land w/o approval.

Comment 11

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=67ee052e79c9

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0104fc2803cb
https://hg.mozilla.org/releases/mozilla-beta/rev/d87f96fc45b1
https://hg.mozilla.org/releases/mozilla-beta/rev/6b716448c59d

Calling this in-testsuite+ since we in fact have tests that caught the issue, albeit on beta and on a platform we're soon to not be testing on anymore.

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/67ee052e79c9

Comment 14

[Gian-Carlo Pascutto [:gcp]]

See bug 1189058. The issue there was detected on the media thread.

This bug will fix it for the main thread (and is a good idea) but it doesn't fix the underlying cause.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

I'm very worried about the fact that this was only caught by chance on an uplift to beta on a platform that's soon to be decommissioned.

Looking at the test manifest, we disable all WebRTC tests on Android 2.3/4.3 - http://hg.mozilla.org/mozilla-central/file/e9389ca320ff/dom/media/tests/mochitest/mochitest.ini#l4. Looks like this goes back to when 2.3/4.3 were enabled to begin with.

Maire, any idea what needs to be done to get these running again on 41+? We obviously got lucky this time.

Comment 16

[Maire Reavy [:mreavy]]

I've opened a new bug and made it a P1: Bug 1189784. I gave Catlee a heads up that we'd likely need someone from releng to assist, and kmoir has already pinged me to ask how she can help.