Possible leak in ConvolverNode::SetBuffer()

RESOLVED FIXED in Firefox 57

Status

()

defect
P3
normal
RESOLVED FIXED
5 years ago
10 months ago

People

(Reporter: mccr8, Assigned: karlt)

Tracking

(Blocks 1 bug, {coverity, memory-leak})

Trunk
mozilla57
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox55 wontfix, firefox56 wontfix, firefox57 fixed)

Details

(Whiteboard: [CID 1123255])

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Updated

5 years ago
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)

Updated

2 years ago
Assignee: nobody → karlt
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)

Comment 4

2 years ago
mozreview-review
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+

Comment 5

2 years ago
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)
(Assignee)

Updated

2 years ago
Depends on: 1389638
(Assignee)

Updated

2 years ago
See Also: → 1389641
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 9

2 years ago
mozreview-review
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.
(Assignee)

Updated

2 years ago
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.

Comment 12

2 years ago
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 hidden (mozreview-request)
(Assignee)

Comment 14

2 years ago
mozreview-review
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 hidden (mozreview-request)
(Assignee)

Comment 16

2 years ago
mozreview-review
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/92e3ce541ccd
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
(Assignee)

Updated

2 years ago
Flags: in-testsuite?

Comment 18

2 years ago
mozreview-review
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 hidden (mozreview-request)
(Assignee)

Comment 20

2 years ago
mozreview-review-reply
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

2 years ago
Pushed by ktomlinson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bad43d6929ca
test normalization of convolution buffers via response concatenation r=dminor
(Assignee)

Updated

2 years ago
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.