Closed Bug 1416799 Opened 2 years ago Closed 2 years ago

Crash in mozilla::MediaDecoderStateMachine::AccurateSeekingState::OnSeekRejected

Categories

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

54 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- wontfix
firefox58 + fixed
firefox59 + fixed

People

(Reporter: marcia, Assigned: jwwang)

References

Details

(4 keywords, Whiteboard: [adv-main58+][post-critsmash-triage])

Crash Data

Attachments

(1 file)

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

=============================================================
Flags: needinfo?(cpearce)
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.
Flags: needinfo?(cpearce) → needinfo?(jwwang)
(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).
Priority: -- → P2
Checking.
Flags: needinfo?(jwwang)
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.
Assignee: nobody → jwwang
Attachment #8928096 - Flags: review?(cpearce)
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.
OS: Windows 7 → All
Hardware: x86 → All
Comment on attachment 8928096 [details] [diff] [review]
1416799_fix_crash.patch

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

Thanks!
Attachment #8928096 - Flags: review?(cpearce) → review+
Thanks for the review!

Regressed by bug 1359058 which affected 54 and the later versions.
Attachment #8928096 - Flags: sec-approval?
Version: Trunk → 54 Branch
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.
Flags: needinfo?(jwwang)
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.
Flags: needinfo?(jwwang)
sec-approval+ for trunk checking on November 28. We'll want a beta patch nominated at that time as well.
Whiteboard: [checkin on 11/28]
Attachment #8928096 - Flags: sec-approval? → sec-approval+
Duplicate of this bug: 1419024
Blocks: 1415478
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
Attachment #8928096 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/9ae3f1f6994b
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Group: media-core-security → core-security-release
Comment on attachment 8928096 [details] [diff] [review]
1416799_fix_crash.patch

Fix a sec-high. Beta58+.
Attachment #8928096 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Whiteboard: [adv-main58+]
Flags: qe-verify-
Whiteboard: [adv-main58+] → [adv-main58+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.