Closed Bug 1406772 Opened 4 years ago Closed 4 years ago
Crash in mozilla::Audio
Channels Down Mix<T>
This bug was filed from the Socorro interface and is report bp-ba2e1ec5-6662-4f05-8951-1377c0171003. ============================================================= Crashing Thread (50) Frame Module Signature Source 0 xul.dll mozilla::AudioChannelsDownMix<short>(nsTArray<short const*> const&, short**, unsigned int, unsigned int) dom/media/AudioChannelFormat.h:131 1 xul.dll mozilla::DownmixAndInterleave<short, float>(nsTArray<short const*> const&, int, float, unsigned int, float*) dom/media/AudioSegment.h:133 2 xul.dll mozilla::WriteChunk<short>(mozilla::AudioChunk&, unsigned int, float*) dom/media/AudioSegment.h:435 3 xul.dll mozilla::AudioSegment::WriteTo(unsigned __int64, mozilla::AudioMixer&, unsigned int, unsigned int) dom/media/AudioSegment.cpp:187 4 xul.dll mozilla::MediaStreamGraphImpl::PlayAudio(mozilla::MediaStream*) dom/media/MediaStreamGraph.cpp:950 5 xul.dll mozilla::MediaStreamGraphImpl::Process() dom/media/MediaStreamGraph.cpp:1396 6 xul.dll mozilla::MediaStreamGraphImpl::OneIteration(__int64) dom/media/MediaStreamGraph.cpp:1458 7 xul.dll mozilla::AudioCallbackDriver::DataCallback(float const*, float*, long) dom/media/GraphDriver.cpp:1031 8 xul.dll cubeb_resampler_speex<float, cubeb_resampler_speex_one_way<float>, delay_line<float> >::fill_internal_duplex(float*, long*, float*, long) media/libcubeb/src/cubeb_resampler.cpp:234 9 xul.dll `anonymous namespace'::refill media/libcubeb/src/cubeb_wasapi.cpp:564 10 xul.dll `anonymous namespace'::refill_callback_duplex media/libcubeb/src/cubeb_wasapi.cpp:763 11 xul.dll `anonymous namespace'::wasapi_stream_render_loop media/libcubeb/src/cubeb_wasapi.cpp:970 12 ucrtbase.dll o__strtoui64 13 kernel32.dll BaseThreadInitThunk 14 ntdll.dll RtlUserThreadStart this crash signature in the content process is getting more common on 58.0a1, starting with build 20170930100302.
Probably a dupe of the channel count change thing, right ?
Flags: needinfo?(padenot) → needinfo?(achronop)
Yeah it could be. In this case it must be solved by Bug 1400889. Latest build id is the following: https://crash-stats.mozilla.com/report/index/092b079b-f072-4b24-b8f1-02c040171008 I will keep an eye this week to verify that it's solved.
(In reply to Alex Chronopoulos [:achronop] from comment #2) > Yeah it could be. In this case it must be solved by Bug 1400889. Latest > build id is the following: > https://crash-stats.mozilla.com/report/index/092b079b-f072-4b24-b8f1- > 02c040171008 > I will keep an eye this week to verify that it's solved. Any updates Alex?
Crash still reported. We need to take a closer look on that.
Only 2 reports this week. Let's keep an eye on it as it goes to Beta.
(In reply to Alex Chronopoulos [:achronop] from comment #4) > Crash still reported. We need to take a closer look on that. Alex - any updates? This is READ/WRITEing to random real-address non-nullptrs -- UAF or wildptr. sec-high
I am discussing it with Paul and we do not have clear opinion why it is happening. Still working on it.
Crash volume has gone up a bit.
Yeah, after merge day, beta got the problematic part and started contributing on the overall results.
We need a fix, and we need to land it and uplift before we lock 58 down for release Assigning Alex in order to kickstart this....
Assignee: nobody → achronop
Rank: 15 → 5
Priority: P2 → P1
Thank you for noticing early enough. I am back on that, I would appreciate any support.
I have a theory and I could bet money on it. Let's assume we have a gUM request to an 8-channel input and 2-channel output. That will lead to the downmix code path that the crash occurs. Now, when we switch driver we take the following steps, drain the current callback, set next driver as the current driver. At that point and till we call the AudioCallbackDriver::Init() the mOutputChannels in the newly created driver is initialized to zero. But, draining in wasapi does not stop the callback immediately but allows one more iteration. When this extra iteration reach the point bellow before the Init() of the newly driver: https://searchfox.org/mozilla-central/source/dom/media/MediaStreamGraph.cpp#965 the output channels that we get is zero and that leads to the crash. The solution is to change the call of the WriteTo() method and get the output channels from MediaStreamGraphImpl::mOutputChannels which is a const and is set at the constructor of MediaStreamGraphImpl. The second is to modify the wasapi code and return at the beginning of refill_callback_duplex callback when stm->draining is true (like we do in refill_callback_output). Any of the above will clear the crash but better to do both.
Attachment #8933721 - Flags: review?(padenot) → review+
Comment on attachment 8933721 [details] [diff] [review] Get channel count from MSG which is constantly available. Security approval request comment] How easily could an exploit be constructed based on the patch? It's not easy but it's not impossible. 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? 58 (beta) If not all supported branches, which bug introduced the flaw? Bug 1378070 Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? No, I do not see an easy way to create one. How likely is this patch to cause regressions; how much testing does it need? No, it's a small patch that restores the old way of doing things.
Attachment #8933721 - Flags: sec-approval?
Comment on attachment 8933721 [details] [diff] [review] Get channel count from MSG which is constantly available. sec-approval+ for trunk. Please nominate for Beta as well.
Attachment #8933721 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/integration/mozilla-inbound/rev/07f0607be42d8e7524d43786ff802fdedfc3e23d Bug 1406772 - Get channel count from MSG. p=padenot
Please nominate this for Beta uplift when you get a chance.
Approval Request Comment [Feature/Bug causing the regression]: Bug 1378070 [User impact if declined]: Crash when downmix audio on Windows [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]: 1. Execute gUM request from a multichannel microphone 2. Output the stream to a stereo device 3. Request an channelCount constrain change to something greater than 2 [List of other uplifts needed for the feature/fix]: N/A [Is the change risky?]: No [Why is the change risky/not risky?]: It's a small patch that restores the old way of doing things. [String changes made/needed]: N/A
Attachment #8934167 - Flags: review?(padenot) → review+
Comment on attachment 8934167 [details] [diff] [review] Uplift to beta Fix a sec-high. Beta58+.
Attachment #8934167 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Unfortunately we don't have the necessary equipment to verify if this issue was fixed or not. I think that we can mark this issue as verified fixed based on the crash stats (The issue was last seen on Firefox 58.0b9). We will keep monitoring the crash stats and if something comes up the issue can be reopened.
You need to log in before you can comment on or make changes to this bug.