867104 - WebAudio heap-buffer-overflow crash [@mozilla::dom::AudioBufferSourceNodeEngine::CopyFromInputBufferWithResampling]

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[Christoph Diehl [:posidron]]

Attachment: testcase

./content/media/webaudio/AudioBufferSourceNode.cpp:203

      speex_resampler_process_float(resampler, i,
                                    inputData, &inSamples,
                                    outputData, &outSamples);


./media/libspeex_resampler/src/resample.c:867

int speex_resampler_process_float
[...]
        if (in) {
           for(j=0;j<ichunk;++j)
*             x[j+filt_offs]=in[j*istride];
        } else {
          for(j=0;j<ichunk;++j)
            x[j+filt_offs]=0;
        }


Tested with m-i changeset: 130174:ea5490a3bca7

Comment 1

[Christoph Diehl [:posidron]]

Attachment: callstack

Comment 2

[Paul Adenot (:padenot)]

I can't repro, tested on Mac and Linux 64, on a just pulled m-c that contains 130174:ea5490a3bca7. Anything in particular to do?

Comment 3

[Christoph Diehl [:posidron]]

You need an ASan build.

Comment 4

[(no longer active)]

Paul, can you please look into this?  Thanks!

Comment 5

[Daniel Veditz [:dveditz]]

You can get ASAN builds (for Linux, anyway) at
https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan/

Thought we had Mac, too, but don't see them.

Comment 6

[Paul Adenot (:padenot)]

I built my own Mac ASAN build, and I found the cause (bad mDuration value in the AudioBufferSourceNodeEngine), but I'm not sure yet of how it can happen.

Comment 7

[Paul Adenot (:padenot)]

Attachment: adds a crashtest

The root cause was basically that we forgot to send along the new duration when setting a new buffer after calling |start()| on the AudioBufferSourceNode.

Comment 8

[Paul Adenot (:padenot)]

Attachment: fix

Comment 9

[(no longer active)]

Comment on attachment 748837 [details] [diff] [review]
adds a crashtest

Review of attachment 748837 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/webaudio/test/test_bug867104.html
@@ +24,5 @@
> +  setTimeout(function() {
> +    ok(true, "We did not crash.");
> +    SpecialPowers.clearUserPref("media.webaudio.enabled");
> +    SimpleTest.finish();
> +  }, 1000);

Presumably you just want to wait until audio processing starts to happen, right?  In that case it's a lot better to stick a ScriptProcessorNode in front of the source node and wait for its first audioprocess event, and then end the test.

Comment 10

[Paul Adenot (:padenot)]

pushed with comments addressed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/744de2aced8c
https://hg.mozilla.org/integration/mozilla-inbound/rev/d98e7c680020

Comment 11

[(no longer active)]

Comment on attachment 748838 [details] [diff] [review]
fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Web Audio
User impact if declined: Web Audio will only remain enabled on Aurora, so this won't impact beta/release users unless they manually turn it on
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): not risky
String or IDL/UUID changes made by this patch: none

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/744de2aced8c
https://hg.mozilla.org/mozilla-central/rev/d98e7c680020

Comment 13

[(no longer active)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/913b31ff6094
https://hg.mozilla.org/releases/mozilla-aurora/rev/3b1a78efc021

Comment 14

[(no longer active)]

Mass moving Web Audio bugs to the Web Audio component.  Filter on duckityduck.