1604117 - Crash in [@ memcpy_repmovs | mozilla::AudioPacketizer<T>::Input]

Status: RESOLVED FIXED

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

Description

[Calixte Denizet (:calixte)]

This bug is for crash report bp-3a7dcb48-7d06-412a-8dc6-130c20191214.

Top 10 frames of crashing thread:

0 vcruntime140.dll memcpy_repmovs f:\dd\vctools\crt\vcruntime\src\string\amd64\memcpy.asm:115
1 xul.dll mozilla::AudioPacketizer<float, float>::Input dom/media/AudioPacketizer.h
2 xul.dll mozilla::AudioInputProcessing::PacketizeAndProcess dom/media/webrtc/MediaEngineWebRTCAudio.cpp:898
3 xul.dll mozilla::AudioInputProcessing::NotifyInputData dom/media/webrtc/MediaEngineWebRTCAudio.cpp:1051
4 xul.dll mozilla::MediaTrackGraphImpl::NotifyInputData dom/media/MediaTrackGraph.cpp:853
5 xul.dll mozilla::AudioCallbackDriver::DataCallback dom/media/GraphDriver.cpp:876
6 xul.dll passthrough_resampler<float>::fill media/libcubeb/src/cubeb_resampler.cpp:97
7 xul.dll `anonymous namespace'::refill media/libcubeb/src/cubeb_wasapi.cpp:789
8 xul.dll `anonymous namespace'::refill_callback_duplex media/libcubeb/src/cubeb_wasapi.cpp
9 xul.dll `anonymous namespace'::wasapi_stream_render_loop media/libcubeb/src/cubeb_wasapi.cpp

There are 2 crashes (from 1 installation) in nightly 73 with buildid 20191214094139. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1603384.

[1] https://hg.mozilla.org/mozilla-central/rev?node=9794b0a3bffc

Comment 1

[Alex Chronopoulos [:achronop]]

With cubeb#560 we forward the incoming buffer when possible instead of coping to the internal_input_buffer. Do you see a problem with that? This crash is happening around that code and regressed since this optimization has added.

Comment 2

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

Crashes are all real-memory non-nullptr reads, marking as a sec issue

Comment 3

[Paul Adenot (:padenot)]

This sounds like a good idea, but there must be a problem somewhere. This is erroring out when reading data, so the pointer could point to something not long enough maybe?

That said, only 4 crash, all on Windows 7. This might have just changed an existing signature.

Comment 4

[Paul Adenot (:padenot)]

We're not handling the case where there is less input data than there is output data it seems.

Comment 5

[Alex Chronopoulos [:achronop]]

I thought this cannot happen because we are in the duplex callback and the amount of input data can be equal or more of the output data.

Comment 6

[Julien Cristau [:jcristau]]

Given bp-78dc76c9-3bbd-4642-a105-991bf0191209 on 72 I'm not convinced the regression range from comment 0 is correct.

Comment 7

[Release mgmt bot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:padenot, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 8

[Paul Adenot (:padenot)]

Alex, can you have a second look at this? It might well be an issue in the Windows backend surfaced by the optimization performed in https://github.com/kinetiknz/cubeb/pull/560.

Comment 9

[Alex Chronopoulos [:achronop]]

This is a crash from yesterday with better crash-stack: https://crash-stats.mozilla.org/report/index/8aacd980-ff7e-4b79-a6b4-f02ec0200107

Comment 10

[Alex Chronopoulos [:achronop]]

Stack from the dump file, from the crash in the comment 9, on VisualStudio debugger:

 	VCRUNTIME140.dll!memcpy(unsigned char * dst, unsigned char * src, unsigned long count) Line 319	Unknown
 	mozglue.dll!mozilla::detail::MutexImpl::unlock() Line 33	C++
>	xul.dll!mozilla::AudioInputProcessing::PacketizeAndProcess(mozilla::MediaTrackGraphImpl * aGraph, const float * aBuffer, unsigned int aFrames, int aRate, unsigned int aChannels) Line 898	C++
 	xul.dll!mozilla::AudioInputProcessing::NotifyInputData(mozilla::MediaTrackGraphImpl * aGraph, const float * aBuffer, unsigned int aFrames, int aRate, unsigned int aChannels) Line 1061	C++
 	xul.dll!mozilla::MediaTrackGraphImpl::NotifyInputData(const float * aBuffer, unsigned int aFrames, int aRate, unsigned int aChannels) Line 858	C++
 	xul.dll!mozilla::AudioCallbackDriver::DataCallback(const float * aInputBuffer, float * aOutputBuffer, long aFrames) Line 921	C++
 	xul.dll!mozilla::AudioCallbackDriver::DataCallback_s(cubeb_stream * aStream, void * aUser, const void * aInputBuffer, void * aOutputBuffer, long aFrames) Line 804	C++
 	xul.dll!passthrough_resampler<float>::fill(void * input_buffer, long * input_frames_count, void * output_buffer, long output_frames) Line 97	C++
 	[Inline Frame] xul.dll!cubeb_resampler_fill(cubeb_resampler * resampler, void * input_buffer, long * input_frames_count, void * output_buffer, long output_frames_needed) Line 358	C++
 	xul.dll!`anonymous namespace'::refill(cubeb_stream * stm, void * input_buffer, long input_frames_count, void * output_buffer, long output_frames_needed) Line 795	C++
 	xul.dll!`anonymous namespace'::refill_callback_duplex(cubeb_stream * stm) Line 0	C++
 	xul.dll!`anonymous namespace'::wasapi_stream_render_loop(void * stream) Line 1271	C++
 	[External Code]

Comment 11

[Alex Chronopoulos [:achronop]]

By examining the local variables in the last 3 frames a possible problem came up. The number of input frames are less than the number of output frames:

input_frames_count = 441
output_frames_needed = 882

This can only be a problem if the incoming buffer is forwarded to the output which is the optimization performed in bug 1603384. The core dump cannot show which branch of the optimization has been followed to verify the case. However, this is the only case that a read violation exception can be produced. When the internal_input_buffer is being used to pass the audio data through, the crash is being prevented by the fact that the capacity of that buffer is already allocated so even if the length is short, reading beyond that length does not crash, it glitches of course.

I will create a patch to avoid the optimization and push additional silence when the input buffer is short. When the fix is verified on the wild and if the glitching is common, that it should not, we can improve that behavior by resamling the existing data instead of pushing silence.

Comment 12

[Alex Chronopoulos [:achronop]]

Attachment: Screenshot from the debugger

This is a screenshot from the debugger as a reference to the comment above.

Comment 14

[Alex Chronopoulos [:achronop]]

Attachment: Bug 1604117 - Backout cubeb commit aa63601. r?padenot

Comment 15

[Release mgmt bot [:suhaib / :marco/ :calixte]]

Changing the priority to p1 as the bug is tracked by a release manager for the current beta.
See What Do You Triage for more information

Comment 16

[Alex Chronopoulos [:achronop]]

Comment on attachment 9120417 [details]
Bug 1604117 - Backout cubeb commit aa63601. r?padenot

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Not easy, the problem reproduces sporadically on random memory place. The path reveals the cause but an exploit cannot be constructed easily. Also, this is a backout of a previous patch. The only new information is that the patch was problematic.
• 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?: beta
• If not all supported branches, which bug introduced the flaw?: Bug 1603384
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: It's the same patch. The same patch can be applied in beta. It would be good to have a beta uplift soon.
• How likely is this patch to cause regressions; how much testing does it need?: Not at all. The patch removes an optimization that took place a few weeks ago. The former version had been working well for a long time.

Comment 17

[Tom Ritter [:tjr]]

Comment on attachment 9120417 [details]
Bug 1604117 - Backout cubeb commit aa63601. r?padenot

Approved to land and request uplift.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/autoland/rev/f980191897a2

Please nominate this for Beta approval when you get a chance. It grafts cleanly as-landed.

Comment 19

[Alex Chronopoulos [:achronop]]

Comment on attachment 9120417 [details]
Bug 1604117 - Backout cubeb commit aa63601. r?padenot

Beta/Release Uplift Approval Request

• User impact if declined: Several sec crashes in windows.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce: Not sure, it happens in the wild probably under specific circumstances, for example, old loaded machine, some devices, etc.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): It backs out an optimization we made on the cubeb driver. The previous code had been working for a long time without the issue.
• String changes made/needed:

Comment 20

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

https://hg.mozilla.org/mozilla-central/rev/f980191897a2ceb0514f83df997dc05a349f54db

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9120417 [details]
Bug 1604117 - Backout cubeb commit aa63601. r?padenot

Fixes a topcrash by disabling a new optimization. Approved for 73.0b6.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

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

Comment 23

[Tom Ritter [:tjr]]

It looks like this was a vulnerability introduced in https://github.com/kinetiknz/cubeb/commit/aa636017aca8f79ebc95fb9f03b9333eaf9fd8fc and fixed in https://github.com/kinetiknz/cubeb/commit/6cdf2fd22b7211bdfe5d7bc4a44430af36275151 - is that correct? If so we'll want to assign a separate CVE to it.

Comment 24

[Alex Chronopoulos [:achronop]]

Yes but ... the bug initially solved (comment 22) by reverting the https://github.com/kinetiknz/cubeb/commit/aa636017aca8f79ebc95fb9f03b9333eaf9fd8fc from the gecko. Then I started working on a proper solution, which is the https://github.com/kinetiknz/cubeb/commit/6cdf2fd22b7211bdfe5d7bc4a44430af36275151, and it has just landed with Bug 1610527. The later has not baked enough in Nightly in order to be 100% sure that it is the right solution. If there are no new occurrences of this crash in the next few days then what you said in comment 23 is correct.

Comment 25

[Tom Ritter [:tjr]]

It looks like Bug 1610527 has baked a bit and stuck, right? If so I will create an advisory for this and assign a CVE.

Comment 26

[Alex Chronopoulos [:achronop]]

Yeah, I am not aware of any problem since Bug 1610527 has landed. Thank you very much.

Comment 27

[Tom Ritter [:tjr]]

Bug 1603384 was where the issue was introduced; but that was in 73; not 72. The crashes observed in 72 must have been something else.