Closed Bug 1223670 Opened 4 years ago Closed 4 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox42 --- wontfix
firefox43 --- wontfix
firefox44 + fixed
firefox45 + fixed
firefox46 + fixed
firefox-esr38 44+ fixed

People

(Reporter: jruderman, Assigned: karlt, NeedInfo)

References

(Blocks 1 open bug)

Details

(5 keywords, Whiteboard: [adv-main44+][adv-esr38.6+][post-critsmash-triage])

Attachments

(12 files)

364 bytes, text/html
Details
3.10 KB, text/plain
Details
40 bytes, text/x-review-board-request
padenot
: review+
Details
40 bytes, text/x-review-board-request
baku
: review+
Details
40 bytes, text/x-review-board-request
baku
: review+
abillings
: sec-approval+
Details
40 bytes, text/x-review-board-request
baku
: review+
Details
359 bytes, text/html
Details
3.45 KB, text/plain
Details
1000 bytes, patch
Details | Diff | Splinter Review
9.91 KB, patch
Details | Diff | Splinter Review
1.79 KB, patch
Details | Diff | Splinter Review
1.54 KB, patch
Details | Diff | Splinter Review
Attached file testcase
Assertion failure: cycleStackMarker == ps->mCycleMarker, at dom/media/MediaStreamGraph.cpp:506
Attached file stack
Would be good to know if this is a regression.  Need debug builds to reproduce.
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.
Looks like a regression from bug 1073615.
Assignee: nobody → karlt
Blocks: 1073615
No longer blocks: 875414, 1189506
Status: NEW → ASSIGNED
Priority: -- → P1
Blocks: 875414
bug 1223670 assert that connected streams have the same graph r?padenot
Attachment #8695478 - Flags: review?(padenot)
bug 1223670 replace public constructors with fallible factory methods r?baku
Attachment #8695479 - Flags: review?(amarchesini)
bug 1223670 throw not supported when creating a node from a stream with different channel r?baku
Attachment #8695480 - Flags: review?(amarchesini)
bug 1223670 make SetMozAudioChannelType() private because the type will not change after construction r?baku
Attachment #8695481 - Flags: review?(amarchesini)
Group: media-core-security
Attached file ASan stack
More likely to crash if I wait a few seconds before quitting.
Duplicate of this bug: 1225643
Probably better to file new bugs than hide bugs with public patches.
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.
Attachment #8695479 - Flags: review?(amarchesini) → review+
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);
Attachment #8695480 - Flags: review?(amarchesini) → review+
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
Attachment #8695481 - Flags: review?(amarchesini) → review+
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.
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 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.
Attachment #8695478 - Flags: review?(padenot) → review+
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.
Attachment #8695480 - Flags: sec-approval?
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.
Attachment #8695480 - Flags: sec-approval? → sec-approval+
karl - if you need, I can land this the 29th if you'll be away.  Just let me know
Flags: needinfo?(karlt)
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!
Flags: needinfo?(karlt)
Karl, could you fill the uplift requests? Thanks
Flags: needinfo?(karlt)
Whiteboard: [checkin on 12/29]
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
Flags: needinfo?(karlt)
Attachment #8695480 - Flags: approval-mozilla-beta?
Attachment #8695480 - Flags: approval-mozilla-aurora?
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 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.
Attachment #8702706 - Flags: approval-mozilla-esr38?
Attachment #8695480 - Flags: approval-mozilla-beta?
Attachment #8695480 - Flags: approval-mozilla-beta+
Attachment #8695480 - Flags: approval-mozilla-aurora?
Attachment #8695480 - Flags: approval-mozilla-aurora+
Attachment #8702706 - Flags: approval-mozilla-esr38? → approval-mozilla-esr38+
Group: media-core-security → core-security-release
Jesse, can you verify this has been fixed? Thank you.
Flags: needinfo?(jruderman)
Whiteboard: [adv-main44+][adv-esr38.6+]
Whiteboard: [adv-main44+][adv-esr38.6+] → [adv-main44+][adv-esr38.6+][post-critsmash-triage]
Group: core-security-release
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.