883591 - WebAudio ASSERTION: Stream did not produce enough data: 'stream->mBuffer.GetEnd() >= GraphTimeToStreamTime(stream, mStateComputedTime)'

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[Christoph Diehl [:posidron]]

Attachment: testcase

Testcase needs location.reload() to trigger the assertion. This assertion gets called repeatedly and messes with the output of upcoming testcases made by the fuzzer -- this is not caused by the recursion through location.reload() in the testcase.

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

Comment 1

[(no longer active)]

Josh, can you please take a look?

Comment 3

[Christoph Diehl [:posidron]]

Interestingly it is not crashing with my MacOS build but on Linux it crashes very fast. The stack is different with each crash.

Comment 4

[Paul Adenot (:padenot)]

Comment on attachment 763189 [details]
testcase

Setting as text plain so I don't crash my browser when opening it :-)

Comment 5

[Paul Adenot (:padenot)]

So, this is cause by the fact that we use an OfflineAudioContext at 44100Hz. If I modify the test case to use 48000Hz, it works just fine.

In fact, I'm sure sure if it is actually allowed for now, OfflineAudioContext should throw if the rate is not 48000Hz. Maybe this is just a matter of throwing the assertion for now.

Comment 6

[(no longer active)]

Oh, right.  So we talked about this at one of the Audio WG teleconfs.  We should make the IDL return value for createMediaStreamDestination() return nullable, and return null if called on an OAC with a "non-standard" sampling rate.

Can you please post on public-audio to remind crogers to spec this?  Thanks!

Comment 7

[Paul Adenot (:padenot)]

Maybe I can needinfo myself as a reminder?

Comment 8

[Daniel Veditz [:dveditz]]

If this is a spec error does that mean it's not a vulnerability? Or do we need to change our code to protect against this illegal value?(In reply to 

Paul Adenot (:padenot) from comment #4)
> Setting as text plain so I don't crash my browser when opening it :-)

Get the bugzilla Tweaks addon which adds a "source" link next to attachments. That way people can use the attachment as a testcase in place, but you can also easily open it for inspection.  (also the Details link will open HTML safely but you don't get as much real estate to view it.)

Comment 9

[Paul Adenot (:padenot)]

I need to investigate more, there are fishy things here, like use-after-free of messages. Seems to be related to some kind of race because when I put printfs to log, the use-after-free went away.

What we could do is to throw for now when the user specifies a sample rate that we don't support, so it gives us time to investigate. This only buys us time.

Comment 10

[(no longer active)]

What does the stack look like here?  This might indeed be specific to OAC.  In that case, we should just throw, and we won't need to worry about this bug beyond that.

Comment 11

[Paul Adenot (:padenot)]

There are a couple different stacks for this testcase, maybe three. I'll post them tomorrow.

Comment 12

[Andrew McCreight [:mccr8]]

needinfoing for the stacks

Comment 13

[Andrew McCreight [:mccr8]]

Marking critical per comment 9.

Comment 14

[Paul Adenot (:padenot)]

Attachment: stack

For some reason, I can't repro the use-after-free and other ASAN errors, but I get this nice assert, which is kind of expected. I think we should throw instead of all this mess.

Comment 15

[Christoph Diehl [:posidron]]

:padenot, based on comment 4 I assumed that you can reproduce the crash.

Please see also comment 3.

Comment 16

[(no longer active)]

(In reply to Paul Adenot (:padenot) from comment #14)
> Created attachment 772059 [details]
> stack
> 
> For some reason, I can't repro the use-after-free and other ASAN errors, but
> I get this nice assert, which is kind of expected. I think we should throw
> instead of all this mess.

Agreed.

Comment 17

[Paul Adenot (:padenot)]

Christoph, I used to be able to repro with at least 3 different stacks, and iirc I have not updated this tree, and I can't repro anymore. Not sure why.

Comment 18

[Paul Adenot (:padenot)]

Attachment: Throw when creating an MediaStreamDestination node on an OfflineAudioContext

Comment 19

[(no longer active)]

Comment on attachment 772084 [details] [diff] [review]
Throw when creating an MediaStreamDestination node on an OfflineAudioContext

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

Can you please post to public-audio to make sure that we spec this?  Thanks!

Comment 20

[Paul Adenot (:padenot)]

I was thinking about that again the other day, and I was wondering if using a MediaStreamDestination node on an OfflineAudioContext would make sense with the new MediaRecorder API? In which case we don't want this patch.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think we should take this patch for now and later figure out our story around MediaStreams and OfflineAudioContext. It's not trivial because we'll need to define some kind of offline MediaStream and that will require spec work.

Comment 22

[Paul Adenot (:padenot)]

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

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4a29a65a3cba

Comment 24

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

How did this land on Aurora before it was checked into trunk? Bugs need explicit aurora approval from release management before landing there and they need to be checked into trunk first.

Also, this bug affects more than one branch so it should have gotten sec-approval before going in.

https://wiki.mozilla.org/Security/Bug_Approval_Process

Comment 25

[(no longer active)]

(In reply to Al Billings [:abillings] from comment #24)
> How did this land on Aurora before it was checked into trunk? Bugs need
> explicit aurora approval from release management before landing there and
> they need to be checked into trunk first.

Web Audio is being developed pretty much in parallel on trunk and Aurora.  This has been discussed on release-drivers, sorry there are no public archives to link to.

> Also, this bug affects more than one branch so it should have gotten
> sec-approval before going in.
> 
> https://wiki.mozilla.org/Security/Bug_Approval_Process

This fix wasn't really a security fix, we just disabled code which used to hit this problem...

Comment 26

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

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #25)

> Web Audio is being developed pretty much in parallel on trunk and Aurora. 
> This has been discussed on release-drivers, sorry there are no public
> archives to link to.

I understand this but, still, fixes that aren't branch only don't normally go into branches before they go into trunk and trunk is green. Aurora fixes don't go in without approval from Release Management. From what you say above, I can't tell if this had Aurora approval from them.

> This fix wasn't really a security fix, we just disabled code which used to
> hit this problem...

Well, the bug is sec-critical here and gets tracked all over as such. Does that mean we should open this bug to the public now?

Comment 27

[Ryan VanderMeulen [:RyanVM]]

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

Comment 28

[(no longer active)]

(In reply to Al Billings [:abillings] from comment #26)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #25)
> 
> > Web Audio is being developed pretty much in parallel on trunk and Aurora. 
> > This has been discussed on release-drivers, sorry there are no public
> > archives to link to.
> 
> I understand this but, still, fixes that aren't branch only don't normally
> go into branches before they go into trunk and trunk is green.

There are sometimes good reasons to wait (such as when you're not sure if the patch is the right fix, or where you're afraid of regressions, etc.)  In other cases that is just unnecessary formality which slows people down.

> Aurora fixes
> don't go in without approval from Release Management. From what you say
> above, I can't tell if this had Aurora approval from them.

Sorry for the confusion, these patches are pre-approved.

> > This fix wasn't really a security fix, we just disabled code which used to
> > hit this problem...
> 
> Well, the bug is sec-critical here and gets tracked all over as such. Does
> that mean we should open this bug to the public now?

Yes, I don't see why we can't do that.

Comment 29

[Daniel Veditz [:dveditz]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #25)
> This fix wasn't really a security fix, we just disabled code which used to
> hit this problem...

... the problem which was almost certainly exploitable (e.g. bug 883938)? Sounds like a "security fix" to me.

But sure, since the fix is now available on every branch that has the feature we don't need to keep the bug hidden.

Comment 30

[Paul Adenot (:padenot)]

This bug had its patch landed on every needed branch, closing.