877527 - WebAudio heap-buffer-overflow crash [@mozilla::AudioBlockCopyChannelWithScale]

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[Christoph Diehl [:posidron]]

Attachment: testcase

content/media/AudioNodeEngine.cpp:64

void
AudioBlockCopyChannelWithScale(const float* aInput,
                               float aScale,
                               float* aOutput)
{
  if (aScale == 1.0f) {
*   memcpy(aOutput, aInput, WEBAUDIO_BLOCK_SIZE*sizeof(float));
[...]
}

Comment 1

[Christoph Diehl [:posidron]]

Attachment: callstack

Comment 2

[Christoph Diehl [:posidron]]

Please see also: https://bugzilla.mozilla.org/show_bug.cgi?id=876207#c0

This is exactly where my concern is, we are not fixing the root of the problems.

I believe we should add a sanitize checks for the memcpy parameters so that we won't crash there again.

Comment 3

[Christoph Diehl [:posidron]]

Tested with http://hg.mozilla.org/integration/mozilla-inbound/rev/d7c6d6061ab5

Comment 4

[(no longer active)]

(In reply to Christoph Diehl [:cdiehl] from comment #2)
> Please see also: https://bugzilla.mozilla.org/show_bug.cgi?id=876207#c0
> 
> This is exactly where my concern is, we are not fixing the root of the
> problems.
> 
> I believe we should add a sanitize checks for the memcpy parameters so that
> we won't crash there again.

I don't think that's the case.  We can't check for the length of the buffer because AudioChunk doesn't keep track of that information at all.  We should just make sure we're consistent in keeping track of the number of channels, the offsets, etc.  There's no single root cause for these kinds of bugs, well, besides using an unsafe language!  :-)

Comment 5

[Paul Adenot (:padenot)]

This does not happen on tip for me, but I can repro on d7c6d6061ab5. Now bisecting to see what "fixed" it, because nothing seem to touch WebAudio in the log.

Comment 6

[(no longer active)]

Paul, can you please check in the test case as a crashtest?

Closing as FIXED for now, for the purpose of tracking the work left.

Comment 7

[Paul Adenot (:padenot)]

Sure, will do later today.

Comment 8

[Paul Adenot (:padenot)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0d327854300c and
https://hg.mozilla.org/integration/mozilla-inbound/rev/0bf1e71fe3cc because I forgot to hg add and actually put the wrong bug number.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Backed out for timeouts.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2de97d3a53f

https://tbpl.mozilla.org/php/getParsedLog.php?id=23724920&tree=Mozilla-Inbound

Comment 10

[(no longer active)]

Paul, please add this to the crashtest suite here:

<http://mxr.mozilla.org/mozilla-central/source/content/media/test/crashtests/>

See my recent commits to that directory for examples.  Thanks!

Comment 11

[Paul Adenot (:padenot)]

Relanded as a crashtest

https://hg.mozilla.org/integration/mozilla-inbound/rev/fd5698ce0440

Comment 12

[(no longer active)]

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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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

Comment 14

[Lukas Blakk [:lsblakk] use ?needinfo]

No longer tracking for FF23