Closed Bug 897981 Opened 6 years ago Closed 6 years ago

Missing lock in ReceivedRTPPacket/ReceivedRTCPPacket in upstream webrtc.org code

Categories

(Core :: WebRTC, defect)

22 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: jesup, Assigned: jesup)

References

(Depends on 1 open bug)

Details

Attachments

(1 file)

vie_channel.cc locks most RTP/RTCP operations with the rtp_rtcp_cs critical section, as they can get created and stopped (and modified) on different threads, and different threads than data arrives on.  However, ReceivedRTPPacket/ReceivedRTCPPacket don't, and when these filter down to the same-named calls in the ViEReceiver class they access receiving_ with no lock, which means there's a potential TSAN bug between reads on the packet arrival thread and shutting down on another thread (in our case MainThread).

Note that ViEChannel::StopSend/StopReceive grab the rtp_rtcp_cs lock, and ViEReceiver has it's own receiver_cs, but does not use it for either StopSend/StopReceive or for ReceivedRTPPacket/ReceivedRTCPPacket.  (Note that we touched this code in bug 832579, but that didn't change the problem here.)

This is an upstream bug and an Issue will need to be filed.
we could also fold InsertRTP/RTCPPacket into ReceivedRTP/RTCPPacket since there are now other callers to them, but since they're separate in upstream I've left it that way.  No perf impact as I was able to put the test in an already-existing critical section lock (for incoming packets).
They committed the fix in r4410.
Comment on attachment 781104 [details] [diff] [review]
access ViEReceiver::receiving_/receiving_rtcp_ under lock

Already in upstream (reviewed by mflodman)
Attachment #781104 - Flags: review?(tterribe)
Comment on attachment 781104 [details] [diff] [review]
access ViEReceiver::receiving_/receiving_rtcp_ under lock

In upstream, marking as r=flodman :-)
Attachment #781104 - Flags: review?(tterribe) → review+
https://hg.mozilla.org/mozilla-central/rev/5acaf91e8218
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.