1830206 - Assertion failure: StorageCapacity() < std::numeric_limits<int>::max() / 2 (buffer too large for the type of index used.), at /builds/worker/workspace/obj-build/dist/include/mozilla/SPSCQueue.h:111

Status: VERIFIED FIXED

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

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.zip

Found while fuzzing m-c 20230426-17ea6f29654b (--enable-debug --enable-fuzzing)

To reproduce via Grizzly Replay:

$ pip install fuzzfetch grizzly-framework
$ python -m fuzzfetch -d --fuzzing -n firefox
$ python -m grizzly.replay ./firefox/firefox testcase.zip

The test case is made up of two parts an html file and a mp4 file. They are packaged in a zip file for bugmon.

Assertion failure: StorageCapacity() < std::numeric_limits<int>::max() / 2 (buffer too large for the type of index used.), at /builds/worker/workspace/obj-build/dist/include/mozilla/SPSCQueue.h:111

#0 0x7f2da63ea1ff in mozilla::SPSCRingBufferBase<float>::SPSCRingBufferBase(int) /builds/worker/workspace/obj-build/dist/include/mozilla/SPSCQueue.h:110:5
#1 0x7f2da63d100b in MakeUnique<mozilla::SPSCRingBufferBase<float>, unsigned int> /builds/worker/workspace/obj-build/dist/include/mozilla/UniquePtr.h:605:27
#2 0x7f2da63d100b in mozilla::AudioSink::AudioSink(mozilla::AbstractThread*, mozilla::MediaQueue<mozilla::AudioData>&, mozilla::AudioInfo const&, bool) /builds/worker/checkouts/gecko/dom/media/mediasink/AudioSink.cpp:58:7
#3 0x7f2da60b6582 in operator() /builds/worker/checkouts/gecko/dom/media/MediaDecoderStateMachine.cpp:3437:32
#4 0x7f2da60b6582 in mozilla::AudioSinkWrapper::CreatorImpl<mozilla::MediaDecoderStateMachine::CreateAudioSink()::$_0>::Create() /builds/worker/checkouts/gecko/dom/media/mediasink/AudioSinkWrapper.h:41:43
#5 0x7f2da63d7718 in mozilla::AudioSinkWrapper::StartAudioSink(mozilla::media::TimeUnit const&, mozilla::AudioSinkWrapper::AudioSinkStartPolicy) /builds/worker/checkouts/gecko/dom/media/mediasink/AudioSinkWrapper.cpp:401:32
#6 0x7f2da63e32b7 in mozilla::VideoSink::Start(mozilla::media::TimeUnit const&, mozilla::MediaInfo const&) /builds/worker/checkouts/gecko/dom/media/mediasink/VideoSink.cpp:227:29
#7 0x7f2da5ff7140 in mozilla::MediaDecoderStateMachine::StartMediaSink() /builds/worker/checkouts/gecko/dom/media/MediaDecoderStateMachine.cpp:4014:29
#8 0x7f2da5fe7f27 in mozilla::MediaDecoderStateMachine::MaybeStartPlayback() /builds/worker/checkouts/gecko/dom/media/MediaDecoderStateMachine.cpp:3610:3
#9 0x7f2da5ffed82 in mozilla::MediaDecoderStateMachine::ResumeMediaSink() /builds/worker/checkouts/gecko/dom/media/MediaDecoderStateMachine.cpp:4486:5
#10 0x7f2da60c1929 in operator()<> /builds/worker/workspace/obj-build/dist/include/nsThreadUtils.h:1164:18
#11 0x7f2da60c1929 in __invoke_impl<void, (lambda at /builds/worker/workspace/obj-build/dist/include/nsThreadUtils.h:1163:9)> /builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/lib/gcc/x86_64-linux-gnu/7/../../../../include/c++/7/bits/invoke.h:60:14
#12 0x7f2da60c1929 in __invoke<(lambda at /builds/worker/workspace/obj-build/dist/include/nsThreadUtils.h:1163:9)> /builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/lib/gcc/x86_64-linux-gnu/7/../../../../include/c++/7/bits/invoke.h:95:14
#13 0x7f2da60c1929 in __apply_impl<(lambda at /builds/worker/workspace/obj-build/dist/include/nsThreadUtils.h:1163:9), std::tuple<> &> /builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/lib/gcc/x86_64-linux-gnu/7/../../../../include/c++/7/tuple:1662:14
#14 0x7f2da60c1929 in apply<(lambda at /builds/worker/workspace/obj-build/dist/include/nsThreadUtils.h:1163:9), std::tuple<> &> /builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/lib/gcc/x86_64-linux-gnu/7/../../../../include/c++/7/tuple:1671:14
#15 0x7f2da60c1929 in apply<mozilla::MediaDecoderStateMachine, void (mozilla::MediaDecoderStateMachine::*)()> /builds/worker/workspace/obj-build/dist/include/nsThreadUtils.h:1162:12
#16 0x7f2da60c1929 in mozilla::detail::RunnableMethodImpl<mozilla::MediaDecoderStateMachine*, void (mozilla::MediaDecoderStateMachine::*)(), true, (mozilla::RunnableKind)0>::Run() /builds/worker/workspace/obj-build/dist/include/nsThreadUtils.h:1213:13
#17 0x7f2da229d5cf in mozilla::AutoTaskDispatcher::TaskGroupRunnable::Run() /builds/worker/workspace/obj-build/dist/include/mozilla/TaskDispatcher.h:230:35
#18 0x7f2da22a203c in mozilla::TaskQueue::Runner::Run() /builds/worker/checkouts/gecko/xpcom/threads/TaskQueue.cpp:259:20
#19 0x7f2da22bf385 in nsThreadPool::Run() /builds/worker/checkouts/gecko/xpcom/threads/nsThreadPool.cpp:343:14
#20 0x7f2da22b5fda in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/checkouts/gecko/xpcom/threads/nsThread.cpp:1233:16
#21 0x7f2da22bc60d in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.cpp:479:10
#22 0x7f2da2f0d01c in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) /builds/worker/checkouts/gecko/ipc/glue/MessagePump.cpp:300:20
#23 0x7f2da2e2bce1 in RunHandler /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:362:3
#24 0x7f2da2e2bce1 in MessageLoop::Run() /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:344:3
#25 0x7f2da22b1336 in nsThread::ThreadFunc(void*) /builds/worker/checkouts/gecko/xpcom/threads/nsThread.cpp:391:10
#26 0x7f2db60b8b1f in _pt_root /builds/worker/checkouts/gecko/nsprpub/pr/src/pthreads/ptthread.c:201:5
#27 0x7f2db5a94b42 in start_thread nptl/pthread_create.c:442:8
#28 0x7f2db5b269ff  misc/../sysdeps/unix/sysv/linux/x86_64/clone3.S:81

Comment 1

[Bugmon [:jkratzer for issues]]

Verified bug as reproducible on mozilla-central 20230427040619-2a66944d3ee6.
The bug appears to have been introduced in the following build range:

Start: 0b74dca33cb0b069ae2c120f72cad1949de685ca (20230111215657)
End: 9f8d4babe01d141af1e6a1a63f314d56a5c3b1d8 (20230111220339)
Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0b74dca33cb0b069ae2c120f72cad1949de685ca&tochange=9f8d4babe01d141af1e6a1a63f314d56a5c3b1d8

Comment 2

[Daniel Veditz [:dveditz]]

Possibly bug 1772769? tagging kinetik to take a look. Also padenot could weigh in on how serious to take this assertion, but sounds like a buffer overflow in the making.

Comment 3

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

Set release status flags based on info from the regressing bug 1772769

Comment 4

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

Set release status flags based on info from the regressing bug 1772769

Comment 5

[Paul Adenot (:padenot)]

Attachment: Bug 1830206 - Add an arbitrary limit to the number of audio channels a media can have. r?alwu

Comment 6

[Paul Adenot (:padenot)]

Attachment: Bug 1830206 - Tests. r?alwu

Depends on D177644

Comment 7

[Paul Adenot (:padenot)]

I think this is safe, and that the assert is bogus, I don't remember why it's divided by two. It's probably going to OOM however, and this is best caught early.

I'll double-check tomorrow.

Comment 8

[Paul Adenot (:padenot)]

Pernosco here: https://pernos.co/debug/KOjI19lpPLMim9aavvzgMA/index.html#f{m[Bavt,AA_,t[ASk,HciK_,f{e[Bavs,PTk_,s{afyGA4AAA,bAZA,uDAFvVA,oDAu/+A___/

Comment 9

[Paul Adenot (:padenot)]

Attachment: Bug 1830206 - Fix an overly restrictive assertion in the SPSC ringbuffer. r?alwu

Depends on D177645

Comment 10

[Paul Adenot (:padenot)]

So we're in the following situation:

• in a media file (mp4 here, but it can work in webm as well -- other codecs have a max number of channels that's lower), we can retrieve an uint16_t or uint32_t of arbitrary value
• we perform a multiplication with this number of channels, and a fixed value, in float, and static cast to a uint32_t (the value can exceed UINT32_MAX). This is UB if the float is too large.
• we pass this uint32_t (potentially large) in a function that takes an int, that's UB if the uint32_t is greater than INT32_MAX.
• this it is then passed to new to allocate elements. new takes size_t and that's defined regardless of the int value, so we might allocate a very large buffer if the number ended up negative. We only ever index this buffer with positive int value, so the index are always valid.

We have two UBs so anything goes. David, how do we categorize this? I have patches for everything in any case, and this affects all current builds.

Comment 11

[Paul Adenot (:padenot)]

Attachment: Bug 1830206 - Rework AudioSink's ringbuffer size computation. r?alwu

Depends on D177770

Comment 12

[Jim Mathies [:jimm]]

Patches up and approved. Waiting on sec approval.

Comment 13

[Paul Adenot (:padenot)]

Comment on attachment 9332656 [details]
Bug 1830206 - Add an arbitrary limit to the number of audio channels a media can have. r?alwu

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Hard and unpredictable, it depends on how an UB is compiled, see https://bugzilla.mozilla.org/show_bug.cgi?id=1830206#c10 for an assessment of the situation. We can https://phabricator.services.mozilla.com/D177644 on its own, fixing this bug, and it's pretty far from the crash.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
• Which older supported branches are affected by this flaw?: All
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: --
• How likely is this patch to cause regressions; how much testing does it need?: Cause and fix are clear.
• Is Android affected?: Yes

Comment 14

[Daniel Veditz [:dveditz]]

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

• we pass this uint32_t (potentially large) in a function that takes an int, that's UB if the uint32_t is greater than INT32_MAX.
• this it is then passed to new to allocate elements. new takes size_t and that's defined regardless of the int value, so we might allocate a very large buffer if the number ended up negative. We only ever index this buffer with positive int value, so the index are always valid.

We have two UBs so anything goes. Daniel, how do we categorize this? I have patches for everything in any case, and this affects all current builds.

Allocating too much with indexes that stay in bounds wouldn't be a security problem (assuming we'd catch an allocation that failed because of OOM). What if the UB truncation resulted in a very small number? Would our indexes and filling of the buffer respect that small capacity? Or would either of those be based on a parallel calculation that might not truncate in the same way? As long as everything respects the buffer capacity there shouldn't have been a security problem. If there's code that comes up with their own ideas of what ought to fit we could be in trouble.

I see you added a max # of channels. I assume there are real-world constraints on the output rate and length in seconds to make sure capacitySeconds * mOutputChannels * mOutputRate won't overflow anymore? I guess that calculation is done as double until after the clamp() is performed so yeah, it shouldn't overflow now.

Comment 15

[Daniel Veditz [:dveditz]]

bug 1772769 can't have been the regressor based on the patch, and comment 13 says ESR needs the patch also. Bugmon got it wrong

Comment 16

[Paul Adenot (:padenot)]

It's very sneaky, but bug 1772769 exposed the issue, because it made MOZ_PULSEAUDIO defined. This in turns flipped the default value of a pref: https://searchfox.org/mozilla-central/source/modules/libpref/init/StaticPrefList.yaml#9648, that restricted the maximum number of output channels an AudioSink was using: https://searchfox.org/mozilla-central/source/dom/media/VideoUtils.cpp#181-183. So in this sense this bug was impossible to trigger, because mOutputChannels was always 2 on Linux, and we're fuzzing on Linux.

The actual bug was introduced bug 1752345 (that introduced this wait-free ring-buffer), I marked it as such.

Comment 17

[Paul Adenot (:padenot)]

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

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

• we pass this uint32_t (potentially large) in a function that takes an int, that's UB if the uint32_t is greater than INT32_MAX.
• this it is then passed to new to allocate elements. new takes size_t and that's defined regardless of the int value, so we might allocate a very large buffer if the number ended up negative. We only ever index this buffer with positive int value, so the index are always valid.

We have two UBs so anything goes. Daniel, how do we categorize this? I have patches for everything in any case, and this affects all current builds.

Allocating too much with indexes that stay in bounds wouldn't be a security problem (assuming we'd catch an allocation that failed because of OOM).

As long as, in the end, we have an integer and nothing bad happened, we're good.

What if the UB truncation resulted in a very small number? Would our indexes and filling of the buffer respect that small capacity? Or would > either of those be based on a parallel calculation that might not truncate in the same way? As long as everything respects the buffer capacity > there shouldn't have been a security problem. If there's code that comes up with their own ideas of what ought to fit we could be in trouble.

No the one input to this problem is a number, that's the result of UB (or, more precisely, implementation defined behaviour). First it allocates, and two indices are set to 0 (not controllable). If the allocation fails (OOM), then it crashes. Otherwise, any operation that is possible with this data structure will work.

I see you added a max # of channels. I assume there are real-world constraints on the output rate and length in seconds to make sure capacitySeconds * mOutputChannels * mOutputRate won't overflow anymore? I guess that calculation is done as double until after the clamp() is performed so yeah, it shouldn't overflow now.

Yes, the rate was limited already. The maximum number this can have now is:

625 MB on macOS and 250MB on other OSes, but this is pushing it to the extreme, with audio a dozen times outside of the human hearing range, and 256 channels of audio. The limits are existent but extremely high on purpose. This is well in the range of all the data types involved here.

For reference, a typical size for this data structure is ~300kB (playing stereo sound) on macOS, about 140kB on other OSes.

Comment 18

[Paul Adenot (:padenot)]

Let me know how we should proceed for landing this with the details in comment 17.

Comment 19

[Tom Ritter [:tjr]]

Comment on attachment 9332656 [details]
Bug 1830206 - Add an arbitrary limit to the number of audio channels a media can have. r?alwu

Approved to request uplift and land

Comment 20

[Tom Ritter [:tjr]]

Comment on attachment 9333692 [details]
Bug 1830206 - Rework AudioSink's ringbuffer size computation. r?alwu

Approved to request uplift and land

Comment 21

[Tom Ritter [:tjr]]

Comment on attachment 9333082 [details]
Bug 1830206 - Fix an overly restrictive assertion in the SPSC ringbuffer. r?alwu

Approved to request uplift and land

Comment 22

[Tom Ritter [:tjr]]

Comment on attachment 9332657 [details]
Bug 1830206 - Tests. r?alwu

Assuming this makes it into 114, we can land the tests on or after July 18

Comment 23

[Paul Adenot (:padenot)]

Attachment: Bug 1830206 - Add an arbitrary limit to the number of audio channels a media can have. r?alwu

Original Revision: https://phabricator.services.mozilla.com/D177644

Comment 24

[Paul Adenot (:padenot)]

Attachment: Bug 1830206 - Fix an overly restrictive assertion in the SPSC ringbuffer. r?alwu

Original Revision: https://phabricator.services.mozilla.com/D177770

Comment 25

[Paul Adenot (:padenot)]

Attachment: Bug 1830206 - Rework AudioSink's ringbuffer size computation. r?alwu

Original Revision: https://phabricator.services.mozilla.com/D178074

Comment 26

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

Add an arbitrary limit to the number of audio channels a media can have. r=alwu
https://hg.mozilla.org/integration/autoland/rev/08748aa0d682ed47f293a3f77c4cc7bfff919609
https://hg.mozilla.org/mozilla-central/rev/08748aa0d682

Fix an overly restrictive assertion in the SPSC ringbuffer. r=alwu
https://hg.mozilla.org/integration/autoland/rev/32f6844e3a978a9e5a5e83746a853fb7a5aa9991
https://hg.mozilla.org/mozilla-central/rev/32f6844e3a97

Rework AudioSink's ringbuffer size computation. r=alwu
https://hg.mozilla.org/integration/autoland/rev/e8bd54b76cda45342ff07e6733d2e728822471e2
https://hg.mozilla.org/mozilla-central/rev/e8bd54b76cda

apply code formatting via Lando
https://hg.mozilla.org/integration/autoland/rev/a708c20c4e36970e5a9bb9b244a6865ad6bdcc92
https://hg.mozilla.org/mozilla-central/rev/a708c20c4e36

Comment 27

[Bugmon [:jkratzer for issues]]

Verified bug as fixed on rev mozilla-central 20230524214208-6b82236cab6f.

Comment 28

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

The patch landed in nightly and beta is affected.
:padenot, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox114 to wontfix.

For more information, please visit BugBot documentation.

Comment 29

[Paul Adenot (:padenot)]

Comment on attachment 9332656 [details]
Bug 1830206 - Add an arbitrary limit to the number of audio channels a media can have. r?alwu

Beta/Release Uplift Approval Request

• User impact if declined: Content process crash with specially crafted mp4 files.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Clear bug and fix, verified on Nightly.
• String changes made/needed: none
• Is Android affected?: Yes

Comment 30

[Pascal Chevrel:pascalc]

Comment on attachment 9332656 [details]
Bug 1830206 - Add an arbitrary limit to the number of audio channels a media can have. r?alwu

Approved for 114 beta 9 (last beta), thanks.

Comment 31

[Pascal Chevrel:pascalc]

https://hg.mozilla.org/releases/mozilla-beta/rev/be94ce2c4b10
https://hg.mozilla.org/releases/mozilla-beta/rev/bf8914b9aefe
https://hg.mozilla.org/releases/mozilla-beta/rev/81c821139a54

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9332656 [details]
Bug 1830206 - Add an arbitrary limit to the number of audio channels a media can have. r?alwu

Approved for 102.12esr.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/838b4ee62cdf
https://hg.mozilla.org/releases/mozilla-esr102/rev/1393c9aa14fb
https://hg.mozilla.org/releases/mozilla-esr102/rev/3a6eed95ff5b

Comment 34

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

a month ago, tjr placed a reminder on the bug using the whiteboard tag [reminder-test 2023-07-18] .

padenot, please refer to the original comment to better understand the reason for the reminder.

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dfce6aa1304a
Tests. r=alwu

Comment 36

[Ryan VanderMeulen [:RyanVM]]

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