Closed Bug 1024182 Opened 10 years ago Closed 7 years ago

Possible leak in ConvolverNode::SetBuffer()

Categories

(Core :: Web Audio, defect, P3)

Product:
Core
Component:
Web Audio
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox-esr52 --- wontfix
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- fixed

People

(Reporter: mccr8, Assigned: karlt)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, memory-leak, Whiteboard: [CID 1123255])

Attachments

(2 files)

bug 1024182 remove buffer copy for short impulse responses and so correct normalization scale
59 bytes, text/x-review-board-request
padenot
: review+
Details
bug 1024182 test normalization of convolution buffers via response concatenation
59 bytes, text/x-review-board-request
dminor
: review+
Details 
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.
Whiteboard: [CID 1123255]
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.
Priority: -- → P3
Assignee: nobody → karlt
Status: NEW → ASSIGNED
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
Attachment #8895272 - Flags: review?(padenot) → review+
Pushed by ktomlinson@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f03833f24817
remove unnecessary buffer copy for short impulse responses r=padenot
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
Flags: needinfo?(karlt)
Depends on: 1389638
See Also: → 1389641
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.
Blocks: 815643
Flags: needinfo?(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.
Pushed by ktomlinson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/92e3ce541ccd
remove buffer copy for short impulse responses and so correct normalization scale r=padenot
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 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.
https://hg.mozilla.org/mozilla-central/rev/92e3ce541ccd
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Flags: in-testsuite?
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
Attachment #8896494 - Flags: review?(dminor) → review+
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.
Pushed by ktomlinson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bad43d6929ca
test normalization of convolution buffers via response concatenation r=dminor
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: