Closed Bug 1497751 Opened 1 year ago Closed 1 year ago

update driver state before handing control to another thread

Categories

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

63 Branch
All
Unspecified
defect

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
geckoview62 --- unaffected
firefox-esr60 --- unaffected
firefox62 --- unaffected
firefox63 + fixed
firefox64 + fixed

People

(Reporter: karlt, Assigned: karlt)

References

Details

(5 keywords)

Crash Data

Attachments

(1 file)

+++ 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 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.
Attachment #9015762 - Flags: sec-approval?
Comment on attachment 9015762 [details]
Bug 1497751 update driver state before handing control to another thread r?achronop!

sec-approval=dveditz
Attachment #9015762 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/integration/mozilla-inbound/rev/22f5c9171d258adadc703aca664bbca476fa441a
https://hg.mozilla.org/mozilla-central/rev/22f5c9171d25
Group: media-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Please request Beta approval on this ASAP.
Flags: needinfo?(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.
Flags: needinfo?(karlt)
Attachment #9015762 - Flags: approval-mozilla-beta?
Comment on attachment 9015762 [details]
Bug 1497751 update driver state before handing control to another thread r?achronop!

Uplift approved for 63rc2
Attachment #9015762 - Flags: approval-mozilla-beta? → approval-mozilla-release+
No longer blocks: 1496669
Duplicate of this bug: 1496669
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.