1786502 - Data race in VideoCaptureModule::DeviceInfo's devicechange callback handling

Status: RESOLVED FIXED

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

Description

[Andreas Pehrson [:pehrsons]]

The devicechange callbacks are added/removed on the VideoCapture thread. But the callback itself comes from the (platform specific) capture backend, on a thread of its choice.

On Mac this is main thread.

On Windows the docs for the notifications suggest main thread as well.

On Linux this is a special InotifyEventThread.

Comment 1

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1786502 - Lock access to DeviceInfo devicechange callbacks. r?#webrtc-reviewers!, r?jib!

Comment 2

[Andreas Pehrson [:pehrsons]]

Comment on attachment 9291104 [details]
Bug 1786502 - Lock access to DeviceInfo devicechange callbacks. r?#webrtc-reviewers!, r?jib!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Difficult, since one side of the race is outside the hands of the application (requires a video input device to be added or removed from the OS platform)
• 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?: This code has been sitting for a long time, I expect it to go cleanly.
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely; doesn't affect the threading model or lifetimes.
• Is Android affected?: No

Comment 3

[Andrew McCreight [:mccr8]]

I'll mark this moderate, because it sounds like a difficult-to-exploit race. As such, you don't need sec-approval to land it.

Comment 4

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

Lock access to DeviceInfo devicechange callbacks. r=webrtc-reviewers,jib
https://hg.mozilla.org/integration/autoland/rev/e826dfadfe1264c59d9b13e3c17d6f75a40f5c33
https://hg.mozilla.org/mozilla-central/rev/e826dfadfe12

Comment 5

[Andreas Pehrson [:pehrsons]]

Comment on attachment 9291104 [details]
Bug 1786502 - Lock access to DeviceInfo devicechange callbacks. r?#webrtc-reviewers!, r?jib!

Beta/Release Uplift Approval Request

• User impact if declined: Most obvious is a user-triggered crash when capturing one camera and plugging in or out another. But there's a race behind this so one could imagine worse things than just crashing too.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: I have tested this on mac but it could reproduce on all platforms but android.

1. get camera capture going on https://mozilla.github.io/webrtc-landing/gum_test.html
2. plug in or out another camera

expected: nothing much
actual: crashes (NB this is a race so not a guaranteed crash, repeating step 2 should increase your chances)

• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): No major changes, just adding a Mutex. We don't grab any other locks under this mutex (even in the callback) so no risk of deadlocks.
• String changes made/needed:
• Is Android affected?: No

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Low-risk patch to fix a user-triggered race that tend to manifest itself as a crash.
• User impact if declined: Most obvious is a user-triggered crash when capturing one camera and plugging in or out another. But there's a race behind this so one could imagine worse things than just crashing too.
• Fix Landed on Version: 106
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): No major changes, just adding a Mutex. We don't grab any other locks under this mutex (even in the callback) so no risk of deadlocks.

Comment 6

[Rares Doghi, Desktop QA]

Hi @Andreas I am trying to verify the fix on this issue but I cant get it to crash in the older builds, I only have the Integrated camera on my macbook 11.1 and 1 extra webcam I keep plugging in and out of the USB port but it doesnt crash, I tried it in multiple versions of Nightly and Beta but it wont crash, do I need to have 2 external webcams for this issue to occur ? do I have to wait more than 10 minutes for it to happen while plugging in and out the webcam ? is there any other way to verify the fix here if it doesnt crash on my end ?

Comment 7

[Andreas Pehrson [:pehrsons]]

I recalled getting this to crash with these STR in the past on Mac, but looking at the surrounding code again it looks accidentally safe. I must have been mistaken as to the reason for the crash I saw. Going further on verifying this will be fruitless.

Should be all right to let this ride the trains then, too, though I leave the final judgment to relman.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9291104 [details]
Bug 1786502 - Lock access to DeviceInfo devicechange callbacks. r?#webrtc-reviewers!, r?jib!

Given the low risk, it seems better safe than sorry to me here. Approved for 105.0b8 and 102.3esr.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/3cab427780c7