Closed Bug 1416799 Opened 2 years ago Closed 2 years ago
Crash in mozilla::Media
Decoder State Machine::Accurate Seeking State::On Seek Rejected
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 =============================================================
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
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.
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?
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.
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.
sec-approval+ for trunk checking on November 28. We'll want a beta patch nominated at that time as well.
Attachment #8928096 - Flags: sec-approval? → sec-approval+
Whiteboard: [checkin on 11/28]
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?
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+] → [adv-main58+][post-critsmash-triage]
You need to log in before you can comment on or make changes to this bug.