From 8e24d8777849951ed86fb01e0bf556d4eda65161 Mon Sep 17 00:00:00 2001
From: "stefan@webrtc.org" <stefan@webrtc.org>
Date: Tue, 2 Sep 2014 18:58:24 +0000
Subject: [PATCH] Fix race in Voice Engine's Channel where it accesses
 RemoteNtpTimeEstimator from both the audio playback thread and the network
 thread without locking.

BUG=3681
R=pbos@webrtc.org, xians@webrtc.org

Review URL: https://webrtc-codereview.appspot.com/24379004

git-svn-id: http://webrtc.googlecode.com/svn/trunk@7030 4adac7df-926f-26a2-2b94-8c16560cd09d
---
 webrtc/voice_engine/channel.cc | 29 +++++++++++++++++------------
 webrtc/voice_engine/channel.h  |  5 +++--
 2 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/webrtc/voice_engine/channel.cc b/webrtc/voice_engine/channel.cc
index 3f13ba2732..73d6be93b9 100644
--- a/webrtc/voice_engine/channel.cc
+++ b/webrtc/voice_engine/channel.cc
@@ -16,7 +16,6 @@
 #include "webrtc/modules/audio_processing/include/audio_processing.h"
 #include "webrtc/modules/interface/module_common_types.h"
 #include "webrtc/modules/rtp_rtcp/interface/receive_statistics.h"
-#include "webrtc/modules/rtp_rtcp/interface/remote_ntp_time_estimator.h"
 #include "webrtc/modules/rtp_rtcp/interface/rtp_payload_registry.h"
 #include "webrtc/modules/rtp_rtcp/interface/rtp_receiver.h"
 #include "webrtc/modules/rtp_rtcp/source/rtp_receiver_strategy.h"
@@ -699,15 +698,18 @@ int32_t Channel::GetAudioFrame(int32_t id, AudioFrame& audioFrame)
           (unwrap_timestamp - capture_start_rtp_time_stamp_) /
           (GetPlayoutFrequency() / 1000);
 
-      // Compute ntp time.
-      audioFrame.ntp_time_ms_ = ntp_estimator_->Estimate(audioFrame.timestamp_);
-      // |ntp_time_ms_| won't be valid until at least 2 RTCP SRs are received.
-      if (audioFrame.ntp_time_ms_ > 0) {
-        // Compute |capture_start_ntp_time_ms_| so that
-        // |capture_start_ntp_time_ms_| + |elapsed_time_ms_| == |ntp_time_ms_|
+      {
         CriticalSectionScoped lock(ts_stats_lock_.get());
-        capture_start_ntp_time_ms_ =
-            audioFrame.ntp_time_ms_ - audioFrame.elapsed_time_ms_;
+        // Compute ntp time.
+        audioFrame.ntp_time_ms_ = ntp_estimator_.Estimate(
+            audioFrame.timestamp_);
+        // |ntp_time_ms_| won't be valid until at least 2 RTCP SRs are received.
+        if (audioFrame.ntp_time_ms_ > 0) {
+          // Compute |capture_start_ntp_time_ms_| so that
+          // |capture_start_ntp_time_ms_| + |elapsed_time_ms_| == |ntp_time_ms_|
+          capture_start_ntp_time_ms_ =
+              audioFrame.ntp_time_ms_ - audioFrame.elapsed_time_ms_;
+        }
       }
     }
 
@@ -877,7 +879,7 @@ Channel::Channel(int32_t channelId,
     _outputExternalMediaCallbackPtr(NULL),
     _timeStamp(0), // This is just an offset, RTP module will add it's own random offset
     _sendTelephoneEventPayloadType(106),
-    ntp_estimator_(new RemoteNtpTimeEstimator(Clock::GetRealTimeClock())),
+    ntp_estimator_(Clock::GetRealTimeClock()),
     jitter_buffer_playout_timestamp_(0),
     playout_timestamp_rtp_(0),
     playout_timestamp_rtcp_(0),
@@ -1949,8 +1951,11 @@ int32_t Channel::ReceivedRTCPPacket(const int8_t* data, int32_t length) {
         "Channel::IncomingRTPPacket() RTCP packet is invalid");
   }
 
-  ntp_estimator_->UpdateRtcpTimestamp(rtp_receiver_->SSRC(),
-                                      _rtpRtcpModule.get());
+  {
+    CriticalSectionScoped lock(ts_stats_lock_.get());
+    ntp_estimator_.UpdateRtcpTimestamp(rtp_receiver_->SSRC(),
+                                       _rtpRtcpModule.get());
+  }
   return 0;
 }
 
diff --git a/webrtc/voice_engine/channel.h b/webrtc/voice_engine/channel.h
index a4f7ecd4c1..c1f6ed2c22 100644
--- a/webrtc/voice_engine/channel.h
+++ b/webrtc/voice_engine/channel.h
@@ -17,6 +17,7 @@
 #include "webrtc/modules/audio_conference_mixer/interface/audio_conference_mixer_defines.h"
 #include "webrtc/modules/audio_processing/rms_level.h"
 #include "webrtc/modules/bitrate_controller/include/bitrate_controller.h"
+#include "webrtc/modules/rtp_rtcp/interface/remote_ntp_time_estimator.h"
 #include "webrtc/modules/rtp_rtcp/interface/rtp_header_parser.h"
 #include "webrtc/modules/rtp_rtcp/interface/rtp_rtcp.h"
 #include "webrtc/modules/utility/interface/file_player.h"
@@ -548,7 +549,7 @@ private:
     uint32_t _timeStamp;
     uint8_t _sendTelephoneEventPayloadType;
 
-    scoped_ptr<RemoteNtpTimeEstimator> ntp_estimator_;
+    RemoteNtpTimeEstimator ntp_estimator_ GUARDED_BY(ts_stats_lock_);
 
     // Timestamp of the audio pulled from NetEq.
     uint32_t jitter_buffer_playout_timestamp_;
@@ -566,7 +567,7 @@ private:
     int64_t capture_start_rtp_time_stamp_;
     // The capture ntp time (in local timebase) of the first played out audio
     // frame.
-    int64_t capture_start_ntp_time_ms_;
+    int64_t capture_start_ntp_time_ms_ GUARDED_BY(ts_stats_lock_);
 
     // uses
     Statistics* _engineStatisticsPtr;