1497751 - update driver state before handing control to another thread

Status: RESOLVED FIXED

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

Description

[Karl Tomlinson (:karlt)]

+++ This bug was initially created as a clone of Bug #1496669 +++

This is a suspected cause of bug 1496669, but tracking separately in case this is not the (only) cause.

It is usually not appropriate to modify the driver from a thread which is not currently the one in control of the graph.
https://hg.mozilla.org/releases/mozilla-beta/rev/656a0868f578#l1.104

Comment 1

[Karl Tomlinson (:karlt)]

Attachment: Bug 1497751 update driver state before handing control to another thread r?achronop!

Comment 2

[Karl Tomlinson (:karlt)]

Comment on attachment 9015762 [details]
Bug 1497751 update driver state before handing control to another thread r?achronop!

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: Difficult due to races needing to be won by threads performing more work.

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?: 63

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

Do you have backports for the affected branches?: Yes

If not, how different, hard to create, and risky will they be?: 

How likely is this patch to cause regressions; how much testing does it need?: Unlikely.  There is no new code introduced, only reordering, with no branches on this thread.  It does affect thread scheduling however, and so presents a small risk of triggering latent issues.

Existing automated tests are more thorough than manual testing.

Comment 3

[Daniel Veditz [:dveditz]]

Comment on attachment 9015762 [details]
Bug 1497751 update driver state before handing control to another thread r?achronop!

sec-approval=dveditz

Comment 4

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/22f5c9171d258adadc703aca664bbca476fa441a
https://hg.mozilla.org/mozilla-central/rev/22f5c9171d25

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this ASAP.

Comment 6

[Karl Tomlinson (:karlt)]

Comment on attachment 9015762 [details]
Bug 1497751 update driver state before handing control to another thread r?achronop!

There are many automated tests using this code, but we don't have sufficient control of the scheduler to cause this race to be run in this way.

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1460346

User impact if declined: crash, csectype-race, csectype-uaf, regression, sec-high

Is this code covered by automated tests?: Unknown

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): There is no new code introduced, only reordering, with no branches on this thread.  It does affect thread scheduling however, and so presents a small risk of triggering any latent issues.

String changes made/needed: None.

Comment 7

[Pascal Chevrel:pascalc]

Comment on attachment 9015762 [details]
Bug 1497751 update driver state before handing control to another thread r?achronop!

Uplift approved for 63rc2

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/75787884e0a3