1673526 - Assertion failure: aTrack->GraphImpl() == GraphImpl(), at /builds/worker/checkouts/gecko/dom/media/MediaTrackGraph.cpp:2998

Status: VERIFIED FIXED

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

Description

[Jason Kratzer [:jkratzer]]

Attachment: testcase.html

Testcase found while fuzzing mozilla-central rev b1a74943bc51 (built with --enable-debug).

Assertion failure: aTrack->GraphImpl() == GraphImpl(), at /builds/worker/checkouts/gecko/dom/media/MediaTrackGraph.cpp:2998

==1506437==ERROR: UndefinedBehaviorSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f1fdd8778f2 bp 0x7ffce9342720 sp 0x7ffce93426d0 T1506437)
==1506437==The signal is caused by a WRITE memory access.
==1506437==Hint: address points to the zero page.
    #0 0x7f1fdd8778f2 in mozilla::ProcessedMediaTrack::AllocateInputPort(mozilla::MediaTrack*, unsigned short, unsigned short) /builds/worker/checkouts/gecko/dom/media/MediaTrackGraph.cpp:2998:5
    #1 0x7f1fddd6abff in mozilla::MediaPipelineTransmit::SetTrack(RefPtr<mozilla::dom::MediaStreamTrack>) /builds/worker/checkouts/gecko/dom/media/webrtc/transportbridge/MediaPipeline.cpp:1043:31
    #2 0x7f1fddc6a7b0 in mozilla::TransceiverImpl::UpdateSendTrack(mozilla::dom::MediaStreamTrack*) /builds/worker/checkouts/gecko/dom/media/webrtc/jsapi/TransceiverImpl.cpp:182:29
    #3 0x7f1fddc6a505 in mozilla::PeerConnectionImpl::ReplaceTrackNoRenegotiation(mozilla::TransceiverImpl&, mozilla::dom::MediaStreamTrack*) /builds/worker/checkouts/gecko/dom/media/webrtc/jsapi/PeerConnectionImpl.cpp:1835:30
    #4 0x7f1fdc39a622 in ReplaceTrackNoRenegotiation /builds/worker/checkouts/gecko/dom/media/webrtc/jsapi/PeerConnectionImpl.h:291:10
    #5 0x7f1fdc39a622 in mozilla::dom::PeerConnectionImpl_Binding::replaceTrackNoRenegotiation(JSContext*, JS::Handle<JSObject*>, void*, JSJitMethodCallArgs const&) /builds/worker/workspace/obj-build/dom/bindings/PeerConnectionImplBinding.cpp:588:24
    #6 0x7f1fdd05a78a in bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) /builds/worker/checkouts/gecko/dom/bindings/BindingUtils.cpp:3229:13
    #7 0x7f1fdffea001 in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:506:13
    #8 0x7f1fdffe9718 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:598:12
    #9 0x7f1fdffeb2e3 in InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:663:10
    #10 0x7f1fdffdf083 in CallFromStack /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:667:10
    #11 0x7f1fdffdf083 in Interpret(JSContext*, js::RunState&) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:3336:16
    #12 0x7f1fdffd61f4 in js::RunScript(JSContext*, js::RunState&) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:476:13
    #13 0x7f1fdffe96e9 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:635:13
    #14 0x7f1fdffeb2e3 in InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:663:10
    #15 0x7f1fdffeb51f in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:680:8
    #16 0x7f1fe05d690b in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /builds/worker/checkouts/gecko/js/src/jsapi.cpp:2829:10
    #17 0x7f1fdc472b8e in mozilla::dom::RTCRtpSenderJSImpl::SetTrack(mozilla::dom::MediaStreamTrack*, mozilla::ErrorResult&, JS::Realm*) /builds/worker/workspace/obj-build/dom/bindings/RTCRtpSenderBinding.cpp:3435:8
    #18 0x7f1fdc4e631b in SetTrack /builds/worker/workspace/obj-build/dom/bindings/RTCRtpSenderBinding.cpp:3757:17
    #19 0x7f1fdc4e631b in mozilla::dom::RTCRtpSender_Binding::setTrack(JSContext*, JS::Handle<JSObject*>, void*, JSJitMethodCallArgs const&) /builds/worker/workspace/obj-build/dom/bindings/RTCRtpSenderBinding.cpp:2451:24
    #20 0x7f1fdd05a78a in bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) /builds/worker/checkouts/gecko/dom/bindings/BindingUtils.cpp:3229:13
    #21 0x7f1fdffea001 in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:506:13
    #22 0x7f1fdffe9718 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:598:12
    #23 0x7f1fdffeb2e3 in InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:663:10
    #24 0x7f1fdffdf083 in CallFromStack /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:667:10
    #25 0x7f1fdffdf083 in Interpret(JSContext*, js::RunState&) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:3336:16
    #26 0x7f1fdffd61f4 in js::RunScript(JSContext*, js::RunState&) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:476:13
    #27 0x7f1fdffe96e9 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:635:13
    #28 0x7f1fdffeb2e3 in InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:663:10
    #29 0x7f1fdffeb51f in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:680:8
    #30 0x7f1fe05d690b in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /builds/worker/checkouts/gecko/js/src/jsapi.cpp:2829:10
    #31 0x7f1fdc44ad93 in mozilla::dom::RTCPeerConnectionJSImpl::AddTrack(mozilla::dom::MediaStreamTrack&, nsTArray<mozilla::OwningNonNull<mozilla::DOMMediaStream> > const&, mozilla::ErrorResult&, JS::Realm*) /builds/worker/workspace/obj-build/dom/bindings/RTCPeerConnectionBinding.cpp:7480:8
    #32 0x7f1fdc4cb0ef in AddTrack /builds/worker/workspace/obj-build/dom/bindings/RTCPeerConnectionBinding.cpp:10000:17
    #33 0x7f1fdc4cb0ef in mozilla::dom::RTCPeerConnection_Binding::addTrack(JSContext*, JS::Handle<JSObject*>, void*, JSJitMethodCallArgs const&) /builds/worker/workspace/obj-build/dom/bindings/RTCPeerConnectionBinding.cpp:3592:79
    #34 0x7f1fdd05a78a in bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) /builds/worker/checkouts/gecko/dom/bindings/BindingUtils.cpp:3229:13
    #35 0x7f1fdffea001 in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:506:13
    #36 0x7f1fdffe9718 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:598:12
    #37 0x7f1fdffeb2e3 in InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:663:10
    #38 0x7f1fdffdf083 in CallFromStack /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:667:10
    #39 0x7f1fdffdf083 in Interpret(JSContext*, js::RunState&) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:3336:16
    #40 0x7f1fdffd61f4 in js::RunScript(JSContext*, js::RunState&) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:476:13
    #41 0x7f1fdffe96e9 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:635:13
    #42 0x7f1fdffeb2e3 in InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:663:10
    #43 0x7f1fe09b5365 in js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) /builds/worker/checkouts/gecko/js/src/jit/BaselineIC.cpp:2997:10
    #44 0x123ef89b3582  (<unknown module>)

UndefinedBehaviorSanitizer can not provide additional info.
SUMMARY: UndefinedBehaviorSanitizer: SEGV /builds/worker/checkouts/gecko/dom/media/MediaTrackGraph.cpp:2998:5 in mozilla::ProcessedMediaTrack::AllocateInputPort(mozilla::MediaTrack*, unsigned short, unsigned short)

Comment 1

[Jason Kratzer [:jkratzer]]

Bugmon Analysis:
Verified bug as reproducible on mozilla-central 20201027044126-46a0e993f8bb.
The bug appears to have been introduced in the following build range:

Start: f02f44b9f7a51da2123fdf7e892fe4aa6cbd92b6 (20191212223426)
End: 172a5c2edd6e46ad5863ca043d7c6af2bdd92e20 (20191212225453)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=f02f44b9f7a51da2123fdf7e892fe4aa6cbd92b6&tochange=172a5c2edd6e46ad5863ca043d7c6af2bdd92e20

Comment 2

[Tyson Smith [:tsmith]]

A Pernosco session is available here: https://pernos.co/debug/LqQGv-en1n3BifwswlU3mQ/index.html

Comment 3

[Karl Tomlinson (:karlt)]

While it would be possible to make tracks work cross-frame, we probably just want a safe failure for now.

Comment 4

[Andreas Pehrson [:pehrsons]]

This is a regression from bug 1604746.

In MediaPipelineTransmit, we check whether mDomTrack and aDomTrack are both live and reside in different graphs, and if they are we re-create mSendTrack. Then we set up an input port from aDomTrack to mSendTrack.

mDomTrack and mSendTrack are not set in sync. For instance, replaceTrack(null) will unset mDomTrack but keep mSendTrack around, presumably so it can continue sending silence.

We should in the cross-graph check check the graphs of aDomTrack and mSendTrack. mDomTrack is irrelevant.

Comment 5

[Andreas Pehrson [:pehrsons]]

I filed bug 1680007 to do some followup cleaning of things I've seen during this investigation.

Comment 6

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1673526 - Add crashtests. r?bwc

Comment 7

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1673526 - Simplify send track recreation condition. r?bwc

Comment 8

[Andreas Pehrson [:pehrsons]]

(In reply to Andreas Pehrson [:pehrsons] from comment #4)

This is a regression from bug 1604746.

I should add some nuance here.

This is not a regression from bug 1604746 per se. Bug 1604746 fixed a larger issue of connecting tracks across graphs but missed a corner case that is showing up here.

Comment 9

[Andreas Pehrson [:pehrsons]]

Comment on attachment 9190593 [details]
Bug 1673526 - Simplify send track recreation condition. r?bwc

Security Approval Request

• How easily could an exploit be constructed based on the patch?: From this patch, it would be a serious effort of backtracking code, but plausible. The tests make it obvious how to trigger the assertion failure in debug builds, but I haven't been able to make them cause a UAF in non-debug builds. As they stand they're racy, but likely need more massaging to trigger a UAF.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
• Which older supported branches are affected by this flaw?: all
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: They go clean, modulo a minimal conflict in the crashtest manifest for esr78.
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely. No threading changes; no timing changes.

Comment 10

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Comment on attachment 9190593 [details]
Bug 1673526 - Simplify send track recreation condition. r?bwc

sec-approved to land the patch on Jan 4

Comment 11

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Comment on attachment 9190592 [details]
Bug 1673526 - Add crashtests. r?bwc

sec-approved to land the test after March 1

Comment 12

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/integration/autoland/rev/be75c841d21b

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 14

[Andreas Pehrson [:pehrsons]]

Comment on attachment 9190593 [details]
Bug 1673526 - Simplify send track recreation condition. r?bwc

Beta/Release Uplift Approval Request

• User impact if declined: There's a risk for a UAF.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Simple change only affecting logic.
• String changes made/needed:

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: There's a risk for a UAF.
• Fix Landed on Version: 86
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Simple change only affecting logic.
• String or UUID changes made by this patch:

Comment 15

[Julien Cristau [:jcristau]]

Comment on attachment 9190593 [details]
Bug 1673526 - Simplify send track recreation condition. r?bwc

approved for 85.0b7

Comment 16

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a71de2290d75

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9190593 [details]
Bug 1673526 - Simplify send track recreation condition. r?bwc

Approved for 78.7esr.

Comment 18

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/5f074812437b

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Add crashtests. r=bwc
https://hg.mozilla.org/integration/autoland/rev/6c9eb88f3412a3943cd2e1bbf11b02f937d3becc
https://hg.mozilla.org/mozilla-central/rev/6c9eb88f3412

Comment 20

[Andreas Pehrson [:pehrsons]]

Would be great to get the crashtests patch into beta and ESR78, thanks.

Comment 21

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f0b3e511874e

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/bb9ef6c1c3bd

Comment 23

[Bugmon [:jkratzer for issues]]

Bugmon Analysis:
Verified bug as fixed on rev mozilla-central 20210107163634-81e34ce5a390.
Removing bugmon keyword as no further action possible.
Please review the bug and re-add the keyword for further analysis.