967972 - heap overflow read or infinite loop with mPosition overflow AudioBufferSourceNodeEngine

Status: RESOLVED FIXED

(Core :: Web Audio, defect, P2)

Description

[Karl Tomlinson (:karlt)]

If a looping AudioBufferSourceNode is allowed to play out for 13 hours, then mPosition overflows and becomes a 2^32 offset in CopyFromBuffer.

Setting playbackRate can reduce the time before overflow, but excessive cpu requirements from bug 967924 mean that the time cannot be reduced considerably.
Bug 966636 will turn the playbackRate scenario into an infinite loop instead of heap overflow, but the 13 hour case is still a heap overflow.

The overflow of mPosition happened before bug 937475, but https://hg.mozilla.org/mozilla-central/rev/2e2cdd1826e1 turned this into a -ve offset for the mOffset + t < mLoopEnd test, introducing the heap overflow.

Comment 1

[Karl Tomlinson (:karlt)]

Attachment: testcase with playbackRate

With bug 967924 fixed, this takes 30 minutes in a debug build.
If the sample count printed passes 2147483648, then the bug is fixed.
Much faster in an opt build.  Awfully slow without bug 967924 fixed.

Comment 2

[Karl Tomlinson (:karlt)]

Attachment: part 1: change DURATION engine parameter to BUFFEREND

Using absolute buffer positions will enable simplification.
mOffset will disappear in the next patch.

Comment 3

[Karl Tomlinson (:karlt)]

Attachment: part 2: include offset in buffer position

There is a change in behaviour re the offset parameter when changing the
buffer while playing to one with a different sample rate, but I'm not clear
that what we current do with mPosition in that situation is best anyway.  I
think the main thing is to ensure playback from the same place if the buffer
is changed to one with the same sample rate.

Comment 4

[Karl Tomlinson (:karlt)]

Attachment: part 3: avoid overflow in buffer position when looping

As a side-effect, the buffer playback will now continue from the same buffer
sample number (even when looping), where possible, in the following situations:

* Changing the loop start or end parameters.

* Changing the buffer to one of a different sample rate.

Comment 5

[Paul Adenot (:padenot)]

Comment on attachment 8370495 [details] [diff] [review]
part 1: change DURATION engine parameter to BUFFEREND

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

::: content/media/webaudio/AudioBufferSourceNode.h
@@ +119,5 @@
>      SAMPLE_RATE,
>      START,
>      STOP,
> +    BUFFERSTART,
> +    BUFFEREND,

This makes sense, although the names in engine parameters are usually the same as in the spec. Can you just put a comment after the new names, saying they come respectively from offset and duration?

Comment 6

[Karl Tomlinson (:karlt)]

I pushed parts 1 and 2 because part 1 is needed by bug 967992 and part 2 fits the same theme.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a908ed1d0408
https://hg.mozilla.org/integration/mozilla-inbound/rev/d26d7a023b5b

Comment 7

[Paul Adenot (:padenot)]

Comment on attachment 8370498 [details] [diff] [review]
part 3: avoid overflow in buffer position when looping

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

::: content/media/webaudio/AudioBufferSourceNode.cpp
@@ +420,5 @@
>          continue;
>        }
>        if (mLoop) {
> +        if (mBufferPosition >= mLoopEnd) {
> +          mBufferPosition = mLoopStart;

So, technically, mBufferPosition should never be > mLoopEnd, right ?

Comment 8

[Carsten Book [:Tomcat]]

landed on central as https://hg.mozilla.org/mozilla-central/rev/a908ed1d0408 and https://hg.mozilla.org/mozilla-central/rev/d26d7a023b5b

Comment 9

[Karl Tomlinson (:karlt)]

(In reply to Paul Adenot (:padenot) from comment #7)
> Comment on attachment 8370498 [details] [diff] [review]
> part 3: avoid overflow in buffer position when looping
> 
> Review of attachment 8370498 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/webaudio/AudioBufferSourceNode.cpp
> @@ +420,5 @@
> >          continue;
> >        }
> >        if (mLoop) {
> > +        if (mBufferPosition >= mLoopEnd) {
> > +          mBufferPosition = mLoopStart;
> 
> So, technically, mBufferPosition should never be > mLoopEnd, right ?

mLoopEnd can become < mBufferPosition if the "loopend" is changed or if the buffer is replaced with another of a lower sample rate, which also changes LOOPEND.  I can add a comment to note this possibility.

Comment 10

[Karl Tomlinson (:karlt)]

Attachment: part 3 fold-in: use consistent type for buffer positions

This belongs in part 3, to avoid warnings re comparisons between signed and unsigned.

Comment 11

[Karl Tomlinson (:karlt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4dc3a78b4030

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4dc3a78b4030

Still has [leave open] on the whiteboard, so treating it as such...