1024182 - Possible leak in ConvolverNode::SetBuffer()

Status: RESOLVED FIXED

(Core :: Web Audio, defect, P3)

Description

[Andrew McCreight [:mccr8]]

I'm not sure if this is valid or not.  I think what Coverity is saying is that if data->GetChannels() == 0, then we don't hit the loop body, and end up leaking channelData.  Can that happen?  It seems like channelData doesn't leak if we go through the loop at least once.

Comment 1

[Karl Tomlinson (back June 29 :karlt)]

ThreadSharedFloatArrayBufferList::mContents is a fallible array so that can happen.
That's an unlikely memory allocation failure, and we have a limit of 32 channels now so it need not be a fallible allocation.

But it would be reasonable to tidy up the code so that it didn't depend so much on the behavior of AudioBuffer methods.

Comment 2

[Karl Tomlinson (back June 29 :karlt)]

Attachment: bug 1024182 remove buffer copy for short impulse responses and so correct normalization scale

Despite the comment, this was necessary only for a direct convolver stage,
which has been removed:
https://hg.mozilla.org/mozilla-central/rev/e66105937eef190dec073f1b9859e07a0272706b#l4.29

FFT convolver stages pad the buffer to the necessary length in
FFTBlock::PadAndMakeScaledDFT().

Review commit: https://reviewboard.mozilla.org/r/166440/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/166440/

Comment 3

[Karl Tomlinson (back June 29 :karlt)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9b17c6715c2a8d675d7d6e9466da2e432128dd6

Comment 4

[Paul Adenot (:padenot)]

Comment on attachment 8895272 [details]
bug 1024182 remove buffer copy for short impulse responses and so correct normalization scale

https://reviewboard.mozilla.org/r/166440/#review171634

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f03833f24817
remove unnecessary buffer copy for short impulse responses r=padenot

Comment 6

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

Backed out bug 1388656 and bug 1024182 for heap buffer overflow at AudioNodeEngine.cpp:375:12 in mozilla::AudioBufferSumOfSquares:

Bug 1388656
https://hg.mozilla.org/integration/autoland/rev/24627256990de09d0ea84eac1a7eda1d1a67c2bf
https://hg.mozilla.org/integration/autoland/rev/0f4c205ad82d371714e1a2b9a19e61bc9a21ce5c
https://hg.mozilla.org/integration/autoland/rev/1f7bad50144216f900e4683498831fb74a19c07d
https://hg.mozilla.org/integration/autoland/rev/c55ff6b0d67864730791e04036b091ebc502fce2

Bug 1024182
https://hg.mozilla.org/integration/autoland/rev/49cfa5269c2c1e40ef62f570321e754ed380536b

Push which ran failing test: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=f03833f24817e9cf748a89508eefc347033cb462&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=122195448&repo=autoland

==1104==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6020000d7a7c at pc 0x7fcddd00270c bp 0x7fcd692835e0 sp 0x7fcd692835d8
SUMMARY: AddressSanitizer: heap-buffer-overflow /home/worker/workspace/build/src/dom/media/webaudio/AudioNodeEngine.cpp:375:12 in mozilla::AudioBufferSumOfSquares(float const*, unsigned int)
REFTEST ERROR | file:///home/worker/workspace/build/tests/reftest/tests/dom/media/test/crashtests/884459.html | application timed out after 330 seconds with no output

Comment 7

[Karl Tomlinson (back June 29 :karlt)]

Attachment: bug 1024182 test normalization of convolution buffers via response concatenation

Review commit: https://reviewboard.mozilla.org/r/167744/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/167744/

Comment 8

[Karl Tomlinson (back June 29 :karlt)]

Comment on attachment 8895272 [details]
bug 1024182 remove buffer copy for short impulse responses and so correct normalization scale

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166440/diff/1-2/

Comment 9

[Karl Tomlinson (back June 29 :karlt)]

Comment on attachment 8895272 [details]
bug 1024182 remove buffer copy for short impulse responses and so correct normalization scale

https://reviewboard.mozilla.org/r/166440/#review172940

::: commit-message-ca367:1
(Diff revisions 1 - 2)
> -bug 1024182 remove unnecessary buffer copy for short impulse responses r?padenot
> +bug 1024182 remove buffer copy for short impulse responses and so correct normalization scale r=padenot

Updated the commit message to mention the scaling bug that this fixes.

Comment 10

[Karl Tomlinson (back June 29 :karlt)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e25ffac52583a8a2acbd2591f91e436b6432a5c0

Comment 11

[Karl Tomlinson (back June 29 :karlt)]

I see that web platform tests are not running on ASAN nor on Android, and so I'll probably move the test to dom/media/webaudio/test.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/92e3ce541ccd
remove buffer copy for short impulse responses and so correct normalization scale r=padenot

Comment 13

[Karl Tomlinson (back June 29 :karlt)]

Comment on attachment 8896494 [details]
bug 1024182 test normalization of convolution buffers via response concatenation

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167744/diff/1-2/

Comment 14

[Karl Tomlinson (back June 29 :karlt)]

Comment on attachment 8896494 [details]
bug 1024182 test normalization of convolution buffers via response concatenation

https://reviewboard.mozilla.org/r/167744/#review174282

::: testing/web-platform/tests/webaudio/the-audio-api/the-convolvernode-interface/convolver-normalization.html:71
(Diff revisions 1 - 2)
> -      // Blink and Gecko use a slighty different GainCalibration to that
> -      // specified, and so, instead of expecting unit output, check that the
> -      // output is constant.
> -      assert_approx_equals(output[maxIndex], output[minIndex], EPSILON,
> +      assert_approx_equals(output[maxIndex], 1.0, EPSILON,
> +                           "max output at " + maxIndex);
> +      assert_approx_equals(output[maxIndex], 1.0, EPSILON,
> +                           "min output at " + minIndex);
> -                           "output at " + maxIndex +
> -                           " differs from output at " + minIndex);

Tightened expectations after fix for bug 1389641.

Comment 15

[Karl Tomlinson (back June 29 :karlt)]

Comment on attachment 8896494 [details]
bug 1024182 test normalization of convolution buffers via response concatenation

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167744/diff/2-3/

Comment 16

[Karl Tomlinson (back June 29 :karlt)]

Comment on attachment 8896494 [details]
bug 1024182 test normalization of convolution buffers via response concatenation

https://reviewboard.mozilla.org/r/167744/#review174286

::: dom/media/webaudio/test/mochitest.ini:115
(Diff revision 3)
>  [test_convolverNode.html]
>  [test_convolverNode_mono_mono.html]
>  [test_convolverNodeChannelCount.html]
>  [test_convolverNodeDelay.html]
>  [test_convolverNodeFiniteInfluence.html]
> +[test_convolverNodeNormalization.html]

Moved to dom/media/webaudio/test for testing on more platforms.

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/92e3ce541ccd

Comment 18

[Dan Minor [:dminor]]

Comment on attachment 8896494 [details]
bug 1024182 test normalization of convolution buffers via response concatenation

https://reviewboard.mozilla.org/r/167744/#review175386

::: dom/media/webaudio/test/test_convolverNodeNormalization.html:6
(Diff revision 3)
> +<!DOCTYPE html>
> +<title>Test normalization of convolution buffers</title>
> +<script src="/resources/testharness.js"></script>
> +<script src="/resources/testharnessreport.js"></script>
> +<script>
> +const GainCalibration = 0.00125;

Please add a comment explaining how this constant was derived.

::: dom/media/webaudio/test/test_convolverNodeNormalization.html:14
(Diff revision 3)
> +const LENGTH = 12800;
> +// tolerate 16-bit math.
> +const EPSILON = 1.0 / Math.pow(2, 15);
> +
> +function test_normalization_via_response_concat(delayIndex) {
> +

nit: please remove empty line.

::: dom/media/webaudio/test/test_convolverNodeNormalization.html:73
(Diff revision 3)
> +          minIndex = i;
> +        }
> +      }
> +      assert_approx_equals(output[maxIndex], 1.0, EPSILON,
> +                           "max output at " + maxIndex);
> +      assert_approx_equals(output[maxIndex], 1.0, EPSILON,

minIndex rather than maxIndex

Comment 19

[Karl Tomlinson (back June 29 :karlt)]

Comment on attachment 8896494 [details]
bug 1024182 test normalization of convolution buffers via response concatenation

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167744/diff/3-4/

Comment 20

[Karl Tomlinson (back June 29 :karlt)]

Comment on attachment 8896494 [details]
bug 1024182 test normalization of convolution buffers via response concatenation

https://reviewboard.mozilla.org/r/167744/#review175386

> minIndex rather than maxIndex

Thank you for catching this.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bad43d6929ca
test normalization of convolution buffers via response concatenation r=dminor

Comment 22

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

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