1416799 - Crash in mozilla::MediaDecoderStateMachine::AccurateSeekingState::OnSeekRejected

Status: RESOLVED FIXED

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

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is
report bp-66c27fc2-d689-4735-a148-932160171112.
=============================================================

Seen while looking at crash stats: http://bit.ly/2jnYUS8. Crashes date back to at least 20171107122447. Marking as sec sensitive since there are several possible UAF addresses among the crashes. 26 crashes/26 installs so far.

Possible regression range based on crash stats: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=179dae92e4d794e7f45ad080ff01908c80691f31&tochange=40df5dd35fdb7ce3652fe4448ac8961c075c928e

Bug 1414680 landed in that time frame. ni on :cpearce

Top 10 frames of crashing thread:

0 xul.dll mozilla::MediaDecoderStateMachine::AccurateSeekingState::OnSeekRejected dom/media/MediaDecoderStateMachine.cpp:1167
1 xul.dll mozilla::MozPromise<mozilla::media::TimeUnit, mozilla::SeekRejectValue, 1>::InvokeCallbackMethod<0, <lambda_22680133a5d5989365f728d14cadbff9>, void const, mozilla::SeekRejectValue, RefPtr<mozilla::MozPromise<mozilla::media::TimeUnit, mozilla::SeekRejectValue, 1>::Private> > xpcom/threads/MozPromise.h:565
2 xul.dll mozilla::MozPromise<mozilla::media::TimeUnit, mozilla::SeekRejectValue, 1>::ThenValue<<lambda_9d45ba55067d58ca3a399286c26ce766>, <lambda_22680133a5d5989365f728d14cadbff9> >::DoResolveOrRejectInternal xpcom/threads/MozPromise.h:771
3 xul.dll mozilla::MozPromise<nsTString<char>, nsresult, 1>::ThenValueBase::DoResolveOrReject xpcom/threads/MozPromise.h:497
4 xul.dll mozilla::MozPromise<mozilla::media::TimeUnit, mozilla::SeekRejectValue, 1>::ThenValueBase::ResolveOrRejectRunnable::Run xpcom/threads/MozPromise.h:402
5 xul.dll mozilla::AutoTaskDispatcher::TaskGroupRunnable::Run xpcom/threads/TaskDispatcher.h:209
6 xul.dll mozilla::TaskQueue::Runner::Run xpcom/threads/TaskQueue.cpp:246
7 xul.dll nsThreadPool::Run xpcom/threads/nsThreadPool.cpp:228
8 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1037
9 xul.dll NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:513

=============================================================

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

I can repro (on MacOS in Nightly) on this URL:
ftp://pra13-1-82-66-186-145.fbx.proxad.net/Public/Shared%20Music/Ennio%20Morricone/The%20Soundtracks/1-01%20Titoli.m4a

I don't think my patches in Bug 1414680 would cause this.

JW: Do you want to look into this? It's in the MDSM seeking code.

Comment 2

[Blake Wu [:bwu][:blakewu]]

(In reply to Chris Pearce (:cpearce) from comment #1)
> I can repro (on MacOS in Nightly) on this URL:
> ftp://pra13-1-82-66-186-145.fbx.proxad.net/Public/Shared%20Music/
> Ennio%20Morricone/The%20Soundtracks/1-01%20Titoli.m4a
I also can repro this bug on my windows 10 laptop with the latest nightly, 59.0a1 (2017-11-13) (64 bit).

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

Checking.

Comment 4

[JW Wang [:jwwang] [:jw_wang]]

Attachment: 1416799_fix_crash.patch

https://searchfox.org/mozilla-central/rev/bab833ebeef6b2202e71f81d225b968283521fd6/dom/media/MediaDecoderStateMachine.cpp#1166-1167

HandleEndOfAudio() calls MaybeFinishSeek() which might transition to another state. If transition does happen, the following HandleEndOfVideo() call will operate on an invalid |this| pointer.

Comment 5

[Marcia Knous [:marcia]]

Adding 59 as affected. 11 crashes there so far, and 30 when 58 was in nightly. This affects all 3 platforms, so changing platform to all.

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8928096 [details] [diff] [review]
1416799_fix_crash.patch

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

Thanks!

Comment 7

[JW Wang [:jwwang] [:jw_wang]]

Thanks for the review!

Regressed by bug 1359058 which affected 54 and the later versions.

Comment 8

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

JW, you set sec-approval? but didn't answer any of the sec-approval template questions that come up when you set the flag. We need those answered before you can get approval.

Comment 9

[JW Wang [:jwwang] [:jw_wang]]

Somehow the template is missing...

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Unlikely. The patch looks like extracting some common code to a helper function.

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? 57

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

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

How likely is this patch to cause regressions; how much testing does it need? Unlikely. The change is as simple as extracting some common code to helper functions.

Comment 10

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

sec-approval+ for trunk checking on November 28. We'll want a beta patch nominated at that time as well.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9ae3f1f6994bc9b076b34f8dcb2281aac51dbfb3

Comment 13

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8928096 [details] [diff] [review]
1416799_fix_crash.patch

Approval Request Comment
[Feature/Bug causing the regression]:1359058
[User impact if declined]:crash
[Is this code covered by automated tests?]:no
[Has the fix been verified in Nightly?]:yes
[Needs manual test from QE? If yes, steps to reproduce]: comment 1
[List of other uplifts needed for the feature/fix]:none
[Is the change risky?]:no
[Why is the change risky/not risky?]:The change is as simple as extracting common code to a helper function.
[String changes made/needed]:none

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/9ae3f1f6994b

Comment 15

[Gerry Chang [:gchang]]

Comment on attachment 8928096 [details] [diff] [review]
1416799_fix_crash.patch

Fix a sec-high. Beta58+.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5f8bf703e8cd