Closed Bug 945334 Opened 6 years ago Closed 6 years ago

MediaManager's JS Objects released on non-main threads

Categories

(Core :: WebRTC, defect, critical)

26 Branch
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla28
blocking-b2g koi+
Tracking Status
firefox25 --- wontfix
firefox26 --- wontfix
firefox27 + fixed
firefox28 + fixed
firefox-esr17 --- unaffected
firefox-esr24 27+ fixed
b2g18 --- unaffected
b2g-v1.1hd --- unaffected
b2g-v1.2 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed

People

(Reporter: sotaro, Assigned: jesup)

References

Details

(Keywords: crash, regression, sec-moderate, Whiteboard: [qa-][adv-main27+][adv-esr24.3+])

Attachments

(1 file, 1 obsolete file)

+++ This bug was initially created as a clone of Bug #929029 +++

From, Bug 929029 Comment 57, some MediaManager's JS Objects seems to be released on non main thread.
No longer blocks: 927884
Crash Signature: [@ nsSimpleURI::Release() ]
Whiteboard: [b2g-crash]
Copied from Bug 929029.

(In reply to Ryan VanderMeulen [:RyanVM UTC-5][Back 2-Dec] from comment #55)
> Backed out for crashes/hangs in test_dataChannel_basicAudio.html on all
> platforms. Please run this through Try before requesting checkin again.
> https://hg.mozilla.org/integration/b2g-inbound/rev/d99d3514fd6a
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=31322007&tree=B2g-Inbound
> https://tbpl.mozilla.org/php/getParsedLog.php?id=31321907&tree=B2g-Inbound
> https://tbpl.mozilla.org/php/getParsedLog.php?id=31322194&tree=B2g-Inbound
From the following log, JS object seems to be released on media thread. MediaOperationRunnable::Run() rund on media thread.

---------------------------------------------------------

09:05:02     INFO -  Assertion: Unknown assertion type 0x00000000
09:05:02     INFO -  Thread 25 (crashed)
09:05:02     INFO -   0  xul.dll!nsXPCWrappedJS::Release() [XPCWrappedJS.cpp:bc50783a4cf7 : 175 + 0x0]
09:05:02     INFO -      eip = 0x7333aca2   esp = 0x0a9ef6a4   ebp = 0x0a9ef6a4   ebx = 0x1008dbe0
09:05:02     INFO -      esi = 0x09fadee0   edi = 0x0a191514   eax = 0x00000000   ecx = 0xfeefd000
09:05:02     INFO -      edx = 0x09fadee4   efl = 0x00000246
09:05:02     INFO -      Found by: given as instruction pointer in context
09:05:02     INFO -   1  xul.dll!nsXPTCStubBase::Release() [xptcall.cpp:bc50783a4cf7 : 36 + 0xb]
09:05:02     INFO -      eip = 0x72d68e23   esp = 0x0a9ef6ac   ebp = 0x0a9ef6b0
09:05:02     INFO -      Found by: call frame info
09:05:02     INFO -   2  xul.dll!mozilla::GetUserMediaNotificationEvent::~GetUserMediaNotificationEvent() [MediaManager.h:bc50783a4cf7 : 223 + 0xc]
09:05:02     INFO -      eip = 0x733e4375   esp = 0x0a9ef6b8   ebp = 0x0a9ef6c8
09:05:02     INFO -      Found by: call frame info
09:05:02     INFO -   3  xul.dll!mozilla::GetUserMediaNotificationEvent::`scalar deleting destructor'(unsigned int) + 0xa
09:05:02     INFO -      eip = 0x733e43af   esp = 0x0a9ef6c4   ebp = 0x0a9ef6c8
09:05:02     INFO -      Found by: call frame info
09:05:02     INFO -   4  xul.dll!nsTransportEventSinkProxy::Release() [EventTokenBucket.cpp:bc50783a4cf7 : 46 + 0x26]
09:05:02     INFO -      eip = 0x72d9a2a1   esp = 0x0a9ef6d0   ebp = 0x0a9ef6d4
09:05:02     INFO -      Found by: call frame info
09:05:02     INFO -   5  xul.dll!mozilla::MediaOperationRunnable::Run() [MediaManager.h:bc50783a4cf7 : 373 + 0x9]
09:05:02     INFO -      eip = 0x733e7bd7   esp = 0x0a9ef6dc   ebp = 0x0a9ef6ec
09:05:02     INFO -      Found by: call frame info
09:05:02     INFO -   6  xul.dll!nsThread::ProcessNextEvent(bool,bool *) [nsThread.cpp:bc50783a4cf7 : 612 + 0x5]
09:05:02     INFO -      eip = 0x72d64698   esp = 0x0a9ef6f4   ebp = 0x0a9ef728
09:05:02     INFO -      Found by: call frame info
09:05:02     INFO -   7  xul.dll!NS_ProcessNextEvent(nsIThread *,bool) [nsThreadUtils.cpp:bc50783a4cf7 : 263 + 0xc]
09:05:02     INFO -      eip = 0x72d2ea74   esp = 0x0a9ef730   ebp = 0x0a9ef73c
09:05:02     INFO -      Found by: call frame info
09:05:02     INFO -   8  xul.dll!nsThread::ThreadFunc(void *) [nsThread.cpp:bc50783a4cf7 : 246 + 0x7]
09:05:02     INFO -      eip = 0x72d6617b   esp = 0x0a9ef744   ebp = 0x0a9ef758
09:05:02     INFO -      Found by: call frame info
To fix the problem, it might be better to use nsMainThreadPtrHandle and nsMainThreadPtrHolder like Bug 827503 and Bug 929029.
Blocks: 929029
No longer depends on: 929029
Summary: MediaManager's JS Objects release'd on non-main threads → MediaManager's JS Objects released on non-main threads
blocking-b2g: --- → 1.3?
Blocks: 926746
This appears to be a B2G (or at least E10S-only) issue given the release from nsTransportEventSinkProxy::Release()
Blocks a blocker.
blocking-b2g: 1.3? → koi?
Severity: normal → critical
Keywords: crash
Version: unspecified → 26 Branch
Blocking a blocker and hence koi+ on the same.
blocking-b2g: koi? → koi+
Caused by the landing of bug 866514, which added a JS object to the items released by the NotificationEvent object (the TracksAvailableCallback object).  This was landed in 23 and uplifted to 22.

Before bug 926746, the only things we'd seen that might have been this bug were occasional did-not-exit bugs.  That bug landing made this very frequent and caused it to be backed out.  Thanks to Steven for digging in to find the root cause.

Marking as a sec bug because I can't guarantee that all the failure paths are safe if this happens (which isn't easy without that new landing).  Because of this, we'll want to uplift it as far as we can.  There's no indication this is exploitable, so I would not at this moment recommend any form of spill for it; we might consider it riding along with any other releases is they occur (particularly for 26 and 24ESR).

Patch will be attached; it's simply making sure no reference is kept to the event even temporarily after NS_DispatchToMainThread().  I'm also going to run a Try with the bug 926746 patches.
Assignee: nobody → rjesup
Group: media-core-security
Status: NEW → ASSIGNED
Depends on: 866514
OS: Gonk (Firefox OS) → All
Hardware: ARM → All
Target Milestone: --- → mozilla28
Attachment #8342508 - Flags: review?(roc)
Attachment #8342508 - Flags: review?(roc)
simpler patch; avoids leaking
Attachment #8342508 - Attachment is obsolete: true
Comment on attachment 8342591 [details] [diff] [review]
Fix runnable pointer holding

Try build is running
Attachment #8342591 - Flags: review?(roc)
Comment on attachment 8342591 [details] [diff] [review]
Fix runnable pointer holding

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

::: dom/media/MediaManager.h
@@ +374,1 @@
>            NS_DispatchToMainThread(event, NS_DISPATCH_NORMAL);

So if NS_DispatchToMainThread fails, we just leak. I guess that's OK.
Attachment #8342591 - Flags: review?(roc) → review+
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #11)
> Comment on attachment 8342591 [details] [diff] [review]
> Fix runnable pointer holding
> 
> Review of attachment 8342591 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/MediaManager.h
> @@ +374,1 @@
> >            NS_DispatchToMainThread(event, NS_DISPATCH_NORMAL);
> 
> So if NS_DispatchToMainThread fails, we just leak. I guess that's OK.

Yeah, it has to be ok, since we can't free it on this thread anyways.  (And if it fails, all sorts of other things are in massive trouble).  Thanks
Comment on attachment 8342591 [details] [diff] [review]
Fix runnable pointer holding

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

Not easily (likely very hard) - it would be very hard to force the timing, and our only indication is it fails to exit when this is hit.

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

Pretty much they point out the wrong-thread release.  I could pull the comments which would make it considerably more obtuse, but knowing it was a sec bug likely someone could eyeball it and figure out the issue (and also from the refs in the open bug this was spun off from)

Which older supported branches are affected by this flaw?

All back to FF22.  This includes 24ESR. We'll want to uplift to Aurora, and if the opportunity presents itself I'd update it on 26, 25 and 24ESR.  Right now I wouldn't recommend a spill for this.

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

Caused by bug 886514's landing (in 23, uplifted to 22).

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

The patch will trivially apply to all branches.

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

Virtually no chances of regressions.
Attachment #8342591 - Flags: sec-approval?
This is with Steven's patches and without this fix: (ugly)
https://tbpl.mozilla.org/?tree=Try&rev=1065025135af
Blocks: 886514
No longer depends on: 866514
Calling this 'sec-moderate' because we don't currently know a way to trigger this reliably in an exploitable way, but higher than 'low' because we know this kind of bug can be exploited should someone clever figure out how to catch it.
Comment on attachment 8342591 [details] [diff] [review]
Fix runnable pointer holding

[Triage Comment]
approval for aurora and sec-approval
Attachment #8342591 - Flags: sec-approval?
Attachment #8342591 - Flags: sec-approval+
Attachment #8342591 - Flags: approval-mozilla-aurora+
Backed out the wallpaper patch from bug 926746, and landed this.  Will land on Aurora once we're cleanly on m-c
https://hg.mozilla.org/integration/b2g-inbound/rev/c4b843dfa959
https://hg.mozilla.org/integration/b2g-inbound/rev/d212ce6a9a18
https://hg.mozilla.org/mozilla-central/rev/d212ce6a9a18

I'll uplift this in the morning if nobody beats me to it.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [qa-]
> Is this wontfix for esr24?

No, I think this will be opportunistically landed if there are convenient respins of ESR24 or 26, but no spills for those (or rather at least considered if there are spins caused by other bugs).  Given ESR24's lifetime, that's fairly likely for ESR24; 26 is at least 50/50 given history.  (From above and discussion with dveditz).
Group: media-core-security → core-security
Randell: do we want to land this on ESR24 for the release that corresponds to Fx27 (24.3)?
Flags: needinfo?(rjesup)
Whiteboard: [qa-] → [qa-][adv-main27+]
Per my previous comments, if we're spinning ESR24 I think this is something we should probably take as a safety measure.
Flags: needinfo?(rjesup)
Comment on attachment 8342591 [details] [diff] [review]
Fix runnable pointer holding

Let's get this into the ESR going today then.
Attachment #8342591 - Flags: approval-mozilla-esr24+
Whiteboard: [qa-][adv-main27+] → [qa-][adv-main27+][adv-esr24.3+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.