1605894 - heap-buffer-overflow in [@ mozilla::AudioSegment::Mix]

Status: RESOLVED FIXED

(Core :: Audio/Video, defect, P3)

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Reduced with m-c:
BuildID=20191224212327
SourceStamp=48159e53bfb85d9b22e94ecaaa6590ab4abd9545

==106930==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x626000080c10 at pc 0x56311a97218a bp 0x7fdea2e034b0 sp 0x7fdea2e02c78
READ of size 22016 at 0x626000080c10 thread T46 (GraphRunner)
    #0 0x56311a972189 in __asan_memcpy /builds/worker/fetches/llvm-project/llvm/projects/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cc:22:3
    #1 0x7fdec13b9475 in PodCopy<float> src/obj-firefox/dist/include/mozilla/PodOperations.h:99:5
    #2 0x7fdec13b9475 in mozilla::AudioSegment::Mix(mozilla::AudioMixer&, unsigned int, unsigned int) src/dom/media/AudioSegment.cpp:147:11
    #3 0x7fdec13806bc in mozilla::AudioCaptureTrack::ProcessInput(long, long, unsigned int) src/dom/media/AudioCaptureTrack.cpp:97:13
    #4 0x7fdec186e999 in mozilla::MediaTrackGraphImpl::Process(mozilla::AudioMixer*) src/dom/media/MediaTrackGraph.cpp:1248:15
    #5 0x7fdec18700ed in mozilla::MediaTrackGraphImpl::OneIterationImpl(long, long, mozilla::AudioMixer*) src/dom/media/MediaTrackGraph.cpp:1367:3
    #6 0x7fdec14b9585 in mozilla::GraphRunner::Run() src/dom/media/GraphRunner.cpp:111:32
    #7 0x7fdebaf540f7 in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1241:14
    #8 0x7fdebaf5e8fc in NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:486:10
    #9 0x7fdebc184922 in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:302:20
    #10 0x7fdebc07c357 in RunInternal src/ipc/chromium/src/base/message_loop.cc:315:10
    #11 0x7fdebc07c357 in RunHandler src/ipc/chromium/src/base/message_loop.cc:308:3
    #12 0x7fdebc07c357 in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:290:3
    #13 0x7fdebaf4d21a in nsThread::ThreadFunc(void*) src/xpcom/threads/nsThread.cpp:459:11
    #14 0x7fdedefd825e in _pt_root src/nsprpub/pr/src/pthreads/ptthread.c:201:5
    #15 0x7fdedec256b9 in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x76b9)
    #16 0x7fdeddc4b41c in clone /build/glibc-LK5gWL/glibc-2.23/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:109

0x626000080c10 is located 0 bytes to the right of 11024-byte region [0x62600007e100,0x626000080c10)
allocated by thread T46 (GraphRunner) here:
    #0 0x56311a972d4d in malloc /builds/worker/fetches/llvm-project/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:145:3
    #1 0x56311a9a853d in moz_xmalloc src/memory/mozalloc/mozalloc.cpp:52:15
    #2 0x7fdebd5dc526 in operator new src/obj-firefox/dist/include/mozilla/cxxalloc.h:33:10
    #3 0x7fdebd5dc526 in mozilla::SharedBuffer::Create(unsigned long) src/dom/media/SharedBuffer.h:71:15
    #4 0x7fdec1f81f16 in mozilla::AudioSourcePullListener::NotifyPull(mozilla::MediaTrackGraph*, long, long) src/dom/media/webrtc/MediaEngineDefault.cpp:501:33
    #5 0x7fdec186c61d in mozilla::SourceMediaTrack::PullNewData(long) src/dom/media/MediaTrackGraph.cpp:2426:8
    #6 0x7fdec186ab2a in mozilla::MediaTrackGraphImpl::UpdateGraph(long) src/dom/media/MediaTrackGraph.cpp:1137:34
    #7 0x7fdec187007a in mozilla::MediaTrackGraphImpl::OneIterationImpl(long, long, mozilla::AudioMixer*) src/dom/media/MediaTrackGraph.cpp:1362:3
    #8 0x7fdec14b9585 in mozilla::GraphRunner::Run() src/dom/media/GraphRunner.cpp:111:32
    #9 0x7fdebaf540f7 in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1241:14
    #10 0x7fdebaf5e8fc in NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:486:10
    #11 0x7fdebc184922 in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:302:20
    #12 0x7fdebc07c357 in RunInternal src/ipc/chromium/src/base/message_loop.cc:315:10
    #13 0x7fdebc07c357 in RunHandler src/ipc/chromium/src/base/message_loop.cc:308:3
    #14 0x7fdebc07c357 in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:290:3
    #15 0x7fdebaf4d21a in nsThread::ThreadFunc(void*) src/xpcom/threads/nsThread.cpp:459:11
    #16 0x7fdedefd825e in _pt_root src/nsprpub/pr/src/pthreads/ptthread.c:201:5
    #17 0x7fdedec256b9 in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x76b9)

Thread T46 (GraphRunner) created by T0 (file:// Content) here:
    #0 0x56311a95d4da in pthread_create /builds/worker/fetches/llvm-project/llvm/projects/compiler-rt/lib/asan/asan_interceptors.cc:209:3
    #1 0x7fdedefcab75 in _PR_CreateThread src/nsprpub/pr/src/pthreads/ptthread.c:458:14
    #2 0x7fdedefb88de in PR_CreateThread src/nsprpub/pr/src/pthreads/ptthread.c:533:12
    #3 0x7fdebaf4fc87 in nsThread::Init(nsTSubstring<char> const&) src/xpcom/threads/nsThread.cpp:675:8
    #4 0x7fdebaf5d455 in nsThreadManager::NewNamedThread(nsTSubstring<char> const&, unsigned int, nsIThread**) src/xpcom/threads/nsThreadManager.cpp:618:12
    #5 0x7fdebaf61773 in NS_NewNamedThread(nsTSubstring<char> const&, nsIThread**, nsIRunnable*, unsigned int) src/xpcom/threads/nsThreadUtils.cpp:139:57
    #6 0x7fdec14b7f70 in NS_NewNamedThread<12> src/obj-firefox/dist/include/nsThreadUtils.h:71:10
    #7 0x7fdec14b7f70 in mozilla::GraphRunner::Create(mozilla::MediaTrackGraphImpl*) src/dom/media/GraphRunner.cpp:37:7
    #8 0x7fdec1885e1f in mozilla::MediaTrackGraphImpl::MediaTrackGraphImpl(mozilla::MediaTrackGraph::GraphDriverType, mozilla::MediaTrackGraph::GraphRunType, int, unsigned int, mozilla::AbstractThread*) src/dom/media/MediaTrackGraph.cpp:2910:26
    #9 0x7fdec18876f2 in mozilla::MediaTrackGraph::GetInstance(mozilla::MediaTrackGraph::GraphDriverType, nsPIDOMWindowInner*, int) src/dom/media/MediaTrackGraph.cpp:3048:17
    #10 0x7fdec17f9cf8 in mozilla::GetUserMediaStreamRunnable::Run() src/dom/media/MediaManager.cpp:1162:28
    #11 0x7fdebaf540f7 in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1241:14
    #12 0x7fdebaf5e8fc in NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:486:10
    #13 0x7fdebc182e2f in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:87:21
    #14 0x7fdebc07c357 in RunInternal src/ipc/chromium/src/base/message_loop.cc:315:10
    #15 0x7fdebc07c357 in RunHandler src/ipc/chromium/src/base/message_loop.cc:308:3
    #16 0x7fdebc07c357 in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:290:3
    #17 0x7fdec2f72148 in nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:137:27
    #18 0x7fdec6a9de06 in XRE_RunAppShell() src/toolkit/xre/nsEmbedFunctions.cpp:946:20
    #19 0x7fdebc07c357 in RunInternal src/ipc/chromium/src/base/message_loop.cc:315:10
    #20 0x7fdebc07c357 in RunHandler src/ipc/chromium/src/base/message_loop.cc:308:3
    #21 0x7fdebc07c357 in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:290:3
    #22 0x7fdec6a9d4af in XRE_InitChildProcess(int, char**, XREChildData const*) src/toolkit/xre/nsEmbedFunctions.cpp:781:34
    #23 0x56311a9a5331 in content_process_main src/browser/app/../../ipc/contentproc/plugin-container.cpp:56:28
    #24 0x56311a9a5331 in main src/browser/app/nsBrowserApp.cpp:303:18

Comment 1

[Tyson Smith [:tsmith]]

Attachment: prefs.js

Use this prefs.js file when attempting to reproduce the issue.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

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

For more information, please visit auto_nag documentation.

Comment 3

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Paul, are you familiar with this code? Could you take a look at this? Please adjust the priority as needed.

Comment 4

[Paul Adenot (:padenot)]

Yes. This is using audiocapture, which is preffed off. Taking, but adjusting priority.

Comment 5

[Maire Reavy [:mreavy]]

(In reply to Paul Adenot (:padenot) from comment #4)

Yes. This is using audiocapture, which is preffed off. Taking, but adjusting priority.

Hi Andrew, Can you re-evaluate the severity of this bug given Paul's comment?

Comment 6

[Andrew McCreight [:mccr8]]

(In reply to Maire Reavy [:mreavy] Away Dec 21-Jan 2 from comment #5)

Hi Andrew, Can you re-evaluate the severity of this bug given Paul's comment?

We rate bugs as if they were enabled, and then set branches to disabled. Which is admittedly not ideal for doing a straightforward query of sec-high bugs...

Comment 7

[Daniel Veditz [:dveditz]]

For features that we intend to ship it's important to know that a blocking bug is a sec-high when evaluating when something can be enabled. If something is disabled forever (testing/debugging setting? obsolete that we can't quite kill because a segment of users?) then we do lower the severity to sec-moderate.

Comment 8

[Paul Adenot (:padenot)]

(In reply to Daniel Veditz [:dveditz] from comment #7)

For features that we intend to ship it's important to know that a blocking bug is a sec-high when evaluating when something can be enabled. If something is disabled forever (testing/debugging setting? obsolete that we can't quite kill because a segment of users?) then we do lower the severity to sec-moderate.

This feature is useful for testing, and having it is required if we want to implement a particular portion of the Web Extension API 0. We don't have immediate plans to enable it.

Comment 11

[Karl Tomlinson (:karlt)]

Attachment: Bug 1605894 close AudioContext before stopping the signal track r?padenot

so that, at the end of the test, the AudioStreamAnalyser debug canvas shows
the spectrum from before the track is stopped.

Comment 12

[Karl Tomlinson (:karlt)]

Attachment: Bug 1605894 remove an unused LoopbackTone instance r?pehrsons

As the comment indicates, this LoopbackTone is unused. It is an unnecessary
use of the global TEST_AUDIO_FREQ. If verification of gUM call stream
contents should be required in the future, then the test can choose which
frequency to use and match this with verification.

Having less realtime audio running can simplify debugging.

Depends on D193514

Comment 13

[Karl Tomlinson (:karlt)]

Attachment: Bug 1605894 use same TEST_AUDIO_FREQ regardless of whether fake or loopback audio input devices are used r?pehrsons

This no longer needs to be 440 Hz for the system device since this was
switched from a sine source to a loopback device in
https://hg.mozilla.org/mozilla-central/rev/629c42087f29#l1.40

Depends on D193515

Comment 14

[Karl Tomlinson (:karlt)]

Attachment: Bug 1605894 reduce the proliferation of DefaultLoopbackTone to only AudioStreamFlowingHelper r?pehrsons

Such side-effects are less surprising from AudioStreamFlowingHelper than from
getUserMedia().

Having less realtime audio running can simplify debugging.

Explicitly added LoopbackTones are added even without loopback devices, and
getUserMedia() in head.js disables processing of audio by default now also
regardless of prefs, so that tests run more similarly when
--use-test-media-devices is used or not.

Depends on D193516

Comment 15

[Karl Tomlinson (:karlt)]

Attachment: Bug 1605894 correct binIndexForFrequency and frequencyForBinIndex by 1 bin r?padenot

https://webaudio.github.io/web-audio-api/#fourier-transform indicates that bin
k = 0 is the mean of the time domain samples, which corresponds to a frequency
of zero. The Nyquist frequency corresponds to bin k = N/2, the first which is
not provided by getByteFrequencyData().

Depends on D193517

Comment 16

[Karl Tomlinson (:karlt)]

Attachment: Bug 1605894 use signal frequencies exactly matching DFT bins r?padenot

to reduce noise spread across bins.

Depends on D193518

Comment 17

[Karl Tomlinson (:karlt)]

Attachment: Bug 1605894 use a seamlessly looping file instead of a blob from MediaRecorder to test tab audio capture r?padenot

The blob from MediaRecorder generated blips each time the file looped.
Carefully choosing the sample rate and length did not resolve this.

The blips did not cause the test to fail but were visually confusing on the
spectrum analysis.

This also reduces the bandwidth of the tone, even between the blips.

Depends on D193519

Comment 18

[Karl Tomlinson (:karlt)]

Attachment: Bug 1605894 unmute media elements for tab audio capture test r?padenot

Depends on D193520

Comment 19

[Karl Tomlinson (:karlt)]

Attachment: Bug 1605894 support different in and out sample formats in AudioChannelsDownMix r?padenot

Having this will mean that AudioSegment::Mix() will not need an additional
buffer when mixing 16-bit audio to float (when that is supported in a future
patch).

Sample-loops are now unswitched.
16-bit arithmetic is now saturating, but I haven't optimized this.
Arithmetic is now skipped for dropped channels.

Depends on D193521

Comment 20

[Karl Tomlinson (:karlt)]

Attachment: Bug 1605894 support AUDIO_FORMAT_S16 chunks and Chunk::mVolume in AudioSegment::Mix() r?padenot

AUDIO_FORMAT_S16 is still used by MediaPipeline and fake microphones.

The shadowing of offsetSamples, which would lead to incorrect offsets when
down-mixing multiple chunks, is removed.

The code now checks for down-mixing after up-mixing, as intended by the
up-mixing design, even though this is not necessary for the current up-mixing
tables since
https://hg.mozilla.org/mozilla-central/rev/7ed8524e54f5c3d740780d52cc73510ae6e80337#l1.18

Depends on D193522

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2cb4b1085baf
close AudioContext before stopping the signal track r=padenot

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/77d5eb0444c8
remove an unused LoopbackTone instance r=padenot

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d5bccc7a9f0e
use same TEST_AUDIO_FREQ regardless of whether fake or loopback audio input devices are used r=padenot

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/45f7ef474963
correct binIndexForFrequency and frequencyForBinIndex by 1 bin r=padenot

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3d878e45d5c5
support different in and out sample formats in AudioChannelsDownMix r=padenot

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ffb4f207949a
reduce the proliferation of DefaultLoopbackTone to only AudioStreamFlowingHelper r=padenot

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9aa11e35a70b
use signal frequencies exactly matching DFT bins r=padenot
https://hg.mozilla.org/integration/autoland/rev/8a2266836594
use a seamlessly looping file instead of a blob from MediaRecorder to test tab audio capture r=padenot
https://hg.mozilla.org/integration/autoland/rev/8ca12af626f1
unmute media elements for tab audio capture test r=padenot

Comment 28

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e0d78e5ad2e5
Backed out 3 changesets for causing mochitest-media failures on test_getUserMedia_audioCapture.html. CLOSED TREE

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b92e71baa8e9
use signal frequencies exactly matching DFT bins r=padenot
https://hg.mozilla.org/integration/autoland/rev/134f08619acf
use a seamlessly looping file instead of a blob from MediaRecorder to test tab audio capture r=padenot

Comment 30

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

https://hg.mozilla.org/mozilla-central/rev/2cb4b1085baf
https://hg.mozilla.org/mozilla-central/rev/77d5eb0444c8
https://hg.mozilla.org/mozilla-central/rev/d5bccc7a9f0e
https://hg.mozilla.org/mozilla-central/rev/45f7ef474963
https://hg.mozilla.org/mozilla-central/rev/3d878e45d5c5
https://hg.mozilla.org/mozilla-central/rev/ffb4f207949a

I am leaving the bug open because not all changesets have been merged yet.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/14475becf0de
unmute media elements for tab audio capture test r=padenot
https://hg.mozilla.org/integration/autoland/rev/1f98573e788c
support AUDIO_FORMAT_S16 chunks and Chunk::mVolume in AudioSegment::Mix() r=padenot

Comment 32

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

https://hg.mozilla.org/mozilla-central/rev/b92e71baa8e9
https://hg.mozilla.org/mozilla-central/rev/134f08619acf

Comment 33

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

https://hg.mozilla.org/mozilla-central/rev/14475becf0de
https://hg.mozilla.org/mozilla-central/rev/1f98573e788c

Comment 34

[Alex Finder]

(In reply to Pulsebot from comment #25)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3d878e45d5c5
support different in and out sample formats in AudioChannelsDownMix r=padenot

== Change summary for alert #40346 (as of Wed, 22 Nov 2023 16:43:35 GMT) ==

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)	Performance Profiles
4%	webaudio	linux1804-64-shippable-qr	fission webrender	105.25 -> 100.92	Before/After
3%	webaudio	windows10-64-shippable-qr	fission webrender	83.12 -> 81.00	Before/After
2%	webaudio	linux1804-64-shippable-qr	fission webrender	104.17 -> 101.75	Before/After

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=40346

Comment 35

[Daniel Veditz [:dveditz]]

Bulk-unhiding security bugs fixed in Firefox 119-121 (Fall 2023). Use "moo-doctrine-subsidy" to filter