1671923 - ThreadSanitizer: data race on secondary_sinks_ [@RtpVideoStreamReceiver::RemoveSecondarySink] vs [@RtpVideoStreamReceiver::OnRtpPacket]

Status: RESOLVED FIXED

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

Description

[Byron Campen [:bwc]]

We are accessing |secondary_sinks_| here under the protection of a mutex:

https://searchfox.org/mozilla-central/rev/819be4899a92213abf121b449779ced662f2ce13/third_party/libwebrtc/webrtc/video/rtp_video_stream_receiver.cc#474

However, we are not protecting the access here with that mutex:

https://searchfox.org/mozilla-central/rev/819be4899a92213abf121b449779ced662f2ce13/third_party/libwebrtc/webrtc/video/rtp_video_stream_receiver.cc#386

This seems pretty dangerous.

Comment 1

[Byron Campen [:bwc]]

Attachment: try output

Comment 2

[Byron Campen [:bwc]]

We seem to be locking that mutex everywhere else we use |secondary_sinks_|, so maybe the fix is simply to do that. I suppose there's a chance of causing a deadlock bug though.

Comment 3

[Dan Minor [:dminor]]

Is upstream affected by this?

Comment 4

[Byron Campen [:bwc]]

It looks like the current version of webrtc.org is assuming that all accesses are coming from a single thread (a worker thread inside webrtc.org); this is going to pose a bit of a problem for our update to webrtc.org. Just a heads up.

https://source.chromium.org/chromium/chromium/src/+/master:third_party/webrtc/video/rtp_video_stream_receiver.h;l=377;bpv=1;bpt=1?q=secondary_sinks_&ss=chromium

Comment 5

[Byron Campen [:bwc]]

It looks like we're going to either be forced to make a lot of invasive modifications to webrtc.org, or we're going to have to find a way to play along with their threading model (ie; we're going to need to dispatch our tasks to their threads). How hard is the latter going to be?

Comment 6

[Dan Minor [:dminor]]

That isn't really a simple question to answer, but I'm not sure if we have much choice but to move to their threading model eventually if we want to continue to be able to update from upstream. They've been tightening up threading assertions with each release and we've had to disable them locally, which is obviously not idea. The last time I looked at this (Bug 1499379) a large source of the pain was the way we gather stats, but I'm not sure if that is still the case.

I'm still looking at rebasing our changes on top of the new upstream. Once that is done, I'll start looking into API changes we need to make, which would include making our threading model align better with theirs.

For now, I'd just fix this problem in whatever way is convenient with our current version of libwebrtc.

Comment 7

[Byron Campen [:bwc]]

Attachment: Bug 1671923: Lock to protect secondary_sinks_ r?dminor

Depends on D94155

Comment 8

[Byron Campen [:bwc]]

Try looks ok here.

Comment 9

[Byron Campen [:bwc]]

Marking sec-high because this could cause UAF.

Comment 10

[Byron Campen [:bwc]]

Comment on attachment 9182704 [details]
Bug 1671923: Lock to protect secondary_sinks_ r?dminor

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Triggering this bug would probably be somewhat hard timing-wise, since the window of opportunity is narrow. The patch is very simple, and the sec flaw is pretty obvious given the patch.
• 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?: Probably trivial.
• How likely is this patch to cause regressions; how much testing does it need?: There is a chance that this could result in deadlock bugs, but it looks like this chance is small.

Comment 11

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

Comment on attachment 9182704 [details]
Bug 1671923: Lock to protect secondary_sinks_ r?dminor

Approved to land and uplift

Comment 12

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

https://hg.mozilla.org/integration/autoland/rev/a13c03cd62a535a892ca6571ebff6c9af8c0096f
https://hg.mozilla.org/mozilla-central/rev/a13c03cd62a5

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0d503d6ef26d

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/9d89e0112dab