Fix a data race in AudioEncoderMutableImpl and derived classes

Before this change, it could happen that a caller would get a pointer
to the encoder_ but not use it before another thread called the
Reconstruct method, changing the pointer. This of course resulted in
bad access crashes. With this change, each use of the pointer acquired
from the encoder() method is protected by the same lock that is
required to update the pointer. Note that this fix is probably too
aggressive, since it also affects the Opus implementation; the crash
has so far only been seen for iSAC.

Also adding a test to trigger the problem. The test did not trigger
the problem deterministically, but out would typically find it in less
than 1000 runs.

BUG=chromium:499468
R=jmarusic@webrtc.org, kwiberg@webrtc.org

Review URL: https://codereview.webrtc.org/1176303004.

Cr-Commit-Position: refs/heads/master@{#9436}

webrtc/modules/audio_coding/codecs/audio_encoder_mutable_impl.h

@@ -12,7 +12,10 @@
 #define WEBRTC_MODULES_AUDIO_CODING_CODECS_AUDIO_ENCODER_MUTABLE_IMPL_H_
 
 #include "webrtc/base/scoped_ptr.h"
+#include "webrtc/base/thread_annotations.h"
 #include "webrtc/modules/audio_coding/codecs/audio_encoder.h"
+#include "webrtc/system_wrappers/interface/critical_section_wrapper.h"
+#include "webrtc/system_wrappers/interface/thread_wrapper.h"
 
 namespace webrtc {
 
@@ -24,7 +27,14 @@ namespace webrtc {
 template <typename T, typename P = AudioEncoderMutable>
 class AudioEncoderMutableImpl : public P {
  public:
-  void Reset() override { Reconstruct(config_); }
+  void Reset() override {
+    typename T::Config config;
+    {
+      CriticalSectionScoped cs(encoder_lock_.get());
+      config = config_;
+    }
+    Reconstruct(config);
+  }
 
   bool SetFec(bool enable) override { return false; }
 
@@ -44,50 +54,74 @@ class AudioEncoderMutableImpl : public P {
                                                   const int16_t* audio,
                                                   size_t max_encoded_bytes,
                                                   uint8_t* encoded) override {
+    CriticalSectionScoped cs(encoder_lock_.get());
     return encoder_->EncodeInternal(rtp_timestamp, audio, max_encoded_bytes,
                                     encoded);
   }
-  int SampleRateHz() const override { return encoder_->SampleRateHz(); }
-  int NumChannels() const override { return encoder_->NumChannels(); }
+  int SampleRateHz() const override {
+    CriticalSectionScoped cs(encoder_lock_.get());
+    return encoder_->SampleRateHz();
+  }
+  int NumChannels() const override {
+    CriticalSectionScoped cs(encoder_lock_.get());
+    return encoder_->NumChannels();
+  }
   size_t MaxEncodedBytes() const override {
+    CriticalSectionScoped cs(encoder_lock_.get());
     return encoder_->MaxEncodedBytes();
   }
   int RtpTimestampRateHz() const override {
+    CriticalSectionScoped cs(encoder_lock_.get());
     return encoder_->RtpTimestampRateHz();
   }
   int Num10MsFramesInNextPacket() const override {
+    CriticalSectionScoped cs(encoder_lock_.get());
     return encoder_->Num10MsFramesInNextPacket();
   }
   int Max10MsFramesInAPacket() const override {
+    CriticalSectionScoped cs(encoder_lock_.get());
     return encoder_->Max10MsFramesInAPacket();
   }
   void SetTargetBitrate(int bits_per_second) override {
+    CriticalSectionScoped cs(encoder_lock_.get());
     encoder_->SetTargetBitrate(bits_per_second);
   }
   void SetProjectedPacketLossRate(double fraction) override {
+    CriticalSectionScoped cs(encoder_lock_.get());
     encoder_->SetProjectedPacketLossRate(fraction);
   }
 
  protected:
-  explicit AudioEncoderMutableImpl(const typename T::Config& config) {
+  explicit AudioEncoderMutableImpl(const typename T::Config& config)
+      : encoder_lock_(CriticalSectionWrapper::CreateCriticalSection()) {
     Reconstruct(config);
   }
 
   bool Reconstruct(const typename T::Config& config) {
     if (!config.IsOk())
       return false;
+    CriticalSectionScoped cs(encoder_lock_.get());
     config_ = config;
     encoder_.reset(new T(config_));
     return true;
   }
 
-  const typename T::Config& config() const { return config_; }
-  T* encoder() { return encoder_.get(); }
-  const T* encoder() const { return encoder_.get(); }
+  typename T::Config config() const {
+    CriticalSectionScoped cs(encoder_lock_.get());
+    return config_;
+  }
+  T* encoder() EXCLUSIVE_LOCKS_REQUIRED(encoder_lock_) {
+    return encoder_.get();
+  }
+  const T* encoder() const EXCLUSIVE_LOCKS_REQUIRED(encoder_lock_) {
+    return encoder_.get();
+  }
+
+  const rtc::scoped_ptr<CriticalSectionWrapper> encoder_lock_;
 
  private:
-  rtc::scoped_ptr<T> encoder_;
-  typename T::Config config_;
+  rtc::scoped_ptr<T> encoder_ GUARDED_BY(encoder_lock_);
+  typename T::Config config_ GUARDED_BY(encoder_lock_);
 };
 
 }  // namespace webrtc

webrtc/modules/audio_coding/codecs/isac/fix/source/audio_encoder_isacfix.cc

@@ -67,6 +67,7 @@ int AudioEncoderDecoderMutableIsacFix::Decode(const uint8_t* encoded,
                                               size_t max_decoded_bytes,
                                               int16_t* decoded,
                                               SpeechType* speech_type) {
+  CriticalSectionScoped cs(encoder_lock_.get());
   return encoder()->Decode(encoded, encoded_len, sample_rate_hz,
                            max_decoded_bytes, decoded, speech_type);
 }
@@ -78,20 +79,24 @@ int AudioEncoderDecoderMutableIsacFix::DecodeRedundant(
     size_t max_decoded_bytes,
     int16_t* decoded,
     SpeechType* speech_type) {
+  CriticalSectionScoped cs(encoder_lock_.get());
   return encoder()->DecodeRedundant(encoded, encoded_len, sample_rate_hz,
                                     max_decoded_bytes, decoded, speech_type);
 }
 
 bool AudioEncoderDecoderMutableIsacFix::HasDecodePlc() const {
+  CriticalSectionScoped cs(encoder_lock_.get());
   return encoder()->HasDecodePlc();
 }
 
 int AudioEncoderDecoderMutableIsacFix::DecodePlc(int num_frames,
                                                  int16_t* decoded) {
+  CriticalSectionScoped cs(encoder_lock_.get());
   return encoder()->DecodePlc(num_frames, decoded);
 }
 
 int AudioEncoderDecoderMutableIsacFix::Init() {
+  CriticalSectionScoped cs(encoder_lock_.get());
   return encoder()->Init();
 }
 
@@ -101,32 +106,38 @@ int AudioEncoderDecoderMutableIsacFix::IncomingPacket(
     uint16_t rtp_sequence_number,
     uint32_t rtp_timestamp,
     uint32_t arrival_timestamp) {
+  CriticalSectionScoped cs(encoder_lock_.get());
   return encoder()->IncomingPacket(payload, payload_len, rtp_sequence_number,
                                    rtp_timestamp, arrival_timestamp);
 }
 
 int AudioEncoderDecoderMutableIsacFix::ErrorCode() {
+  CriticalSectionScoped cs(encoder_lock_.get());
   return encoder()->ErrorCode();
 }
 
 int AudioEncoderDecoderMutableIsacFix::PacketDuration(
     const uint8_t* encoded,
     size_t encoded_len) const {
+  CriticalSectionScoped cs(encoder_lock_.get());
   return encoder()->PacketDuration(encoded, encoded_len);
 }
 
 int AudioEncoderDecoderMutableIsacFix::PacketDurationRedundant(
     const uint8_t* encoded,
     size_t encoded_len) const {
+  CriticalSectionScoped cs(encoder_lock_.get());
   return encoder()->PacketDurationRedundant(encoded, encoded_len);
 }
 
 bool AudioEncoderDecoderMutableIsacFix::PacketHasFec(const uint8_t* encoded,
                                                      size_t encoded_len) const {
+  CriticalSectionScoped cs(encoder_lock_.get());
   return encoder()->PacketHasFec(encoded, encoded_len);
 }
 
 size_t AudioEncoderDecoderMutableIsacFix::Channels() const {
+  CriticalSectionScoped cs(encoder_lock_.get());
   return encoder()->Channels();
 }
 

webrtc/modules/audio_coding/codecs/isac/main/source/audio_encoder_isac.cc

@@ -65,6 +65,7 @@ int AudioEncoderDecoderMutableIsacFloat::Decode(const uint8_t* encoded,
                                                 size_t max_decoded_bytes,
                                                 int16_t* decoded,
                                                 SpeechType* speech_type) {
+  CriticalSectionScoped cs(encoder_lock_.get());
   return encoder()->Decode(encoded, encoded_len, sample_rate_hz,
                            max_decoded_bytes, decoded, speech_type);
 }
@@ -76,20 +77,24 @@ int AudioEncoderDecoderMutableIsacFloat::DecodeRedundant(
     size_t max_decoded_bytes,
     int16_t* decoded,
     SpeechType* speech_type) {
+  CriticalSectionScoped cs(encoder_lock_.get());
   return encoder()->DecodeRedundant(encoded, encoded_len, sample_rate_hz,
                                     max_decoded_bytes, decoded, speech_type);
 }
 
 bool AudioEncoderDecoderMutableIsacFloat::HasDecodePlc() const {
+  CriticalSectionScoped cs(encoder_lock_.get());
   return encoder()->HasDecodePlc();
 }
 
 int AudioEncoderDecoderMutableIsacFloat::DecodePlc(int num_frames,
                                                    int16_t* decoded) {
+  CriticalSectionScoped cs(encoder_lock_.get());
   return encoder()->DecodePlc(num_frames, decoded);
 }
 
 int AudioEncoderDecoderMutableIsacFloat::Init() {
+  CriticalSectionScoped cs(encoder_lock_.get());
   return encoder()->Init();
 }
 
@@ -99,33 +104,39 @@ int AudioEncoderDecoderMutableIsacFloat::IncomingPacket(
     uint16_t rtp_sequence_number,
     uint32_t rtp_timestamp,
     uint32_t arrival_timestamp) {
+  CriticalSectionScoped cs(encoder_lock_.get());
   return encoder()->IncomingPacket(payload, payload_len, rtp_sequence_number,
                                    rtp_timestamp, arrival_timestamp);
 }
 
 int AudioEncoderDecoderMutableIsacFloat::ErrorCode() {
+  CriticalSectionScoped cs(encoder_lock_.get());
   return encoder()->ErrorCode();
 }
 
 int AudioEncoderDecoderMutableIsacFloat::PacketDuration(
     const uint8_t* encoded,
     size_t encoded_len) const {
+  CriticalSectionScoped cs(encoder_lock_.get());
   return encoder()->PacketDuration(encoded, encoded_len);
 }
 
 int AudioEncoderDecoderMutableIsacFloat::PacketDurationRedundant(
     const uint8_t* encoded,
     size_t encoded_len) const {
+  CriticalSectionScoped cs(encoder_lock_.get());
   return encoder()->PacketDurationRedundant(encoded, encoded_len);
 }
 
 bool AudioEncoderDecoderMutableIsacFloat::PacketHasFec(
     const uint8_t* encoded,
     size_t encoded_len) const {
+  CriticalSectionScoped cs(encoder_lock_.get());
   return encoder()->PacketHasFec(encoded, encoded_len);
 }
 
 size_t AudioEncoderDecoderMutableIsacFloat::Channels() const {
+  CriticalSectionScoped cs(encoder_lock_.get());
   return encoder()->Channels();
 }
 

webrtc/modules/audio_coding/codecs/opus/interface/audio_encoder_opus.h

@@ -93,10 +93,17 @@ class AudioEncoderMutableOpus
   bool SetApplication(Application application) override;
   bool SetMaxPlaybackRate(int frequency_hz) override;
   AudioEncoderOpus::ApplicationMode application() const {
+    CriticalSectionScoped cs(encoder_lock_.get());
     return encoder()->application();
   }
-  double packet_loss_rate() const { return encoder()->packet_loss_rate(); }
-  bool dtx_enabled() const { return encoder()->dtx_enabled(); }
+  double packet_loss_rate() const {
+    CriticalSectionScoped cs(encoder_lock_.get());
+    return encoder()->packet_loss_rate();
+  }
+  bool dtx_enabled() const {
+    CriticalSectionScoped cs(encoder_lock_.get());
+    return encoder()->dtx_enabled();
+  }
 };
 
 }  // namespace webrtc

webrtc/modules/audio_coding/main/acm2/audio_coding_module_unittest_oldapi.cc

@@ -17,6 +17,7 @@
 #include "webrtc/base/thread_annotations.h"
 #include "webrtc/modules/audio_coding/codecs/audio_encoder.h"
 #include "webrtc/modules/audio_coding/codecs/g711/include/audio_encoder_pcm.h"
+#include "webrtc/modules/audio_coding/codecs/isac/main/interface/audio_encoder_isac.h"
 #include "webrtc/modules/audio_coding/codecs/mock/mock_audio_encoder.h"
 #include "webrtc/modules/audio_coding/main/acm2/acm_receive_test_oldapi.h"
 #include "webrtc/modules/audio_coding/main/acm2/acm_send_test_oldapi.h"
@@ -699,6 +700,160 @@ TEST_F(AcmIsacMtTestOldApi, DISABLED_ON_IOS(DoTest)) {
   EXPECT_EQ(kEventSignaled, RunTest());
 }
 
+class AcmReRegisterIsacMtTestOldApi : public AudioCodingModuleTestOldApi {
+ protected:
+  static const int kRegisterAfterNumPackets = 5;
+  static const int kNumPackets = 10;
+  static const int kPacketSizeMs = 30;
+  static const int kPacketSizeSamples = kPacketSizeMs * 16;
+
+  AcmReRegisterIsacMtTestOldApi()
+      : AudioCodingModuleTestOldApi(),
+        receive_thread_(
+            ThreadWrapper::CreateThread(CbReceiveThread, this, "receive")),
+        codec_registration_thread_(
+            ThreadWrapper::CreateThread(CbCodecRegistrationThread,
+                                        this,
+                                        "codec_registration")),
+        test_complete_(EventWrapper::Create()),
+        crit_sect_(CriticalSectionWrapper::CreateCriticalSection()),
+        codec_registered_(false),
+        receive_packet_count_(0),
+        next_insert_packet_time_ms_(0),
+        fake_clock_(new SimulatedClock(0)) {
+    AudioEncoderDecoderIsac::Config config;
+    config.payload_type = kPayloadType;
+    isac_encoder_.reset(new AudioEncoderDecoderIsac(config));
+    clock_ = fake_clock_.get();
+  }
+
+  void SetUp() {
+    AudioCodingModuleTestOldApi::SetUp();
+    // Set up input audio source to read from specified file, loop after 5
+    // seconds, and deliver blocks of 10 ms.
+    const std::string input_file_name =
+        webrtc::test::ResourcePath("audio_coding/speech_mono_16kHz", "pcm");
+    audio_loop_.Init(input_file_name, 5 * kSampleRateHz, kNumSamples10ms);
+    RegisterCodec();  // Must be called before the threads start below.
+    StartThreads();
+  }
+
+  void RegisterCodec() override {
+    static_assert(kSampleRateHz == 16000, "test designed for iSAC 16 kHz");
+    AudioCodingModule::Codec("ISAC", &codec_, kSampleRateHz, 1);
+    codec_.pltype = kPayloadType;
+
+    // Register iSAC codec in ACM, effectively unregistering the PCM16B codec
+    // registered in AudioCodingModuleTestOldApi::SetUp();
+    // Only register the decoder for now. The encoder is registered later.
+    ASSERT_EQ(0, acm_->RegisterReceiveCodec(codec_));
+  }
+
+  void StartThreads() {
+    ASSERT_TRUE(receive_thread_->Start());
+    receive_thread_->SetPriority(kRealtimePriority);
+    ASSERT_TRUE(codec_registration_thread_->Start());
+    codec_registration_thread_->SetPriority(kRealtimePriority);
+  }
+
+  void TearDown() {
+    AudioCodingModuleTestOldApi::TearDown();
+    receive_thread_->Stop();
+    codec_registration_thread_->Stop();
+  }
+
+  EventTypeWrapper RunTest() {
+    return test_complete_->Wait(10 * 60 * 1000);  // 10 minutes' timeout.
+  }
+
+  static bool CbReceiveThread(void* context) {
+    return reinterpret_cast<AcmReRegisterIsacMtTestOldApi*>(context)
+        ->CbReceiveImpl();
+  }
+
+  bool CbReceiveImpl() {
+    SleepMs(1);
+    const size_t max_encoded_bytes = isac_encoder_->MaxEncodedBytes();
+    rtc::scoped_ptr<uint8_t[]> encoded(new uint8_t[max_encoded_bytes]);
+    AudioEncoder::EncodedInfo info;
+    {
+      CriticalSectionScoped lock(crit_sect_.get());
+      if (clock_->TimeInMilliseconds() < next_insert_packet_time_ms_) {
+        return true;
+      }
+      next_insert_packet_time_ms_ += kPacketSizeMs;
+      ++receive_packet_count_;
+
+      // Encode new frame.
+      uint32_t input_timestamp = rtp_header_.header.timestamp;
+      while (info.encoded_bytes == 0) {
+        info = isac_encoder_->Encode(
+            input_timestamp, audio_loop_.GetNextBlock(), kNumSamples10ms,
+            max_encoded_bytes, encoded.get());
+        input_timestamp += 160;  // 10 ms at 16 kHz.
+      }
+      EXPECT_EQ(rtp_header_.header.timestamp + kPacketSizeSamples,
+                input_timestamp);
+      EXPECT_EQ(rtp_header_.header.timestamp, info.encoded_timestamp);
+      EXPECT_EQ(rtp_header_.header.payloadType, info.payload_type);
+    }
+    // Now we're not holding the crit sect when calling ACM.
+
+    // Insert into ACM.
+    EXPECT_EQ(0, acm_->IncomingPacket(encoded.get(), info.encoded_bytes,
+                                      rtp_header_));
+
+    // Pull audio.
+    for (int i = 0; i < rtc::CheckedDivExact(kPacketSizeMs, 10); ++i) {
+      AudioFrame audio_frame;
+      EXPECT_EQ(0, acm_->PlayoutData10Ms(-1 /* default output frequency */,
+                                         &audio_frame));
+      fake_clock_->AdvanceTimeMilliseconds(10);
+    }
+    rtp_utility_->Forward(&rtp_header_);
+    return true;
+  }
+
+  static bool CbCodecRegistrationThread(void* context) {
+    return reinterpret_cast<AcmReRegisterIsacMtTestOldApi*>(context)
+        ->CbCodecRegistrationImpl();
+  }
+
+  bool CbCodecRegistrationImpl() {
+    SleepMs(1);
+    if (HasFatalFailure()) {
+      // End the test early if a fatal failure (ASSERT_*) has occurred.
+      test_complete_->Set();
+    }
+    CriticalSectionScoped lock(crit_sect_.get());
+    if (!codec_registered_ &&
+        receive_packet_count_ > kRegisterAfterNumPackets) {
+      // Register the iSAC encoder.
+      EXPECT_EQ(0, acm_->RegisterSendCodec(codec_));
+      codec_registered_ = true;
+    }
+    if (codec_registered_ && receive_packet_count_ > kNumPackets) {
+      test_complete_->Set();
+    }
+    return true;
+  }
+
+  rtc::scoped_ptr<ThreadWrapper> receive_thread_;
+  rtc::scoped_ptr<ThreadWrapper> codec_registration_thread_;
+  const rtc::scoped_ptr<EventWrapper> test_complete_;
+  const rtc::scoped_ptr<CriticalSectionWrapper> crit_sect_;
+  bool codec_registered_ GUARDED_BY(crit_sect_);
+  int receive_packet_count_ GUARDED_BY(crit_sect_);
+  int64_t next_insert_packet_time_ms_ GUARDED_BY(crit_sect_);
+  rtc::scoped_ptr<AudioEncoderDecoderIsac> isac_encoder_;
+  rtc::scoped_ptr<SimulatedClock> fake_clock_;
+  test::AudioLoop audio_loop_;
+};
+
+TEST_F(AcmReRegisterIsacMtTestOldApi, DISABLED_ON_IOS(DoTest)) {
+  EXPECT_EQ(kEventSignaled, RunTest());
+}
+
 // Disabling all of these tests on iOS until file support has been added.
 // See https://code.google.com/p/webrtc/issues/detail?id=4752 for details.
 #if !defined(WEBRTC_IOS)

webrtc/modules/modules.gyp

@@ -64,6 +64,7 @@
             'bwe_simulator',
             'cng',
             'desktop_capture',
+            'isac',
             'isac_fix',
             'media_file',
             'neteq',