Crash in [@ memcpy_repmovs | mozilla::AudioPacketizer<T>::Input]
Categories
(Core :: Audio/Video: cubeb, defect, P1)
Tracking
()
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)
492.86 KB,
image/png
|
Details | |
47 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
tjr
:
sec-approval+
|
Details | Review |
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
Assignee | ||
Comment 1•6 years ago
|
||
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•6 years ago
|
||
Crashes are all real-memory non-nullptr reads, marking as a sec issue
Comment 3•6 years ago
|
||
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•6 years ago
|
||
We're not handling the case where there is less input data than there is output data it seems.
Assignee | ||
Comment 5•6 years ago
•
|
||
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•6 years ago
|
||
Given bp-78dc76c9-3bbd-4642-a105-991bf0191209 on 72 I'm not convinced the regression range from comment 0 is correct.
Comment 7•6 years ago
|
||
The priority flag is not set for this bug.
:padenot, could you have a look please?
For more information, please visit auto_nag documentation.
Updated•6 years ago
|
Comment 8•6 years ago
|
||
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 | ||
Comment 9•6 years ago
|
||
This is a crash from yesterday with better crash-stack: https://crash-stats.mozilla.org/report/index/8aacd980-ff7e-4b79-a6b4-f02ec0200107
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 10•6 years ago
•
|
||
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]
Assignee | ||
Comment 11•5 years ago
|
||
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.
Assignee | ||
Comment 12•5 years ago
|
||
This is a screenshot from the debugger as a reference to the comment above.
Assignee | ||
Comment 14•5 years ago
|
||
Updated•5 years ago
|
Updated•5 years ago
|
Comment 15•5 years ago
|
||
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
Assignee | ||
Comment 16•5 years ago
|
||
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•5 years ago
|
||
Comment on attachment 9120417 [details]
Bug 1604117 - Backout cubeb commit aa63601. r?padenot
Approved to land and request uplift.
Comment 18•5 years ago
|
||
https://hg.mozilla.org/integration/autoland/rev/f980191897a2
Please nominate this for Beta approval when you get a chance. It grafts cleanly as-landed.
Assignee | ||
Comment 19•5 years ago
|
||
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•5 years ago
|
||
Updated•5 years ago
|
Comment 21•5 years ago
|
||
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•5 years ago
|
||
uplift |
Comment 23•5 years ago
|
||
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.
Assignee | ||
Comment 24•5 years ago
|
||
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•5 years ago
|
||
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.
Assignee | ||
Comment 26•5 years ago
|
||
Yeah, I am not aware of any problem since Bug 1610527 has landed. Thank you very much.
Updated•5 years ago
|
Comment 27•5 years ago
|
||
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.
Updated•5 years ago
|
Updated•5 years ago
|
Description
•