heap overflow read or infinite loop with mPosition overflow AudioBufferSourceNodeEngine

RESOLVED FIXED in Firefox 30

Status

()

Core
Web Audio
P2
normal
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: karlt, Assigned: karlt)

Tracking

({sec-moderate})

28 Branch
mozilla30
x86_64
Linux
sec-moderate
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox27 unaffected, firefox28 wontfix, firefox29 wontfix, firefox30 fixed, firefox-esr24 unaffected)

Details

Attachments

(5 attachments)

(Assignee)

Description

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

Comment 1

4 years ago
Created attachment 8370487 [details] [diff] [review]
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.
(Assignee)

Comment 2

4 years ago
Created attachment 8370495 [details] [diff] [review]
part 1: change DURATION engine parameter to BUFFEREND

Using absolute buffer positions will enable simplification.
mOffset will disappear in the next patch.
Attachment #8370495 - Flags: review?(paul)
(Assignee)

Comment 3

4 years ago
Created attachment 8370497 [details] [diff] [review]
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.
Attachment #8370497 - Flags: review?(paul)
(Assignee)

Comment 4

4 years ago
Created attachment 8370498 [details] [diff] [review]
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.
Attachment #8370498 - Flags: review?(paul)
(Assignee)

Updated

4 years ago
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+

Updated

4 years ago
Attachment #8370497 - Flags: review?(paul) → review+
(Assignee)

Comment 6

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

Comment 9

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

Comment 10

4 years ago
Created attachment 8373093 [details] [diff] [review]
part 3 fold-in: use consistent type for buffer positions

This belongs in part 3, to avoid warnings re comparisons between signed and unsigned.
Attachment #8373093 - Flags: review?(paul)

Updated

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

Updated

4 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
(Assignee)

Updated

4 years ago
Target Milestone: --- → mozilla30
status-firefox-esr24: --- → unaffected
status-firefox28: affected → wontfix
status-firefox29: affected → wontfix
status-firefox30: affected → fixed
Group: core-security
You need to log in before you can comment on or make changes to this bug.