Closed Bug 794426 Opened 7 years ago Closed 7 years ago

Simultaneous calls to nsBuiltinDecoderStateMachine::StopAudioThread() cause problems

Categories

(Core :: Audio/Video, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20
Tracking Status
firefox17 --- wontfix
firefox18 + fixed
firefox19 + fixed
firefox20 + fixed
firefox-esr10 --- unaffected
firefox-esr17 18+ fixed
b2g18 --- fixed

People

(Reporter: roc, Assigned: roc)

References

Details

(Keywords: crash, sec-critical, Whiteboard: [adv-main18+][adv-esr17+])

Attachments

(1 file, 1 obsolete file)

Our code assumes that after the state machine thread has called StopAudioThread(), the audio thread is stopped. There is a situation where this is not true:
-- SetAudioCaptured gets called on the main thread, calling StopAudioThread from the main thread
-- main-thread StopAudioThread calls Shutdown() on the audio thread, and waits for it to shut down
-- meanwhile nsBuiltinDecoderStateMachine calls StopAudioThread()
-- state machine thread StopAudioThread calls Shutdown() on the audio thread, and returns immediately with an error because a thread can only be shut down by a single other thread
-- state machine thread carries on and cleans up stuff the audio thread is using --> crash
Attached patch fix (obsolete) — Splinter Review
Attachment #664897 - Flags: review?(cpearce)
Comment on attachment 664897 [details] [diff] [review]
fix

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

I think we should avoid shutting down the audio thread from the main thread, as it will cause the JS context to block until the audio thread has shutdown, which may or may not be instantaneous; it'll introduce the possibility for jank.

How about in SetAudioCaptured() dispatching a runnable to the state machine thread to call StopAudioThread() instead? We can also have AudioLoop() check mAudioCaptured and exit if that's true to speed up the shutdown (similar to how we check for mState==SHUTDOWN).

::: content/media/nsBuiltinDecoderStateMachine.cpp
@@ +1517,5 @@
>    if (mDecodeThread) {
>      LOG(PR_LOG_DEBUG, ("%p Shutdown decode thread", mDecoder.get()));
>      {
>        ReentrantMonitorAutoExit exitMon(mDecoder->GetReentrantMonitor());
> +      nsresult rv = mDecodeThread->Shutdown();

Does the same problem exist with the decode thread? Or are you just trying to prevent the same kind of error happening with the decode thread?
Attachment #664897 - Flags: review?(cpearce)
You're right that we need to not wait for the audio thread to shut down from SetAudioCaptured on the main thread.

The problem is that if the audio thread is running, and we're trying to copy data from the audio queue into one or more MediaStreams, MediaDecoderStateMachine::SendStreamData and MediaDecoderStateMachine::AudioLoop both want to be in charge of popping data from the audio queue. So we can't allow that to happen. I think probably the solution is to not run MediaDecoderStateMachine::SendStreamData at all until the audio thread has shut down.
Attached patch fixSplinter Review
Attachment #664897 - Attachment is obsolete: true
Attachment #688200 - Flags: review?(cpearce)
Attachment #688200 - Flags: review?(cpearce) → review+
https://hg.mozilla.org/mozilla-central/rev/9f30fd422239
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Comment on attachment 688200 [details] [diff] [review]
fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): MediaStreams captured from media elements
User impact if declined: possible security issue
Testing completed: just laned on m-c
Risk to taking this patch (and alternatives if risky): fairly low risk since it should only affect a feature that is pretty much unused currently
String or UUID changes made by this patch: None
Attachment #688200 - Flags: approval-mozilla-beta?
Attachment #688200 - Flags: approval-mozilla-b2g18?
Attachment #688200 - Flags: approval-mozilla-aurora?
(In reply to Alex Keybl [:akeybl] from comment #8)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7)
> > Bug caused by (feature/regressing bug #): MediaStreams captured from media
> > elements
> 
> When did this land? Is it enabled by default?

Landed April 30 so it's been around for a while. Enabled by default.

> > User impact if declined: possible security issue
> > Testing completed: just laned on m-c
> > Risk to taking this patch (and alternatives if risky): fairly low risk since
> > it should only affect a feature that is pretty much unused currently
> > String or UUID changes made by this patch: None
> 
> If this is a critical security bug, we should probably restrict it to the
> core-security group.

OK done.
Group: core-security
Comment on attachment 688200 [details] [diff] [review]
fix

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

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

Which older supported branches are affected by this flaw?
esr17 and release

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should be pretty easy to backport wherever needed.

How likely is this patch to cause regressions; how much testing does it need?
Not likely to cause noticeable regressions because it only affects a feature that's practically unused.
Attachment #688200 - Flags: sec-approval?
Comment on attachment 688200 [details] [diff] [review]
fix

sec-approval+. We should figure out if we want to take this on Aurora, Beta, and ESR.
Attachment #688200 - Flags: sec-approval? → sec-approval+
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10)
> [Security approval request comment]

The idea of sec-approval? is to ask /before/ landing on m-c for potentially serious security bugs, so we don't force ourselves into landing stuff in the last beta or two before release that will end up going out without sufficient regression testing.
Attachment #688200 - Flags: approval-mozilla-beta?
Attachment #688200 - Flags: approval-mozilla-beta+
Attachment #688200 - Flags: approval-mozilla-b2g18?
Attachment #688200 - Flags: approval-mozilla-b2g18+
Attachment #688200 - Flags: approval-mozilla-aurora?
Attachment #688200 - Flags: approval-mozilla-aurora+
Whiteboard: [adv-main18+][adv-esr17+]
Blocks: 813435
Depends on: 826104
Depends on: 834667
Group: core-security
You need to log in before you can comment on or make changes to this bug.