991533 - (CVE-2014-1542) limit AudioBuffer channel counts and sample rate range

Status: VERIFIED FIXED

(Core :: Web Audio, defect)

Description

[Karl Tomlinson (:karlt)]

Bug 987679 identified a critical security bug using the Speex resampler with
extreme down-sampling ratios and/or channel counts.

When the resampler code is patched to fix that, there still remains a null
deref crash.

We don't need to handle so many channels and large numbers of channels have
led to other security bugs such as bug 990794.  There have been previous
attempts to limit the number of channels in bug 877039 and bug 987976 but
there are still ways to go beyond the intended limit of 32 channels.

We can't hope to support extreme down-sampling ratios successfully because
this requires processing a huge number of input samples per output sample, so
there is little harm in applying a limit on sample rates.

Comment 1

[Karl Tomlinson (:karlt)]

Bug 990868 also adds a channel count limit.

Comment 2

[Karl Tomlinson (:karlt)]

Attachment: part 1: move sample rate limits to WebAudioUtils

Also change WebAudioUtils from a class to a namespace, so that constant
variables can be defined inline with internal linkage.

static class variables cannot be defined inline because this violates the one
definition rule, even though some compilers may not notice.

I don't mind whether these are all defined in AudioBuffer or AudioContext or WebAudioUtils, but it is good to have one place.

Comment 3

[Karl Tomlinson (:karlt)]

Attachment: part 2: limit OfflineAudioContext to the same sample rates as AudioBuffer

OfflineAudioCompletionEvent needs to use AudioBuffer for its output,
and so the AudioContext should run at the rates supported by AudioBuffer.

Currently AudioBufferSourceNode down-sampling ratios are limited by the
maximum buffer sample rate, so reducing maximum sample rate of offline context
buffers (from 2^20) is an important part of avoiding the situation of bug
987679.

The size of the memory allocation affected in bug 987679 is
(nb_channels * mem_alloc_size * sizeof(float_or_int16_t).
mem_alloc_size = st->filt_len + 159.
For down-sampling, num_rate > den_rate,
st->filt_len is quality_map[quality].base_length * st->num_rate / st->den_rate,
rounded up to the next multiple of 8.

If we merely limit the channel count to 32 and reduce the maximum buffer
sample rate to 192000 < 1.5 * 2^17 then the memory allocation size with float
values becomes (64*192000+159)*32*4 = 1572884352 < 1.5 * 2^30 and
so becomes just under the current overflow at 2^31.

We can reduce the down-sampling ratio more than this for performance sanity
reasons, but avoiding the overflow is the main goal right now.

Comment 4

[Karl Tomlinson (:karlt)]

Attachment: limit AudioBuffers from decodeAudioData to the same sample rates as createBuffer

The numberOfChannels array on AudioBuffer is now an infallible array
allocation, as this is considerably smaller than infallible channel data array
allocations in AllocateAudioBlock and similar to channel data pointer array
allocations in AudioChunk.

This will also allow us to limit AudioBuffer channel count in one place.

Comment 5

[Karl Tomlinson (:karlt)]

Attachment: part 4: limit AudioBuffer channel count

Comment 6

[(no longer active)]

Comment on attachment 8401166 [details] [diff] [review]
limit AudioBuffers from decodeAudioData to the same sample rates as createBuffer

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

(In reply to Karl Tomlinson (:karlt) from comment #4)
> Created attachment 8401166 [details] [diff] [review]
> limit AudioBuffers from decodeAudioData to the same sample rates as
> createBuffer
> 
> The numberOfChannels array on AudioBuffer is now an infallible array
> allocation, as this is considerably smaller than infallible channel data
> array
> allocations in AllocateAudioBlock and similar to channel data pointer array
> allocations in AudioChunk.

I'm not convinced this is the right thing to do, even though the above may be technically true.  The issue here is that this is directly controllable by content, while not every AudioChunk's channel count is directly controllable from content.  Consider this test case for example:

(new AudioContext()).createBuffer(0xffffffff, 100, 48000)

Right now, we throw an exception.  Your patch will convert this into an OOM crash at least on 32-bit systems.  Please let me know if I'm missing something.

::: content/media/webaudio/AudioBuffer.cpp
@@ +46,5 @@
>    : mContext(aContext),
>      mLength(aLength),
>      mSampleRate(aSampleRate)
>  {
> +  mJSChannels.SetCapacity(aNumberOfChannels);

(When you call into AudioBuffer::Create, this would be where the OOM will happen, before any checks on aNumberOfChannels.)

@@ +81,5 @@
> +
> +  nsRefPtr<AudioBuffer> buffer =
> +    new AudioBuffer(aContext, aNumberOfChannels, aLength, aSampleRate);
> +
> +  if (aLength > INT32_MAX) {

Based on my comment in the header, I'd move this up before we allocate the AudioBuffer object.

::: content/media/webaudio/AudioBuffer.h
@@ +35,5 @@
>  public:
> +  /*
> +   * Returns a new AudioBuffer iff the channel, length, and rate parameters
> +   * are within range, even if memory cannot be allocated.  aRV.Failed() will
> +   * return true when no buffer is returned and when memory allocation fails.

Is this what we really want here?  I'm not sure if it's a good idea to return an object in this weird state.  I'm pretty sure that we make the assumption that mJSChannel contains something useful in various places of the code, and doing this means that those assumptions will now be wrong.

I think we should return null here if the checks fail, and deal with that situation in the caller.  From a practical perspective, this of course only makes a difference in AudioProcessingEvent and MediaBufferDecoder, the rest of the callers already ignore the return value in case of a failure.

::: content/media/webaudio/AudioContext.cpp
@@ +187,1 @@
>      aRv.Throw(NS_ERROR_DOM_INDEX_SIZE_ERR);

Nit: according to the spec we should raise a NOT_SUPPORTED_ERR here, please file a follow-up?

::: content/media/webaudio/AudioProcessingEvent.cpp
@@ +44,5 @@
>  
> +  ErrorResult rv;
> +  aBuffer = AudioBuffer::Create(mNode->Context(), aNumberOfChannels,
> +                                mNode->BufferSize(),
> +                                mNode->Context()->SampleRate(), cx, rv);

Hmm, so I understand that this code originally omits any error checking, but I'm pretty sure that the consumer in ScriptProcessorNodeEngine::Command::Run is broken if we fail here.  Please either fix that or file a follow-up.

Comment 7

[(no longer active)]

Argh, sorry, just saw attachment 8401168 [details] [diff] [review]!  Please ignore the fallibility part of my comment above, but the part about returning a half-initialized object remains.

Comment 8

[Karl Tomlinson (:karlt)]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #6)
> @@ +81,5 @@
> > +
> > +  nsRefPtr<AudioBuffer> buffer =
> > +    new AudioBuffer(aContext, aNumberOfChannels, aLength, aSampleRate);
> > +
> > +  if (aLength > INT32_MAX) {
> 
> Based on my comment in the header, I'd move this up before we allocate the
> AudioBuffer object.

I don't know which comment exactly you are referring to but I'd been thinking
that might be better anyway.  It's not really a NS_ERROR_OUT_OF_MEMORY.
Perhaps a NS_ERROR_DOM_INDEX_SIZE_ERR, but NOT_SUPPORTED_ERR would be more
consistent with the spec.

> ::: content/media/webaudio/AudioBuffer.h
> @@ +35,5 @@
> >  public:
> > +  /*
> > +   * Returns a new AudioBuffer iff the channel, length, and rate parameters
> > +   * are within range, even if memory cannot be allocated.  aRV.Failed() will
> > +   * return true when no buffer is returned and when memory allocation fails.
> 
> Is this what we really want here?  I'm not sure if it's a good idea to
> return an object in this weird state.

Yes, it's odd to have two different failure modes.

This was mostly to retain existing behavior for the event buffer getters.

I had also been thinking about a future optimization of delaying the
allocation until required as an optimization for null buffers (creating a zero
buffer of specified length for AudioBufferSourceNode, perhaps).  There is no
requirement of when we must return the OOM exception, and we already throw
exceptions later for failed allocations after neutering anyway.

However, while we have the code structured so that it already knows there is
an allocation failure, then perhaps it makes sense to advertise this knowledge
ASAP.  Not returning a buffer will mean that sampleRate, length, and duration
are also not available, but they're probably not much use on their own in real
situations.  numberOfChannels is already wrong, but could be made correct even
after OOM if we wanted.

> I'm pretty sure that we make the
> assumption that mJSChannel contains something useful in various places of
> the code, and doing this means that those assumptions will now be wrong.

The rest of the code is working out OK because it always checks the channel
count, which is calculated from the number of mJSChannels.

> I think we should return null here if the checks fail, and deal with that
> situation in the caller.

This patch was keeping the existing behaviour on OOM.  If there's a simple way
that's clearly better then I'm keen to include in this patch, but otherwise
I'd like to defer to another bug.

I'll look into throwing from the event buffer getters.

> From a practical perspective, this of course only
> makes a difference in AudioProcessingEvent and MediaBufferDecoder, the rest
> of the callers already ignore the return value in case of a failure.

MediaBufferDecoder is simple whatever Create() returns because we can use the
failure path.

It's just AudioProcessingEvent where there needs to be a behavior change
because it currently ignores the return value and so currently never needs to
handle null.  The getters will need to throw (until Create defers allocation).

> 
> ::: content/media/webaudio/AudioContext.cpp
> @@ +187,1 @@
> >      aRv.Throw(NS_ERROR_DOM_INDEX_SIZE_ERR);
> 
> Nit: according to the spec we should raise a NOT_SUPPORTED_ERR here, please
> file a follow-up?

Thank you.  I can easily touch that up in this patch.

> ::: content/media/webaudio/AudioProcessingEvent.cpp
> @@ +44,5 @@
> >  
> > +  ErrorResult rv;
> > +  aBuffer = AudioBuffer::Create(mNode->Context(), aNumberOfChannels,
> > +                                mNode->BufferSize(),
> > +                                mNode->Context()->SampleRate(), cx, rv);
> 
> Hmm, so I understand that this code originally omits any error checking, but
> I'm pretty sure that the consumer in ScriptProcessorNodeEngine::Command::Run
> is broken if we fail here.  Please either fix that or file a follow-up.

I haven't found where ScriptProcessorNodeEngine::Command::Run() is broken.
If the bug is only there because of this patch or the changes you are
requesting, can you be more specific, please?
If there is an existing bug, then you can file it.  No need to double handle through me.

Comment 9

[Karl Tomlinson (:karlt)]

Attachment: part 3a: throw exception from AudioProcessingEvent buffer getters when allocation fails

This will allow AudioBuffer::Create() to return null, as requested in comment 6.

Comment 10

[Karl Tomlinson (:karlt)]

Attachment: part 3b: limit AudioBuffers from decodeAudioData to the same sample rates as createBuffer

(In reply to Karl Tomlinson (:karlt) from comment #8)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> > ::: content/media/webaudio/AudioContext.cpp
> > @@ +187,1 @@
> > >      aRv.Throw(NS_ERROR_DOM_INDEX_SIZE_ERR);
> > 
> > Nit: according to the spec we should raise a NOT_SUPPORTED_ERR here, please
> > file a follow-up?
> 
> Thank you.  I can easily touch that up in this patch.

I changed my mind here.  Best to keep behavior changes separate from code
refactoring and I'd like to uplift this patch to branches.  Whether
NOT_SUPPORTED_ERR is better is also debatable because other methods such as
createScriptProcessor throw INDEX_SIZE_ERR on out of range channel counts and
buffer lengths.

Comment 11

[(no longer active)]

(In reply to Karl Tomlinson (:karlt) from comment #10)
> Created attachment 8406624 [details] [diff] [review]
> part 3b: limit AudioBuffers from decodeAudioData to the same sample rates as
> createBuffer
> 
> (In reply to Karl Tomlinson (:karlt) from comment #8)
> > (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> > > ::: content/media/webaudio/AudioContext.cpp
> > > @@ +187,1 @@
> > > >      aRv.Throw(NS_ERROR_DOM_INDEX_SIZE_ERR);
> > > 
> > > Nit: according to the spec we should raise a NOT_SUPPORTED_ERR here, please
> > > file a follow-up?
> > 
> > Thank you.  I can easily touch that up in this patch.
> 
> I changed my mind here.  Best to keep behavior changes separate from code
> refactoring and I'd like to uplift this patch to branches.

Yeah, good point, agreed.

> Whether
> NOT_SUPPORTED_ERR is better is also debatable because other methods such as
> createScriptProcessor throw INDEX_SIZE_ERR on out of range channel counts and
> buffer lengths.

Honestly I don't care much either way.  We can either fix the spec or our implementation.  Paul, what do you think?

Comment 12

[(no longer active)]

(In reply to Karl Tomlinson (:karlt) from comment #8)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #6)
> > @@ +81,5 @@
> > > +
> > > +  nsRefPtr<AudioBuffer> buffer =
> > > +    new AudioBuffer(aContext, aNumberOfChannels, aLength, aSampleRate);
> > > +
> > > +  if (aLength > INT32_MAX) {
> > 
> > Based on my comment in the header, I'd move this up before we allocate the
> > AudioBuffer object.
> 
> I don't know which comment exactly you are referring to but I'd been thinking
> that might be better anyway.  It's not really a NS_ERROR_OUT_OF_MEMORY.
> Perhaps a NS_ERROR_DOM_INDEX_SIZE_ERR, but NOT_SUPPORTED_ERR would be more
> consistent with the spec.

I meant the "Is this what we really want here?" comment...

> > ::: content/media/webaudio/AudioBuffer.h
> > @@ +35,5 @@
> > >  public:
> > > +  /*
> > > +   * Returns a new AudioBuffer iff the channel, length, and rate parameters
> > > +   * are within range, even if memory cannot be allocated.  aRV.Failed() will
> > > +   * return true when no buffer is returned and when memory allocation fails.
> > 
> > Is this what we really want here?  I'm not sure if it's a good idea to
> > return an object in this weird state.
> 
> Yes, it's odd to have two different failure modes.
> 
> This was mostly to retain existing behavior for the event buffer getters.
> 
> I had also been thinking about a future optimization of delaying the
> allocation until required as an optimization for null buffers (creating a
> zero
> buffer of specified length for AudioBufferSourceNode, perhaps).  There is no
> requirement of when we must return the OOM exception, and we already throw
> exceptions later for failed allocations after neutering anyway.
> 
> However, while we have the code structured so that it already knows there is
> an allocation failure, then perhaps it makes sense to advertise this
> knowledge
> ASAP.  Not returning a buffer will mean that sampleRate, length, and duration
> are also not available, but they're probably not much use on their own in
> real
> situations.  numberOfChannels is already wrong, but could be made correct
> even
> after OOM if we wanted.
> 
> > I'm pretty sure that we make the
> > assumption that mJSChannel contains something useful in various places of
> > the code, and doing this means that those assumptions will now be wrong.
> 
> The rest of the code is working out OK because it always checks the channel
> count, which is calculated from the number of mJSChannels.
> 
> > I think we should return null here if the checks fail, and deal with that
> > situation in the caller.
> 
> This patch was keeping the existing behaviour on OOM.  If there's a simple
> way
> that's clearly better then I'm keen to include in this patch, but otherwise
> I'd like to defer to another bug.

Agreed on keeping the existing behavior here.

> I'll look into throwing from the event buffer getters.
> 
> > From a practical perspective, this of course only
> > makes a difference in AudioProcessingEvent and MediaBufferDecoder, the rest
> > of the callers already ignore the return value in case of a failure.
> 
> MediaBufferDecoder is simple whatever Create() returns because we can use the
> failure path.
> 
> It's just AudioProcessingEvent where there needs to be a behavior change
> because it currently ignores the return value and so currently never needs to
> handle null.  The getters will need to throw (until Create defers
> allocation).

Yep, that's fine.

> > ::: content/media/webaudio/AudioContext.cpp
> > @@ +187,1 @@
> > >      aRv.Throw(NS_ERROR_DOM_INDEX_SIZE_ERR);
> > 
> > Nit: according to the spec we should raise a NOT_SUPPORTED_ERR here, please
> > file a follow-up?
> 
> Thank you.  I can easily touch that up in this patch.
> 
> > ::: content/media/webaudio/AudioProcessingEvent.cpp
> > @@ +44,5 @@
> > >  
> > > +  ErrorResult rv;
> > > +  aBuffer = AudioBuffer::Create(mNode->Context(), aNumberOfChannels,
> > > +                                mNode->BufferSize(),
> > > +                                mNode->Context()->SampleRate(), cx, rv);
> > 
> > Hmm, so I understand that this code originally omits any error checking, but
> > I'm pretty sure that the consumer in ScriptProcessorNodeEngine::Command::Run
> > is broken if we fail here.  Please either fix that or file a follow-up.
> 
> I haven't found where ScriptProcessorNodeEngine::Command::Run() is broken.
> If the bug is only there because of this patch or the changes you are
> requesting, can you be more specific, please?
> If there is an existing bug, then you can file it.  No need to double handle
> through me.

Karl, sorry if my tone upset you.  I didn't mean to make you file my bugs :) I was just asking for your opinion.  Looking at this more closely, we have the |mLength != JS_GetTypedArrayLength(mJSChannels[i])| check in AudioBuffer::GetThreadSharedChannelsForRate so this is actually OK.

Comment 13

[(no longer active)]

Comment on attachment 8406622 [details] [diff] [review]
part 3a: throw exception from AudioProcessingEvent buffer getters when allocation fails

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

::: content/media/webaudio/ScriptProcessorNode.cpp
@@ +390,5 @@
>            // Steal the output buffers
>            nsRefPtr<ThreadSharedFloatArrayBufferList> output;
>            if (event->HasOutputBuffer()) {
> +            ErrorResult rv;
> +            output = event->GetOutputBuffer(rv)->GetThreadSharedChannelsForRate(cx);

Please add a comment here saying that ignoring the error value here is OK because GetThreadSharedChannelsForRate() knows how to validate its internal state and will return null if those checks fail, which is handled fine in FinishProducingOutputBuffer.

Comment 14

[(no longer active)]

Comment on attachment 8406624 [details] [diff] [review]
part 3b: limit AudioBuffers from decodeAudioData to the same sample rates as createBuffer

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

::: content/media/webaudio/AudioBuffer.cpp
@@ +71,4 @@
>  {
> +  // Note that a buffer with zero channels is permitted here for the sake of
> +  // AudioProcessingEvent, where channel counts must match parameters passed
> +  // to createScriptProcessor(), one of which may be zero.

I'm still a little bit uneasy about this, but don't really have a better idea...

Comment 15

[Karl Tomlinson (:karlt)]

Attachment: rollup for 30 branch

Comment 16

[Karl Tomlinson (:karlt)]

Attachment: rollup for 29 branch

Comment 17

[Karl Tomlinson (:karlt)]

Attachment: rollup for b2g26 and b2g28 branches

Comment 18

[Karl Tomlinson (:karlt)]

sec-critical because this fixes bug 987679, and patches in that bug are not likely to land soon.

Comment 19

[Karl Tomlinson (:karlt)]

Comment on attachment 8407361 [details] [diff] [review]
rollup for 30 branch

Requesting approval to land all 5 mozilla-central patches in this bug.

If this is not going to make the last beta for 29, then it is probably not the right part of the cycle to land this change.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

The patches indicate the Web Audio features that need to be stressed to exploit bug 987679, but not how to put them together in the right way to generate the exploit.  Someone with a fuzzer might be given an advantage by looking at the patches.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

No.  The problem is actually in code that is not patched here, but these changes prevent exploiting the flaw.

Which older supported branches are affected by this flaw?

Anything based on 25 or newer.

If not all supported branches, which bug introduced the flaw?

Not sure.  Something early in the Web Audio implementation.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

I have backports.  There were several minor merge conflicts to resolve.  I have not compiled every branch.

How likely is this patch to cause regressions; how much testing does it need?

There is some risk, not because of complexity, but because of the number of pieces of code that are changed.  Risk is limited to Web Audio.  There are a number of automated tests for the features involved here.

Comment 20

[Paul Adenot (:padenot)]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #11)
> (In reply to Karl Tomlinson (:karlt) from comment #10)
> > Created attachment 8406624 [details] [diff] [review]
> > part 3b: limit AudioBuffers from decodeAudioData to the same sample rates as
> > createBuffer
> > 
> > (In reply to Karl Tomlinson (:karlt) from comment #8)
> > > (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> > > > ::: content/media/webaudio/AudioContext.cpp
> > > > @@ +187,1 @@
> > > > >      aRv.Throw(NS_ERROR_DOM_INDEX_SIZE_ERR);
> > > > 
> > > > Nit: according to the spec we should raise a NOT_SUPPORTED_ERR here, please
> > > > file a follow-up?
> > > 
> > > Thank you.  I can easily touch that up in this patch.
> > 
> > I changed my mind here.  Best to keep behavior changes separate from code
> > refactoring and I'd like to uplift this patch to branches.
> 
> Yeah, good point, agreed.
> 
> > Whether
> > NOT_SUPPORTED_ERR is better is also debatable because other methods such as
> > createScriptProcessor throw INDEX_SIZE_ERR on out of range channel counts and
> > buffer lengths.
> 
> Honestly I don't care much either way.  We can either fix the spec or our
> implementation.  Paul, what do you think?

I'd prefer NOT_SUPPORTED_ERR: this is not an out of bound access or the like, it's just something that the implementation does not support. Other implementation can choose to have different limits.

This can clearly be a followup, though.

Comment 21

[Al Billings [:abillings - ex-MoCo]]

Sylvestre, can we get this into 29? If not, then we'll take it in the middle of the next cycle. We don't want to land this if it can't get onto the Beta branch.

Comment 22

[Sylvestre Ledru [:Sylvestre]]

Due to the size of the patch, I would prefer to skip 29 for this. We are going to launch the gbt tomorrow for beta 9. Therefor, this change won't get much testing and we won't have much time if we need to fix bugs.

Comment 23

[Al Billings [:abillings - ex-MoCo]]

I'm going to give sec-approval+ for checkin on trunk after 5/15/2014. Please wait until then to check in the code.

Comment 24

[Karl Tomlinson (:karlt)]

Comment on attachment 8406622 [details] [diff] [review]
part 3a: throw exception from AudioProcessingEvent buffer getters when allocation fails

Boris, can you look at the webidl changes here, please?
They need a DOM peer review since https://hg.mozilla.org/hgcustom/hghooks/rev/cacacc4b45e0

This change was necessary to satisfy the "I think we should return null here if the checks fail" request from review in comment 6.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8406622 [details] [diff] [review]
part 3a: throw exception from AudioProcessingEvent buffer getters when allocation fails

The WebIDL bits look fine, but it may be a good idea to add a no-arg version of GetOutputBuffer that just returns mOutputBuffer and use that in ScriptProcessorNode, because it's not obvious without some careful code reading why ignoring the ErrorResult there is ok.

Comment 26

[Andrew McCreight [:mccr8]]

This is okay to land on inbound now.

Comment 27

[Karl Tomlinson (:karlt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/20ac3d5b42d1
https://hg.mozilla.org/integration/mozilla-inbound/rev/1193f184836a
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3f7816af4fe
https://hg.mozilla.org/integration/mozilla-inbound/rev/b11cb148e3b2
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c5ed45ffddb

Comment 28

[Karl Tomlinson (:karlt)]

The patches had to be merged with changes from bug 1006024, most significantly, the new early return in AudioProcessingEvent::LazilyCreateBuffer() needed an error code.

https://hg.mozilla.org/mozilla-central/rev/67bff024b230#l1.34

I chose NS_ERROR_UNEXPECTED because this path would previously have crashed if trying to return null through webidl.
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3f7816af4fe#l1.23

(In reply to :Ehsan Akhgari (@work week, needinfo? me!) from comment #13)
> >            // Steal the output buffers
> >            nsRefPtr<ThreadSharedFloatArrayBufferList> output;
> >            if (event->HasOutputBuffer()) {
> > +            ErrorResult rv;
> > +            output = event->GetOutputBuffer(rv)->GetThreadSharedChannelsForRate(cx);
> 
> Please add a comment here saying that ignoring the error value here is OK
> because GetThreadSharedChannelsForRate() knows how to validate its internal
> state and will return null if those checks fail, which is handled fine in
> FinishProducingOutputBuffer.

(In reply to Boris Zbarsky [:bz] from comment #25)
> it may be a good idea to add a no-arg version
> of GetOutputBuffer that just returns mOutputBuffer and use that in
> ScriptProcessorNode, because it's not obvious without some careful code
> reading why ignoring the ErrorResult there is ok.

I added comments explaining the HasOutputBuffer()/GetOutputBuffer(rv)
relationship, as well as an assert, and noting that
GetThreadSharedChannelsForRate() can also return null.

Even with a no-arg version, skipping the null check would not be safe without
the HasOutputBuffer() check.

Comment 29

[Karl Tomlinson (:karlt)]

Attachment: rollup for 31 branch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Web Audio
User impact if declined: sec-critical (see bug 987679)
Testing completed (on m-c, etc.): on m-i, requesting approval to land once merged to m-c.
Risk to taking this patch (and alternatives if risky): 
There is some risk, not because of complexity, but because of the number of pieces of code that are changed.  Risk is limited to Web Audio.  There are a number of automated tests for the features involved here.
String or IDL/UUID changes made by this patch: none.

Comment 30

[Lukas Blakk [:lsblakk] use ?needinfo]

Ryan has this landed on central yet?  Hopefully this can get landed in time for tomorrow morning's beta too.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Yes, it was merged last Friday.

https://hg.mozilla.org/mozilla-central/rev/20ac3d5b42d1
https://hg.mozilla.org/mozilla-central/rev/1193f184836a
https://hg.mozilla.org/mozilla-central/rev/d3f7816af4fe
https://hg.mozilla.org/mozilla-central/rev/b11cb148e3b2
https://hg.mozilla.org/mozilla-central/rev/8c5ed45ffddb

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1be457e6d85b
https://hg.mozilla.org/releases/mozilla-beta/rev/ece051d029de

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/7052edb3da7f
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/7f26289a3718

Comment 34

[Ryan VanderMeulen [:RyanVM]]

Backed out from b2g28/b2g26 for bustage.
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/a77ac81bc4af
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/f520cd2f276b

https://tbpl.mozilla.org/php/getParsedLog.php?id=40014795&tree=Mozilla-B2g28-v1.3

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/ece051d029de

Comment 36

[Karl Tomlinson (:karlt)]

Attachment: rollup v2 for b2g26 and b2g28 branches

using namespace mozilla::dom in HRTFPanner.cpp, as in DynamicsCompressorKernel.cpp.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/4555f68c30ff
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/55233e2d84bb

Comment 38

[Karl Tomlinson (:karlt)]

Attachment: followup exception code fix for b2g26 and b2g28 branches

Revert rate/channels/length exception to old NOT_SUPPORTED_ERR

This exception was accidentally changed to INDEX_SIZE_ERR in attachment 8425886 [details] [diff] [review].

Versions prior to 29 still had NOT_SUPPORTED_ERR
http://hg.mozilla.org/mozilla-central/rev/6bc4294d2c85

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/80159800cb40
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/7a2645d90153

Comment 40

[Kamil Jozwiak [:kjozwiak]]

Reproduced the original issue using the following build:
- http://inbound-archive.pub.build.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan/1398418578/

As indicated by the whiteboard tag, I opened the POC from Bug # 987679 in the above build and received the heap-buffer-overflow crash instantly.. (reproduced it multiple times)

Once reproduced, I used the following builds during verification:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan/latest/
- http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-aurora-linux64-asan/latest/
- http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-beta-linux64-asan/latest/

Went through the following test cases and ensured there wasn't any crashes:

- Opened the POC with a single tab
- Opened the POC with multiple tabs
- Opened the POC under multiple windows
- Opened the POC under multiple e10s windows (only in m-c)
- Opened the POC under multiple private windows/tabs
- Closed fx via the "X"
- Closed fx via the "Quit Firefox" button under the "Open Menu"
- Ensured that the browser functioned without any issues while the POC was opened in a tab

Please re-open if there's anything that should have been tested but was overlooked/missed!

Comment 41

[Justin Wood (:Callek)]

I applied the (obsoleted) Gecko 29 rollup patch onto a relbranch to serve SeaMonkey 2.26.1

https://hg.mozilla.org/releases/mozilla-release/rev/ac90473bd438

had to apply the change from https://bugzilla.mozilla.org/attachment.cgi?oldid=8407363&action=interdiff&newid=8425886&headers=1

Comment 42

[Al Billings [:abillings - ex-MoCo]]

Paying bounty to  Holger Fuhrmannek for bug 987679 here.