Closed Bug 1604117 Opened 3 years ago Closed 3 years ago

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

Categories

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

73 Branch
Unspecified
Windows 7
defect

Tracking

()

RESOLVED FIXED
mozilla74
Tracking Status
firefox-esr68 --- unaffected
firefox71 --- unaffected
firefox72 --- unaffected
firefox73 + fixed
firefox74 + fixed

People

(Reporter: calixte, Assigned: achronop)

References

(Blocks 1 open bug, Regression)

Details

(4 keywords, Whiteboard: [post-critsmash-triage])

Crash Data

Attachments

(2 files)

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

Flags: needinfo?(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.

Flags: needinfo?(achronop) → needinfo?(padenot)

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

Group: media-core-security

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.

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

Flags: needinfo?(padenot) → needinfo?(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.

Flags: needinfo?(achronop)

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

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

For more information, please visit auto_nag documentation.

Flags: needinfo?(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.

Assignee: nobody → achronop
Flags: needinfo?(padenot) → needinfo?(achronop)
Priority: -- → P3

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

Flags: needinfo?(achronop)
Crash Signature: [@ memcpy_repmovs | mozilla::AudioPacketizer<T>::Input] → [@ memcpy_repmovs | mozilla::AudioPacketizer<T>::Input] [@ memcpy | mozilla::AudioPacketizer<T>::Input] [@ vcruntime140.dll | mozilla::AudioPacketizer<T>::Input]

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]

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.

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

Attachment #9120417 - Attachment description: Bug 1604117 - Use internal buffer on passthrough resampler. r?padenot → Bug 1604117 - Backout cubeb commit aa63601. r?padenot

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

Priority: P3 → P1

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.
Attachment #9120417 - Flags: sec-approval?

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

Approved to land and request uplift.

Attachment #9120417 - Flags: sec-approval? → sec-approval+

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

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

Flags: needinfo?(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:
Flags: needinfo?(achronop)
Attachment #9120417 - Flags: approval-mozilla-beta?
Group: media-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla74
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]

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.

Attachment #9120417 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

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.

Flags: needinfo?(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.

Flags: needinfo?(achronop)

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.

Flags: needinfo?(achronop)

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

Flags: needinfo?(achronop)
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main73+]

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.

Whiteboard: [post-critsmash-triage][adv-main73+] → [post-critsmash-triage]
Group: core-security-release
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.