1133051 - MediaPipeline releases SrtpFlows on the wrong thread

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Description

[Byron Campen [:bwc]]

While playing with TSAN, I noticed that we're getting data races when MediaPipelines are destroyed, because the SrtpFlow members are being destroyed on main instead of STS. This seems to have been a problem for a long time (the bundle work moved these into another member, but they weren't being deleted correctly before either). We probably need to release the references in MediaPipeline::ShutdownTransport_s, in the same place where we release the TransportFlows.

Comment 1

[Byron Campen [:bwc]]

I should note that the specific race I observed was here, which looks pretty alarming at first glance, but AFAICT we don't actually do anything with the value:

https://dxr.mozilla.org/mozilla-central/source/netwerk/srtp/src/crypto/cipher/aes_icm.c?from=aes_icm.c&case=true#149

Comment 2

[Byron Campen [:bwc]]

Attachment: Clean up SctpFlows on STS

Comment 3

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f0563fe2c1f5

Comment 4

[Eric Rescorla (:ekr)]

I note that this bug seems to be about SRTP but the patch is titled SCTP. I believe that
the bug title is what is correct.

This does appear to be a problem. Are we sure that there is never any time when the
flows are also manipulated on main, e.g., when they are created?

Comment 5

[Byron Campen [:bwc]]

Yes, I seem to have made a typo in the patch description.

I have not carried out any analysis of whether there are other racy accesses on these. I'll try to make some time for this.

Comment 6

[Byron Campen [:bwc]]

It does not look like these are touched anywhere other than STS to me.

Comment 7

[Martin Thomson [:mt:]]

Comment on attachment 8564352 [details] [diff] [review]
Clean up SctpFlows on STS

Review of attachment 8564352 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +105,5 @@
>    disconnect_all();
>    transport_->Detach();
>    rtp_.transport_ = nullptr;
> +  rtp_.send_srtp_ = nullptr;
> +  rtp_.recv_srtp_ = nullptr;

Might be better to move this to a TransportInfo::Detach(). or ::Destroy().

Comment 8

[Byron Campen [:bwc]]

Attachment: Clean up SctpFlows on STS

Incorporate feedback.

Comment 9

[Byron Campen [:bwc]]

Final try push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8bcc60fa9fa

Comment 10

[Byron Campen [:bwc]]

I was unable to spot any other races than the one in comment 1, and that one looked harmless since the value was never used apart from logging (https://dxr.mozilla.org/mozilla-central/search?q=%2Bvar-ref%3Acipher_type_t%3A%3Aref_count). This is probably not worth keeping as security sensitive.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/40cc59381e45