Closed Bug 967972 Opened 8 years ago Closed 8 years ago

heap overflow read or infinite loop with mPosition overflow AudioBufferSourceNodeEngine

Categories

(Core :: Web Audio, defect, P2)

28 Branch
x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla30
Tracking Status
firefox27 --- unaffected
firefox28 --- wontfix
firefox29 --- wontfix
firefox30 --- fixed
firefox-esr24 --- unaffected

People

(Reporter: karlt, Assigned: karlt)

References

Details

(Keywords: sec-moderate)

Attachments

(5 files)

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.
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.
Using absolute buffer positions will enable simplification.
mOffset will disappear in the next patch.
Attachment #8370495 - Flags: review?(paul)
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.
Attachment #8370497 - Flags: review?(paul)
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.
Attachment #8370498 - Flags: review?(paul)
Blocks: 967992
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?
Attachment #8370495 - Flags: review?(paul) → review+
Attachment #8370497 - Flags: review?(paul) → review+
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
Whiteboard: [leave open]
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 ?
Attachment #8370498 - Flags: review?(paul) → review+
(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.
This belongs in part 3, to avoid warnings re comparisons between signed and unsigned.
Attachment #8373093 - Flags: review?(paul)
Attachment #8373093 - Flags: review?(paul) → review+
https://hg.mozilla.org/mozilla-central/rev/4dc3a78b4030

Still has [leave open] on the whiteboard, so treating it as such...
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Target Milestone: --- → mozilla30
Group: core-security
You need to log in before you can comment on or make changes to this bug.