807633 - WebRTC data race with webrtc::RTPSender::SendToNetwork vs. webrtc::RTPSender::Bytes

Status: NEW

(Core :: WebRTC: Networking, defect, P4)

Description

[Christoph Diehl [:posidron]]

Attachment: callstack

media/webrtc/trunk/src/modules/rtp_rtcp/source/rtp_sender.cc:947
  // Update send statistics
  CriticalSectionScoped cs(_sendCritsect);
  Bitrate::Update(bytes_sent);
  _packetsSent++;
  if (bytes_sent > rtpLength) {
    _payloadBytesSent += bytes_sent - rtpLength;
  }

media/webrtc/trunk/src/modules/rtp_rtcp/source/rtp_sender.cc:1007
// number of sent RTP bytes
// dont use critsect to avoid potental deadlock
WebRtc_UWord32
RTPSender::Bytes() const
{
    return _payloadBytesSent;
}


Tested with m-c changeset: 111684:e19e170d2f6d

Comment 1

[Eric Rescorla (:ekr)]

Is RTPSender::Bytes() used anywhere that this race condition would matter? Or is it just a stat.

Comment 2

[Christoph Diehl [:posidron]]

Another concurrent read has been identified in: 

webrtc::RTPSender::Packets
/trunk/src/modules/rtp_rtcp/source/rtp_sender.cc:999

Comment 3

[Eric Rescorla (:ekr)]

Same question.

Comment 4

[Christoph Diehl [:posidron]]

WebRtc_Word32 ModuleRtpRtcpImpl::DataCountersRTP(
    WebRtc_UWord32* bytesSent,
    WebRtc_UWord32* packetsSent,
    WebRtc_UWord32* bytesReceived,
    WebRtc_UWord32* packetsReceived) const {
  WEBRTC_TRACE(kTraceStream, kTraceRtpRtcp, _id, "DataCountersRTP()");

  if (bytesSent) {
    *bytesSent = _rtpSender.Bytes();
  }
  if (packetsSent) {
    *packetsSent = _rtpSender.Packets();
  }
  return _rtpReceiver.DataCounters(bytesReceived, packetsReceived);
}

Comment 5

[Eric Rescorla (:ekr)]

this sure sounds like it's just a statistics interface.

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

See Issue 879 at webrtc.org's Issue tracker

Comment 7

[Bulk Bug Changes for mreavy's org]

Mass change P3->P4 to align with new Mozilla triage process.

Comment 8

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

In the process of migrating remaining bugs to the new severity system, the severity for this bug cannot be automatically determined. Please retriage this bug using the new severity system.