1223670 - "Assertion failure: cycleStackMarker == ps->mCycleMarker"

Status: RESOLVED FIXED

(Core :: Audio/Video: MediaStreamGraph, defect, P1)

Description

[Jesse Ruderman]

Attachment: testcase

Assertion failure: cycleStackMarker == ps->mCycleMarker, at dom/media/MediaStreamGraph.cpp:506

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Karl Tomlinson (:karlt)]

Would be good to know if this is a regression.  Need debug builds to reproduce.

Comment 3

[Paul Adenot (:padenot)]

I think this is related to the fact that an AudioChannel is passed. I thought I had taken measures so that it does not happen anymore, but something must have changed, or my code must have had a hole.

From the test case, there is a possibility that we connect a MediaStream from an MSG to a MediaStream from another MSG. pehrsons changed things recently in the area (or maybe it has not landed yet?), that could explain it.

Comment 4

[Karl Tomlinson (:karlt)]

Triggered by https://hg.mozilla.org/mozilla-central/rev/2624ea64e7ab

Comment 5

[Karl Tomlinson (:karlt)]

Looks like a regression from bug 1073615.

Comment 6

[Karl Tomlinson (:karlt)]

Attachment: MozReview Request: bug 1223670 assert that connected streams have the same graph r?padenot

bug 1223670 assert that connected streams have the same graph r?padenot

Comment 7

[Karl Tomlinson (:karlt)]

Attachment: MozReview Request: bug 1223670 replace public constructors with fallible factory methods r?baku

bug 1223670 replace public constructors with fallible factory methods r?baku

Comment 8

[Karl Tomlinson (:karlt)]

Attachment: MozReview Request: bug 1223670 throw not supported when creating a node from a stream with different channel r?baku

bug 1223670 throw not supported when creating a node from a stream with different channel r?baku

Comment 9

[Karl Tomlinson (:karlt)]

Attachment: MozReview Request: bug 1223670 make SetMozAudioChannelType() private because the type will not change after construction r?baku

bug 1223670 make SetMozAudioChannelType() private because the type will not change after construction r?baku

Comment 10

[Jesse Ruderman]

Attachment: testcase variant that sometimes crashes under ASan

Comment 11

[Jesse Ruderman]

Attachment: ASan stack

More likely to crash if I wait a few seconds before quitting.

Comment 13

[Karl Tomlinson (:karlt)]

Probably better to file new bugs than hide bugs with public patches.

Comment 14

[Andrea Marchesini [:baku]]

Comment on attachment 8695479 [details]
MozReview Request: bug 1223670 replace public constructors with fallible factory methods r?baku

https://reviewboard.mozilla.org/r/27121/#review24577

::: dom/media/webaudio/MediaElementAudioSourceNode.cpp:25
(Diff revision 1)
> +  node->Init(aStream, aRv);

if (NS_WARN_IF(aRv.Failed())) {
  return nullptr;
}

return node.forget();

::: dom/media/webaudio/MediaStreamAudioSourceNode.cpp:49
(Diff revision 1)
> +  node->Init(aStream, aRv);

same here.

Comment 15

[Andrea Marchesini [:baku]]

Comment on attachment 8695480 [details]
MozReview Request: bug 1223670 throw not supported when creating a node from a stream with different channel r?baku

https://reviewboard.mozilla.org/r/27123/#review24579

::: dom/media/webaudio/MediaStreamAudioSourceNode.cpp:55
(Diff revision 1)
>  {

MOZ_ASSERT(aMediaStream);

Comment 16

[Andrea Marchesini [:baku]]

Comment on attachment 8695481 [details]
MozReview Request: bug 1223670 make SetMozAudioChannelType() private because the type will not change after construction r?baku

https://reviewboard.mozilla.org/r/27125/#review24581

Comment 17

[Karl Tomlinson (:karlt)]

https://reviewboard.mozilla.org/r/27123/#review24579

> MOZ_ASSERT(aMediaStream);

This will just be noise because the next line would safely and reliably crash anyway if a derived class were to pass a null aMediaStream, but I've added as requested.

Comment 18

[Karl Tomlinson (:karlt)]

https://reviewboard.mozilla.org/r/27121/#review24577

> if (NS_WARN_IF(aRv.Failed())) {
>   return nullptr;
> }
> 
> return node.forget();

I've replaced the ternary with separate return paths.
That skips a temporary, I guess, if the compiler is not able to elide the move
construction.  It also makes setting breaking points on each path easier.

I've added the warning at the source of the failure in
MediaStreamAudioSourceNode::Init(), so as to make the reason for failure clear
without requiring reverse execution.  A message will also be logged when
throwing at the webidl interface.  I'd prefer not to add more spew than that.

Comment 19

[Paul Adenot (:padenot)]

Comment on attachment 8695478 [details]
MozReview Request: bug 1223670 assert that connected streams have the same graph r?padenot

https://reviewboard.mozilla.org/r/27119/#review25317

Karl, sorry for the delay, not sure what happened here.

Comment 20

[Karl Tomlinson (:karlt)]

Comment on attachment 8695480 [details]
MozReview Request: bug 1223670 throw not supported when creating a node from a stream with different channel r?baku

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Not sure.  There are races with how these objects are used but comment 11 implies it is easy to generate a bad pointer, and this bug involves linked objects, some of which have virtual methods.  I haven't studied exactly which methods get called on these objects.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Yes, but the patch was already public before this bug was marked security sensitive.

Which older supported branches are affected by this flaw?
38 and more recent.

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Although touching a number of pieces of code, the patches are simple in concept and so low risk.  I haven't yet applied the patches to older branches, but they should not be hard to back-port.

How likely is this patch to cause regressions; how much testing does it need?
There is existing test coverage for this feature.  It likely doesn't cover every corner case, but covers the core usage.  The biggest risk is if b2g is using it in a way that is already broken.  If this is the case, an exception will be thrown instead of generating unreliable audio.

Comment 21

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

This is approved for checkin on 12/29 (two weeks into the cycle). We'll want this on Aurora, Beta, and ESR38 as well so please nominate patches for those branches.

Comment 22

[Randell Jesup [:jesup] (needinfo me)]

karl - if you need, I can land this the 29th if you'll be away.  Just let me know

Comment 23

[Maire Reavy [:mreavy]]

Randell -- Please land this on the 29th.  I spoke with Anthony last Tuesday, and just about everyone in Auckland is on holiday (it's summer there).  Thanks!

Comment 24

[Karl Tomlinson (:karlt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/164ec87ab3cb
https://hg.mozilla.org/integration/mozilla-inbound/rev/c584499acddc
https://hg.mozilla.org/integration/mozilla-inbound/rev/bdbf2adc3dab
https://hg.mozilla.org/integration/mozilla-inbound/rev/9dbf30a52493

Everyone is on holiday, but not necessarily all at the same times, thanks :)

Comment 25

[Sylvestre Ledru [:Sylvestre]]

Karl, could you fill the uplift requests? Thanks

Comment 26

[Karl Tomlinson (:karlt)]

Comment on attachment 8695480 [details]
MozReview Request: bug 1223670 throw not supported when creating a node from a stream with different channel r?baku

(In reply to Sylvestre Ledru [:sylvestre] from comment #25)
> Karl, could you fill the uplift requests? Thanks

This doesn't need direct-to-branch uplift, and 
https://wiki.mozilla.org/Release_Management/Uplift_rules#Aurora_Uplift_.28approval-mozilla-aurora.29
https://wiki.mozilla.org/Tree_Rules#mozilla-aurora
says "should [...] have landed on mozilla-central to bake on the nightly
channel for a few days" which this has yet not.

However, I'm happy to request prospective approval as it saves me having to
watch for when this reaches central.

Approval Request Comment

This is a request for all patches on this bug that landed on inbound.
They currently transplant to beta without conflict.

The first patch is just an assertion and so is not necessary but helpful to
detect such problems.  I recommend landing, and backing out that patch only if it detects unexpected similar problems.  Bug 1228484 is not yet fixed on these
branches but I don't think we have any tests on these branches to notice that
the assertion will fail, and I'll request approval there anyway.

[Feature/regressing bug #]: bug 1073615
[User impact if declined]:
Effects similar to those described in
https://bugzilla.mozilla.org/show_bug.cgi?id=1179484#c10
[Describe test coverage new/current, TreeHerder]:
There is existing test coverage for this feature.  It likely doesn't cover
every corner case, but covers the core usage.
The new test will not land to provide some security through obscurity.
[Risks and why]: 
The biggest risk is if b2g is using it in a way that is already broken.  If this is the case, an exception will be thrown instead of generating unreliable audio.
[String/UUID change made/needed]:
None

Comment 27

[Karl Tomlinson (:karlt)]

Attachment: esr38: assert that connected streams have the same graph

This assertion fails in the situations of bug 1179484, which is not yet fixed
on esr38, but AFAIK we have no tests triggering the failure there and we
should fix that bug on esr38.

Comment 28

[Karl Tomlinson (:karlt)]

Attachment: esr38: replace public constructors with fallible factory methods

Comment 29

[Karl Tomlinson (:karlt)]

Attachment: esr38: throw not supported when creating a node from a stream with different channel

Comment 30

[Karl Tomlinson (:karlt)]

Attachment: esr38: make SetMozAudioChannelType() private because the type will not change after construction

Comment 31

[Karl Tomlinson (:karlt)]

Comment on attachment 8702706 [details] [diff] [review]
esr38: throw not supported when creating a node from a stream with different channel

[Approval Request Comment]
Please see comment 26.
The difference is that bug 1179484 exists on 38, not bug 1228484.

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/164ec87ab3cb
https://hg.mozilla.org/mozilla-central/rev/c584499acddc
https://hg.mozilla.org/mozilla-central/rev/bdbf2adc3dab
https://hg.mozilla.org/mozilla-central/rev/9dbf30a52493

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1394461357c0
https://hg.mozilla.org/releases/mozilla-aurora/rev/878a28294332
https://hg.mozilla.org/releases/mozilla-aurora/rev/576b11c81977
https://hg.mozilla.org/releases/mozilla-aurora/rev/cd3be172999f

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a3153eb041e6
https://hg.mozilla.org/releases/mozilla-beta/rev/b804fb50d2e3
https://hg.mozilla.org/releases/mozilla-beta/rev/7a0104cded25
https://hg.mozilla.org/releases/mozilla-beta/rev/519bc75be6d2

Comment 35

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-esr38/rev/fc78180165a8
https://hg.mozilla.org/releases/mozilla-esr38/rev/f746c38d160e
https://hg.mozilla.org/releases/mozilla-esr38/rev/6d43ff33bd55
https://hg.mozilla.org/releases/mozilla-esr38/rev/4f6e81673f69

Comment 36

[Matt Wobensmith [:mwobensmith][:matt:]]

Jesse, can you verify this has been fixed? Thank you.

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e95d56bc81c0
crashtest for bug 1223670 r=karlt

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e95d56bc81c0