1406772 - Crash in mozilla::AudioChannelsDownMix<T>

Status: VERIFIED FIXED

(Core :: Audio/Video: cubeb, defect, P1)

Description

[[:philipp]]

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.

Comment 1

[Paul Adenot (:padenot)]

Probably a dupe of the channel count change thing, right ?

Comment 2

[Alex Chronopoulos [: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.

Comment 3

[Mike Taylor [:miketaylr]]

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

Comment 4

[Alex Chronopoulos [:achronop]]

Crash still reported. We need to take a closer look on that.

Comment 5

[Mike Taylor [:miketaylr]]

Only 2 reports this week. Let's keep an eye on it as it goes to Beta.

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

(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

Comment 7

[Alex Chronopoulos [:achronop]]

I am discussing it with Paul and we do not have clear opinion why it is happening. Still working on it.

Comment 8

[Mike Taylor [:miketaylr]]

Crash volume has gone up a bit.

Comment 9

[Alex Chronopoulos [:achronop]]

Yeah, after merge day, beta got the problematic part and started contributing on the overall results.

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

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....

Comment 11

[Alex Chronopoulos [:achronop]]

Thank you for noticing early enough. I am back on that, I would appreciate any support.

Comment 12

[Alex Chronopoulos [:achronop]]

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.

Comment 13

[Alex Chronopoulos [:achronop]]

Attachment: Get channel count from MSG which is constantly available.

Comment 14

[Alex Chronopoulos [:achronop]]

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.

Comment 15

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

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.

Comment 16

[Alex Chronopoulos [:achronop]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/07f0607be42d8e7524d43786ff802fdedfc3e23d
Bug 1406772 - Get channel count from MSG. p=padenot

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/07f0607be42d

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta uplift when you get a chance.

Comment 19

[Alex Chronopoulos [:achronop]]

Attachment: Uplift to beta

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

Comment 20

[Gerry Chang [:gchang]]

Comment on attachment 8934167 [details] [diff] [review]
Uplift to beta

Fix a sec-high. Beta58+.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ff143543997b

Comment 22

[Emil Ghitta, QA [:emilghitta]]

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.