From 3fd31fe5023b299a531ee1ac74234aeba75026dd Mon Sep 17 00:00:00 2001
From: hbos <hbos@webrtc.org>
Date: Tue, 28 Feb 2017 05:43:16 -0800
Subject: [PATCH] Fix TSAN race in webrtc::voe::Channel.

|transport_overhead_per_packet_| and |rtp_overhead_per_packet_| could
be read from and written to on different threads concurrently. This CL
introduces a lock to GUARD these variables.

NOTRY because master.tryserver.webrtc.linux_ubsan_vptr is broken, all
other tests pass.

BUG=webrtc:7231
NOTRY=True

Review-Url: https://codereview.webrtc.org/2710363003
Cr-Commit-Position: refs/heads/master@{#16900}
---
 webrtc/pc/rtcstats_integrationtest.cc | 4 ----
 webrtc/voice_engine/channel.cc        | 8 ++++++--
 webrtc/voice_engine/channel.h         | 8 +++++---
 3 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/webrtc/pc/rtcstats_integrationtest.cc b/webrtc/pc/rtcstats_integrationtest.cc
index 25b31f7dd7..746dc0d6c6 100644
--- a/webrtc/pc/rtcstats_integrationtest.cc
+++ b/webrtc/pc/rtcstats_integrationtest.cc
@@ -616,9 +616,6 @@ class RTCStatsReportVerifier {
   rtc::scoped_refptr<const RTCStatsReport> report_;
 };
 
-// Disabled on Tsan due to:
-// https://bugs.chromium.org/p/webrtc/issues/detail?id=7231
-#if !defined(THREAD_SANITIZER)
 #ifdef HAVE_SCTP
 TEST_F(RTCStatsIntegrationTest, GetStatsFromCaller) {
   StartCall();
@@ -647,7 +644,6 @@ TEST_F(RTCStatsIntegrationTest, GetsStatsWhileDestroyingPeerConnections) {
   EXPECT_TRUE(stats_obtainer->report());
 }
 #endif  // HAVE_SCTP
-#endif  // !defined(THREAD_SANITIZER)
 
 }  // namespace
 
diff --git a/webrtc/voice_engine/channel.cc b/webrtc/voice_engine/channel.cc
index 1d54821a50..11e631512c 100644
--- a/webrtc/voice_engine/channel.cc
+++ b/webrtc/voice_engine/channel.cc
@@ -2779,20 +2779,24 @@ void Channel::SetRtcpRttStats(RtcpRttStats* rtcp_rtt_stats) {
 }
 
 void Channel::UpdateOverheadForEncoder() {
+  size_t overhead_per_packet =
+      transport_overhead_per_packet_ + rtp_overhead_per_packet_;
   audio_coding_->ModifyEncoder([&](std::unique_ptr<AudioEncoder>* encoder) {
     if (*encoder) {
-      (*encoder)->OnReceivedOverhead(transport_overhead_per_packet_ +
-                                     rtp_overhead_per_packet_);
+      (*encoder)->OnReceivedOverhead(overhead_per_packet);
     }
   });
 }
 
 void Channel::SetTransportOverhead(size_t transport_overhead_per_packet) {
+  rtc::CritScope cs(&overhead_per_packet_lock_);
   transport_overhead_per_packet_ = transport_overhead_per_packet;
   UpdateOverheadForEncoder();
 }
 
+// TODO(solenberg): Make AudioSendStream an OverheadObserver instead.
 void Channel::OnOverheadChanged(size_t overhead_bytes_per_packet) {
+  rtc::CritScope cs(&overhead_per_packet_lock_);
   rtp_overhead_per_packet_ = overhead_bytes_per_packet;
   UpdateOverheadForEncoder();
 }
diff --git a/webrtc/voice_engine/channel.h b/webrtc/voice_engine/channel.h
index 2be86be5aa..e4e819c76f 100644
--- a/webrtc/voice_engine/channel.h
+++ b/webrtc/voice_engine/channel.h
@@ -416,7 +416,8 @@ class Channel
                                 RTPExtensionType type,
                                 unsigned char id);
 
-  void UpdateOverheadForEncoder();
+  void UpdateOverheadForEncoder()
+      EXCLUSIVE_LOCKS_REQUIRED(overhead_per_packet_lock_);
 
   int GetRtpTimestampRateHz() const;
   int64_t GetRTT(bool allow_associate_channel) const;
@@ -497,8 +498,9 @@ class Channel
   uint32_t _lastLocalTimeStamp;
   int8_t _lastPayloadType;
   bool _includeAudioLevelIndication;
-  size_t transport_overhead_per_packet_;
-  size_t rtp_overhead_per_packet_;
+  size_t transport_overhead_per_packet_ GUARDED_BY(overhead_per_packet_lock_);
+  size_t rtp_overhead_per_packet_ GUARDED_BY(overhead_per_packet_lock_);
+  rtc::CriticalSection overhead_per_packet_lock_;
   // VoENetwork
   AudioFrame::SpeechType _outputSpeechType;
   // VoEVideoSync