Closed Bug 1439046 Opened 4 years ago Closed 4 years ago

UBSan: division by zero in [@ WebCore::DynamicsCompressorKernel::process]

Categories

(Core :: Web Audio, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: tsmith, Assigned: padenot)

Details

(Keywords: csectype-undefined)

Attachments

(1 file)

Triggered by opening www.theglobeandmail.com

Found in mozilla-central changeset: 404146:928f0f09172f. Built with -fsanitize=float-divide-by-zero,integer-divide-by-zero

/dom/media/webaudio/blink/DynamicsCompressorKernel.cpp:324:91: runtime error: division by zero
    #0 0x7fa866867a9c in WebCore::DynamicsCompressorKernel::process(float**, float**, unsigned int, unsigned int, float, float, float, float, float, float, float, float, float, float, float, float) /dom/media/webaudio/blink/DynamicsCompressorKernel.cpp:324:91
    #1 0x7fa8668672db in WebCore::DynamicsCompressor::process(mozilla::AudioBlock const*, mozilla::AudioBlock*, unsigned int) /dom/media/webaudio/blink/DynamicsCompressor.cpp:251:18
    #2 0x7fa8668514e5 in mozilla::dom::DynamicsCompressorNodeEngine::ProcessBlock(mozilla::AudioNodeStream*, long, mozilla::AudioBlock const&, mozilla::AudioBlock*, bool*) /dom/media/webaudio/DynamicsCompressorNode.cpp:120:18
    #3 0x7fa86681b067 in mozilla::AudioNodeStream::ProcessInput(long, long, unsigned int) /dom/media/webaudio/AudioNodeStream.cpp:565:18
    #4 0x7fa866592aa8 in mozilla::MediaStreamGraphImpl::ProduceDataForStreamsBlockByBlock(unsigned int, int) /dom/media/MediaStreamGraph.cpp:1082:13
    #5 0x7fa8665949bc in mozilla::MediaStreamGraphImpl::Process() /dom/media/MediaStreamGraph.cpp:1264:11
    #6 0x7fa866594df2 in mozilla::MediaStreamGraphImpl::OneIteration(long) /dom/media/MediaStreamGraph.cpp:1353:3
    #7 0x7fa86645bece in mozilla::ThreadedDriver::RunThread() /dom/media/GraphDriver.cpp:319:40
    #8 0x7fa866474a65 in mozilla::MediaStreamGraphInitThreadRunnable::Run() /dom/media/GraphDriver.cpp:194:14
    #9 0x7fa86343b332 in nsThread::ProcessNextEvent(bool, bool*) /xpcom/threads/nsThread.cpp:1040:14
    #10 0x7fa8634570a0 in NS_ProcessNextEvent(nsIThread*, bool) /xpcom/threads/nsThreadUtils.cpp:517:10
    #11 0x7fa863ee1ddd in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) /ipc/glue/MessagePump.cpp:364:5
    #12 0x7fa863e08fa9 in RunHandler /ipc/chromium/src/base/message_loop.cc:319:3
    #13 0x7fa863e08fa9 in MessageLoop::Run() /ipc/chromium/src/base/message_loop.cc:299
    #14 0x7fa863439049 in nsThread::ThreadFunc(void*) /xpcom/threads/nsThread.cpp:423:11
    #15 0x7fa88a9bfe24 in _pt_root /nsprpub/pr/src/pthreads/ptthread.c:201:5
    #16 0x7fa88a6137fb in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x77fb)
    #17 0x7fa889641b5e in clone /build/glibc-itYbWN/glibc-2.26/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:95
Rank: 15
Priority: -- → P2
Assignee: nobody → padenot
Comment on attachment 8953395 [details]
Bug 1439046 - Guard against division by zero in DynamicsCompressorKernel.cpp.

https://reviewboard.mozilla.org/r/222672/#review228618

::: dom/media/webaudio/blink/DynamicsCompressorKernel.cpp:328
(Diff revision 1)
>          // compressionDiffDb is the difference between current compression level and the desired level.
> -        float compressionDiffDb = WebAudioUtils::ConvertLinearToDecibels(m_compressorGain / scaledDesiredGain, -1000.0f);
> +        float compressionDiffDb;
> +        if (scaledDesiredGain == 0.0) {
> +          compressionDiffDb = std::numeric_limits<float>::infinity();
> +        } else {
> +          compressionDiffDb = WebAudioUtils::ConvertLinearToDecibels(m_compressorGain / scaledDesiredGain, -1000.0f);

This div-by-zero is well defined for iec559 floats [1]. Do we have platforms where we build this with that support? Could we add a static assert for `std::numeric_limits<float>::is_iec559()` instead?

[1] http://en.cppreference.com/w/cpp/language/operator_arithmetic
Attachment #8953395 - Flags: review?(apehrson)
> This div-by-zero is well defined for iec559 floats [1]. Do we have platforms where we build this with that support?
> Could we add a static assert for `std::numeric_limits<float>::is_iec559()` instead?

The goal is probably to have a clean `-fsanitize=float-divide-by-zero` output to catch future regression.

Tyson, can you comment on this? Is there a suppression file somewhere we could use to achieve the above goal?
Flags: needinfo?(twsmith)
Flags: needinfo?(twsmith)
(In reply to Paul Adenot (:padenot) from comment #4)

> The goal is probably to have a clean `-fsanitize=float-divide-by-zero` output to catch future regression.

Yep you got it. In fact the larger overall goal is '-fsanitize=undefined'.

> Is there a suppression file somewhere we could use to achieve the above goal?

Yes, but let's avoid it if possible. I say this for a few reasons. Using the (runtime) suppression file slows down the execution because for every report/issue that is found each record in the suppression file needs to checked. It is a blunt tool, we could miss other issues in the same function/file/library depending on the method used for the suppression. It needs to be maintained. So using the suppression files (runtime or compile time) should really be saved for special cases or temporarily while waiting for a fix.

Now going in to the weeds a bit here... most of the time it is better to fix just the 'issue'. 
In this case asserting on 'std::numeric_limits<float>::is_iec559()' will assert the behavior is defined but adding code to prevent a division by zero is more correct IMO because we are dealing with the failure case. It completely gets ride of the potential for UB and all compilers and platforms will be happy regardless of supported standards (and it makes UBSan happy). Also it reads easier, you don't need to explain why you are checking for zero which should likely be the case for the static assert.

In the end it is your call :)
Flags: needinfo?(twsmith)
Comment on attachment 8953395 [details]
Bug 1439046 - Guard against division by zero in DynamicsCompressorKernel.cpp.

https://reviewboard.mozilla.org/r/222672/#review228708

Makes sense. I'll r+ and leave the decision to you Paul.
Attachment #8953395 - Flags: review+
Comment on attachment 8953395 [details]
Bug 1439046 - Guard against division by zero in DynamicsCompressorKernel.cpp.

https://reviewboard.mozilla.org/r/222672/#review228714

::: dom/media/webaudio/blink/DynamicsCompressorKernel.cpp:326
(Diff revision 1)
>          bool isReleasing = scaledDesiredGain > m_compressorGain;
>  
>          // compressionDiffDb is the difference between current compression level and the desired level.
> -        float compressionDiffDb = WebAudioUtils::ConvertLinearToDecibels(m_compressorGain / scaledDesiredGain, -1000.0f);
> +        float compressionDiffDb;
> +        if (scaledDesiredGain == 0.0) {
> +          compressionDiffDb = std::numeric_limits<float>::infinity();

With `IsInfinite()` below, do we want to be consistent and use all-mfbt or all-std?
Pushed by paul@paul.cx:
https://hg.mozilla.org/integration/autoland/rev/e3fea1e629c4
Guard against division by zero in DynamicsCompressorKernel.cpp. r=pehrsons
https://hg.mozilla.org/mozilla-central/rev/e3fea1e629c4
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.