Closed Bug 1406772 Opened 2 years ago Closed 2 years ago

Crash in mozilla::AudioChannelsDownMix<T>

Categories

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

56 Branch
x86_64
Windows
defect

Tracking

()

VERIFIED FIXED
mozilla59
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- unaffected
firefox57 --- unaffected
firefox58 + verified
firefox59 + verified

People

(Reporter: philipp, Assigned: achronop)

References

Details

(4 keywords)

Crash Data

Attachments

(2 files)

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.
Flags: needinfo?(padenot)
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.
Flags: needinfo?(achronop)
Rank: 15
Priority: -- → P2
(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?
Flags: needinfo?(achronop)
Crash still reported. We need to take a closer look on that.
Flags: needinfo?(achronop)
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
Group: media-core-security
Flags: needinfo?(achronop)
I am discussing it with Paul and we do not have clear opinion why it is happening. Still working on it.
Flags: needinfo?(achronop)
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
Flags: needinfo?(achronop)
Priority: P2 → P1
Thank you for noticing early enough. I am back on that, I would appreciate any support.
Flags: needinfo?(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.
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/mozilla-central/rev/07f0607be42d
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Please nominate this for Beta uplift when you get a chance.
Flags: needinfo?(achronop)
Depends on: 1422631
Attached patch Uplift to betaSplinter Review
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
Flags: needinfo?(achronop)
Attachment #8934167 - Flags: review?(padenot)
Attachment #8934167 - Flags: approval-mozilla-beta?
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+
Group: media-core-security → core-security-release
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.