Add lock guarding AudioSourceCallback to AUHALStream.

There are a lot of crashes happening due to an invalid source, so
it appears clear that AudioUnitStop() isn't always working. This
adds a lock so that we do nothing in this case and attempts to
remove the InputProc callback during Close() to prevent UaF on the
stream itself

Fixed: 737527
Change-Id: I97f3442f105317a6214722455dab497280c07336
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2532775
Commit-Queue: Olga Sharonova <olka@chromium.org>
Reviewed-by: Olga Sharonova <olka@chromium.org>
Auto-Submit: Dale Curtis <dalecurtis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#827775}

media/audio/mac/audio_auhal_mac.cc

@@ -182,6 +182,7 @@ AUHALStream::~AUHALStream() {
   DCHECK(thread_checker_.CalledOnValidThread());
   CHECK(!audio_unit_);
 
+  base::AutoLock al(lock_);
   ReportAndResetStats();
 }
 
@@ -208,6 +209,20 @@ bool AUHALStream::Open() {
 
 void AUHALStream::Close() {
   DCHECK(thread_checker_.CalledOnValidThread());
+
+  if (audio_unit_) {
+    Stop();
+
+    // Clear the render callback to try and prevent any callbacks from coming
+    // in after we've called stop. https://crbug.com/737527.
+    AURenderCallbackStruct callback = {0};
+    auto result = AudioUnitSetProperty(
+        audio_unit_->audio_unit(), kAudioUnitProperty_SetRenderCallback,
+        kAudioUnitScope_Input, AUElement::OUTPUT, &callback, sizeof(callback));
+    OSSTATUS_DLOG_IF(ERROR, result != noErr, result)
+        << "Failed to clear input callback.";
+  }
+
   audio_unit_.reset();
   // Inform the audio manager that we have been closed. This will cause our
   // destruction. Also include the device ID as a signal to the audio manager
@@ -225,6 +240,7 @@ void AUHALStream::Start(AudioSourceCallback* callback) {
   }
 
   if (!stopped_) {
+    base::AutoLock al(lock_);
     CHECK_EQ(source_, callback);
     return;
   }
@@ -243,8 +259,12 @@ void AUHALStream::Start(AudioSourceCallback* callback) {
   }
 
   stopped_ = false;
-  audio_fifo_.reset();
-  source_ = callback;
+
+  {
+    base::AutoLock al(lock_);
+    audio_fifo_.reset();
+    source_ = callback;
+  }
 
   OSStatus result = AudioOutputUnitStart(audio_unit_->audio_unit());
   if (result == noErr)
@@ -268,10 +288,16 @@ void AUHALStream::Stop() {
   OSStatus result = AudioOutputUnitStop(audio_unit_->audio_unit());
   OSSTATUS_DLOG_IF(ERROR, result != noErr, result)
       << "AudioOutputUnitStop() failed.";
-  if (result != noErr)
-    source_->OnError(AudioSourceCallback::ErrorType::kUnknown);
-  ReportAndResetStats();
-  source_ = nullptr;
+
+  {
+    base::AutoLock al(lock_);
+    if (result != noErr)
+      source_->OnError(AudioSourceCallback::ErrorType::kUnknown);
+
+    ReportAndResetStats();
+    source_ = nullptr;
+  }
+
   stopped_ = true;
 }
 
@@ -295,6 +321,13 @@ OSStatus AUHALStream::Render(AudioUnitRenderActionFlags* flags,
   TRACE_EVENT2("audio", "AUHALStream::Render", "input buffer size",
                number_of_frames_, "output buffer size", number_of_frames);
 
+  base::AutoLock al(lock_);
+
+  // There's no documentation on what we should return here, but if we're here
+  // something is wrong so just return an AudioUnit error that looks reasonable.
+  if (!source_)
+    return kAudioUnitErr_Uninitialized;
+
   UpdatePlayoutTimestamp(output_time_stamp);
 
   // If the stream parameters change for any reason, we need to insert a FIFO
@@ -331,6 +364,7 @@ OSStatus AUHALStream::Render(AudioUnitRenderActionFlags* flags,
 }
 
 void AUHALStream::ProvideInput(int frame_delay, AudioBus* dest) {
+  lock_.AssertAcquired();
   DCHECK(source_);
 
   const base::TimeTicks playout_time =
@@ -377,6 +411,8 @@ base::TimeTicks AUHALStream::GetPlayoutTime(
 }
 
 void AUHALStream::UpdatePlayoutTimestamp(const AudioTimeStamp* timestamp) {
+  lock_.AssertAcquired();
+
   if ((timestamp->mFlags & kAudioTimeStampSampleTimeValid) == 0)
     return;
 
@@ -402,6 +438,8 @@ void AUHALStream::UpdatePlayoutTimestamp(const AudioTimeStamp* timestamp) {
 }
 
 void AUHALStream::ReportAndResetStats() {
+  lock_.AssertAcquired();
+
   if (!last_sample_time_)
     return;  // No stats gathered to report.
 

media/audio/mac/audio_auhal_mac.h

@@ -28,6 +28,7 @@
 #include "base/cancelable_callback.h"
 #include "base/compiler_specific.h"
 #include "base/macros.h"
+#include "base/synchronization/lock.h"
 #include "base/threading/thread_checker.h"
 #include "base/time/time.h"
 #include "media/audio/audio_io.h"
@@ -136,6 +137,9 @@ class AUHALStream : public AudioOutputStream {
 
   const AudioParameters params_;
 
+  // We may get some callbacks after AudioUnitStop() has been called.
+  base::Lock lock_;
+
   // Size of audio buffer requested at construction. The actual buffer size
   // is given by |actual_io_buffer_frame_size_| and it can differ from the
   // requested size.
@@ -144,10 +148,10 @@ class AUHALStream : public AudioOutputStream {
   // Stores the number of frames that we actually get callbacks for.
   // This may be different from what we ask for, so we use this for stats in
   // order to understand how often this happens and what are the typical values.
-  size_t number_of_frames_requested_;
+  size_t number_of_frames_requested_ GUARDED_BY(lock_);
 
   // Pointer to the object that will provide the audio samples.
-  AudioSourceCallback* source_;
+  AudioSourceCallback* source_ GUARDED_BY(lock_);
 
   // Holds the stream format details such as bitrate.
   AudioStreamBasicDescription output_format_;
@@ -173,7 +177,7 @@ class AUHALStream : public AudioOutputStream {
 
   // Dynamically allocated FIFO used when CoreAudio asks for unexpected frame
   // sizes.
-  std::unique_ptr<AudioPullFifo> audio_fifo_;
+  std::unique_ptr<AudioPullFifo> audio_fifo_ GUARDED_BY(lock_);
 
   // Current playout time.  Set by Render().
   base::TimeTicks current_playout_time_;
@@ -191,11 +195,11 @@ class AUHALStream : public AudioOutputStream {
   // These variables are only touched on the callback thread and then read
   // in the dtor (when no longer receiving callbacks).
   // NOTE: Float64 and UInt32 types are used for native API compatibility.
-  Float64 last_sample_time_;
-  UInt32 last_number_of_frames_;
-  UInt32 total_lost_frames_;
-  UInt32 largest_glitch_frames_;
-  int glitches_detected_;
+  Float64 last_sample_time_ GUARDED_BY(lock_);
+  UInt32 last_number_of_frames_ GUARDED_BY(lock_);
+  UInt32 total_lost_frames_ GUARDED_BY(lock_);
+  UInt32 largest_glitch_frames_ GUARDED_BY(lock_);
+  int glitches_detected_ GUARDED_BY(lock_);
 
   // Used to defer Start() to workaround http://crbug.com/160920.
   base::CancelableOnceClosure deferred_start_cb_;