Fix audio stream creation UAF.

This code assumes that the WebContents owning a RenderFrameHost
outlives the RenderFrameHost, since otherwise RenderFrameHost would
have a dangling |delegate_| pointer. This is apparently false, so
this CL makes sure the RenderFrameAudio{In,Out}putStreamFactory
refers to the ForwardingAudioStreamFactory by a weak pointer.

Test: In addition to CQ, AudioPlayerBrowserTest.ChangeTracks was
repeated 1000 times locally with CrOS/ASAN to ensure it didn't flake.

Bug: 897043
Change-Id: I77925403e95ba8edc7cfaa5db23dc8fe5fd70f93
Reviewed-on: https://chromium-review.googlesource.com/c/1293572Reviewed-by: Olga Sharonova <olka@chromium.org>
Commit-Queue: Olga Sharonova <olka@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601885}

content/browser/media/forwarding_audio_stream_factory.cc

@@ -35,7 +35,8 @@ ForwardingAudioStreamFactory::Core::Core(
       owner_(std::move(owner)),
       broker_factory_(std::move(broker_factory)),
       group_id_(base::UnguessableToken::Create()),
-      connector_(std::move(connector)) {
+      connector_(std::move(connector)),
+      weak_ptr_factory_(this) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(owner_);
   DCHECK(broker_factory_);
@@ -48,6 +49,11 @@ ForwardingAudioStreamFactory::Core::~Core() {
     sink->OnSourceGone();
 }
 
+base::WeakPtr<ForwardingAudioStreamFactory::Core>
+ForwardingAudioStreamFactory::Core::AsWeakPtr() {
+  return weak_ptr_factory_.GetWeakPtr();
+}
+
 void ForwardingAudioStreamFactory::Core::CreateInputStream(
     int render_process_id,
     int render_frame_id,

content/browser/media/forwarding_audio_stream_factory.h

@@ -44,10 +44,18 @@ class CONTENT_EXPORT ForwardingAudioStreamFactory final
     : public WebContentsObserver {
  public:
   // Note: all methods of Core may only be called on the IO thread except for
-  // the constructor. The destruction of Core is posted to the IO thread when
-  // the owning ForwardingAudioStreamFactory is destructed, so any task posted
-  // to the IO thread while the ForwardingAudioStreamFactory is alive may post
-  // |core()| using base::Unretained.
+  // the constructor and group_id(). The destruction of Core is posted to the
+  // IO thread when the owning ForwardingAudioStreamFactory is destructed. For
+  // using |core()|, two rules emerges.
+  // 1) If a task is posted from the UI thread to the IO thread while the
+  //    owning ForwardingAudioStreamFactory is alive, |core()| may be posted
+  //    Unretained.
+  // 2) If |core()| is held until the owning ForwardingAudioStreamFactory could
+  //    potentially be destructed, or if a task is posted to the IO thread with
+  //    the intention of accessing |core| after that task returns, using a raw
+  //    pointer is not safe. In those cases, take a weak pointer using
+  //    AsWeakPtr() and check it for validity before every use. The weak
+  //    pointer may only be checked/dereferenced on the IO thread.
   class CONTENT_EXPORT Core final : public AudioStreamBroker::LoopbackSource {
    public:
     Core(base::WeakPtr<ForwardingAudioStreamFactory> owner,
@@ -63,6 +71,8 @@ class CONTENT_EXPORT ForwardingAudioStreamFactory final
       return connector_.get();
     }
 
+    base::WeakPtr<ForwardingAudioStreamFactory::Core> AsWeakPtr();
+
     // TODO(https://crbug.com/787806): Automatically restore streams on audio
     // service restart.
     void CreateInputStream(
@@ -157,6 +167,9 @@ class CONTENT_EXPORT ForwardingAudioStreamFactory final
     StreamBrokerSet inputs_;
     StreamBrokerSet outputs_;
     base::flat_set<AudioStreamBroker::LoopbackSink*> loopback_sinks_;
+
+    base::WeakPtrFactory<ForwardingAudioStreamFactory::Core> weak_ptr_factory_;
+
     DISALLOW_COPY_AND_ASSIGN(Core);
   };
 

content/browser/renderer_host/media/render_frame_audio_input_stream_factory.cc

@@ -144,7 +144,8 @@ class RenderFrameAudioInputStreamFactory::Core final
   const url::Origin origin_;
 
   mojo::Binding<RendererAudioInputStreamFactory> binding_;
-  ForwardingAudioStreamFactory::Core* forwarding_factory_;
+  // Always null-check this weak pointer before dereferencing it.
+  base::WeakPtr<ForwardingAudioStreamFactory::Core> forwarding_factory_;
 
   base::WeakPtrFactory<Core> weak_ptr_factory_;
 
@@ -181,18 +182,21 @@ RenderFrameAudioInputStreamFactory::Core::Core(
       frame_id_(render_frame_host->GetRoutingID()),
       origin_(render_frame_host->GetLastCommittedOrigin()),
       binding_(this),
-      forwarding_factory_(
-          ForwardingAudioStreamFactory::CoreForFrame(render_frame_host)),
       weak_ptr_factory_(this) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
-  if (!forwarding_factory_) {
-    // The only case when we not have a forwarding factory is when the
-    // frame belongs to an interstitial. Interstitials don't need audio, so it's
-    // fine to drop the request.
+  ForwardingAudioStreamFactory::Core* tmp_factory =
+      ForwardingAudioStreamFactory::CoreForFrame(render_frame_host);
+
+  if (!tmp_factory) {
+    // The only case when we not have a forwarding factory at this point is when
+    // the frame belongs to an interstitial. Interstitials don't need audio, so
+    // it's fine to drop the request.
     return;
   }
 
+  forwarding_factory_ = tmp_factory->AsWeakPtr();
+
   // Unretained is safe since the destruction of |this| is posted to the IO
   // thread.
   base::PostTaskWithTraits(
@@ -218,10 +222,12 @@ void RenderFrameAudioInputStreamFactory::Core::CreateStream(
     uint32_t shared_memory_count,
     audio::mojom::AudioProcessingConfigPtr processing_config) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  DCHECK(forwarding_factory_);
   TRACE_EVENT1("audio", "RenderFrameAudioInputStreamFactory::CreateStream",
                "session id", session_id);
 
+  if (!forwarding_factory_)
+    return;
+
   const MediaStreamDevice* device =
       media_stream_manager_->audio_input_device_manager()->GetOpenedDeviceById(
           session_id);
@@ -274,7 +280,7 @@ void RenderFrameAudioInputStreamFactory::Core::CreateLoopbackStream(
     uint32_t shared_memory_count,
     bool disable_local_echo,
     AudioStreamBroker::LoopbackSource* loopback_source) {
-  if (!loopback_source)
+  if (!loopback_source || !forwarding_factory_)
     return;
 
   forwarding_factory_->CreateLoopbackStream(
@@ -286,7 +292,6 @@ void RenderFrameAudioInputStreamFactory::Core::AssociateInputAndOutputForAec(
     const base::UnguessableToken& input_stream_id,
     const std::string& output_device_id) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  DCHECK(forwarding_factory_);
   if (!IsValidDeviceId(output_device_id))
     return;
 
@@ -307,9 +312,8 @@ void RenderFrameAudioInputStreamFactory::Core::
         MediaDeviceSaltAndOrigin salt_and_origin,
         bool access_granted) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  DCHECK(forwarding_factory_);
 
-  if (!access_granted)
+  if (!forwarding_factory_ || !access_granted)
     return;
 
   if (media::AudioDeviceDescription::IsDefaultDevice(output_device_id) ||
@@ -333,7 +337,8 @@ void RenderFrameAudioInputStreamFactory::Core::
         const base::UnguessableToken& input_stream_id,
         const std::string& raw_output_device_id) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  DCHECK(forwarding_factory_);
+  if (!forwarding_factory_)
+    return;
   forwarding_factory_->AssociateInputAndOutputForAec(input_stream_id,
                                                      raw_output_device_id);
 }

content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc

@@ -76,9 +76,13 @@ class RenderFrameAudioOutputStreamFactory::Core final
                    "RenderFrameAudioOutputStreamFactory::ProviderImpl::Acquire",
                    "raw device id", device_id_);
 
-      owner_->forwarding_factory_->CreateOutputStream(
-          owner_->process_id_, owner_->frame_id_, device_id_, params,
-          processing_id, std::move(provider_client));
+      base::WeakPtr<ForwardingAudioStreamFactory::Core> factory =
+          owner_->forwarding_factory_;
+      if (factory) {
+        factory->CreateOutputStream(owner_->process_id_, owner_->frame_id_,
+                                    device_id_, params, processing_id,
+                                    std::move(provider_client));
+      }
 
       // Since the stream creation has been propagated, |this| is no longer
       // needed.
@@ -127,7 +131,8 @@ class RenderFrameAudioOutputStreamFactory::Core final
   AudioOutputAuthorizationHandler authorization_handler_;
 
   mojo::Binding<mojom::RendererAudioOutputStreamFactory> binding_;
-  ForwardingAudioStreamFactory::Core* forwarding_factory_;
+  // Always null-check this weak pointer before dereferencing it.
+  base::WeakPtr<ForwardingAudioStreamFactory::Core> forwarding_factory_;
 
   // The OutputStreamProviders for authorized streams are kept here while
   // waiting for the renderer to finish creating the stream, and destructed
@@ -179,17 +184,21 @@ RenderFrameAudioOutputStreamFactory::Core::Core(
       frame_id_(frame->GetRoutingID()),
       authorization_handler_(audio_system, media_stream_manager, process_id_),
       binding_(this),
-      forwarding_factory_(ForwardingAudioStreamFactory::CoreForFrame(frame)),
       weak_ptr_factory_(this) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
-  if (!forwarding_factory_) {
-    // The only case when we not have a forwarding factory is when the
-    // frame belongs to an interstitial. Interstitials don't need audio, so it's
-    // fine to drop the request.
+  ForwardingAudioStreamFactory::Core* tmp_factory =
+      ForwardingAudioStreamFactory::CoreForFrame(frame);
+
+  if (!tmp_factory) {
+    // The only case when we not have a forwarding factory at this point is when
+    // the frame belongs to an interstitial. Interstitials don't need audio, so
+    // it's fine to drop the request.
     return;
   }
 
+  forwarding_factory_ = tmp_factory->AsWeakPtr();
+
   // Unretained is safe since the destruction of |this| is posted to the IO
   // thread.
   base::PostTaskWithTraits(
@@ -200,7 +209,6 @@ RenderFrameAudioOutputStreamFactory::Core::Core(
 void RenderFrameAudioOutputStreamFactory::Core::Init(
     mojom::RendererAudioOutputStreamFactoryRequest request) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  DCHECK(forwarding_factory_);
 
   binding_.Bind(std::move(request));
 }
@@ -211,7 +219,6 @@ void RenderFrameAudioOutputStreamFactory::Core::RequestDeviceAuthorization(
     const std::string& device_id,
     RequestDeviceAuthorizationCallback callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  DCHECK(forwarding_factory_);
   TRACE_EVENT2(
       "audio",
       "RenderFrameAudioOutputStreamFactory::RequestDeviceAuthorization",

content/browser/renderer_host/media/render_frame_audio_output_stream_factory.h

@@ -32,13 +32,8 @@ class RenderFrameHost;
 //      FASF::Core           <--          RFAOSF::Core
 //
 // Both FASF::Core and RFAOSF::Core live on (and are destructed on) the IO
-// thread. ForwardingAudioStreamFactory exists until destruction of its owning
-// WebContentsImpl. Since WebContentsImpl outlives all of its
-// RenderFrameHostImpls, ForwardingAudioStreamFactory will outlive
-// RenderFrameAudioOutputStreamFactory. As a consequence, the destruction of
-// FASF::Core will be posted to the IO thread after the destruction of
-// RFAOSF::Core has been posted, so RFAOSF::Core can safely have a raw pointer
-// to FASF::Core
+// thread. A weak pointer to ForwardingAudioStreamFactory is used since
+// WebContentsImpl is sometimes destructed shortly before RenderFrameHostImpl.
 
 // This class takes care of stream requests from a render frame. It verifies
 // that the stream creation is allowed and then forwards the request to the