From e87caa4598d70afedda68428f15419e40131245e Mon Sep 17 00:00:00 2001 From: Dale Curtis Date: Fri, 10 Jan 2020 20:56:56 +0000 Subject: [Backport] Security bug 1029865 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Merge M80: "Neuter DefaultDecoderFactory after MediaFactory destruction." Manual backport of patch originally reviewed on https://chromium-review.googlesource.com/c/chromium/src/+/1990200 Since we started allowing asynchronous stop of media::PipelineImpl, we have released the DefaultDecoderFactory on the media thread assuming it was safe to continue usage after ~MediaFactory. It turns out this is not the case for the MojoDecoderFactory used by DefaultDecoderFactory. To fix, we now neuter the DefaultDecoderFactory in ~MediaFactory to prevent decoders from being created. This is done under a lock since the decoder creation methods are called from the media thread and ~MediaFactory happens on the render thread at time of ~RenderFrame. TBR=xhwang (cherry picked from commit 7b100c28d219f682763522ed0a3e30e231c1176b) Fixed: 1029865 Change-Id: I3cde99102863565c05f9da042f27eaac982bcc28 Reviewed-by: Jüri Valdmann --- chromium/content/renderer/media/media_factory.cc | 5 +++++ chromium/content/renderer/media/media_factory.h | 3 ++- chromium/media/renderers/default_decoder_factory.cc | 14 ++++++++++++++ chromium/media/renderers/default_decoder_factory.h | 10 +++++++++- 4 files changed, 30 insertions(+), 2 deletions(-) diff --git a/chromium/content/renderer/media/media_factory.cc b/chromium/content/renderer/media/media_factory.cc index 8228992bfde..bc800288557 100644 --- a/chromium/content/renderer/media/media_factory.cc +++ b/chromium/content/renderer/media/media_factory.cc @@ -169,6 +169,11 @@ MediaFactory::~MediaFactory() { // new tasks using the DecoderFactory will execute, so we don't need to worry // about additional posted tasks from Stop(). if (decoder_factory_) { + // Prevent any new decoders from being created to avoid future access to the + // external decoder factory (MojoDecoderFactory) since it requires access to + // the (about to be destructed) RenderFrame. + decoder_factory_->Shutdown(); + // DeleteSoon() shouldn't ever fail, we should always have a RenderThread at // this time and subsequently a media thread. To fail, the media thread must // be dead/dying (which only happens at ~RenderThreadImpl), in which case diff --git a/chromium/content/renderer/media/media_factory.h b/chromium/content/renderer/media/media_factory.h index 5c3032076be..4ae50195e76 100644 --- a/chromium/content/renderer/media/media_factory.h +++ b/chromium/content/renderer/media/media_factory.h @@ -43,6 +43,7 @@ class LayerTreeSettings; namespace media { class CdmFactory; class DecoderFactory; +class DefaultDecoderFactory; class MediaLog; class MediaObserver; class RemotePlaybackClientWrapper; @@ -173,7 +174,7 @@ class MediaFactory { media::RendererWebMediaPlayerDelegate* media_player_delegate_ = nullptr; // The CDM and decoder factory attached to this frame, lazily initialized. - std::unique_ptr decoder_factory_; + std::unique_ptr decoder_factory_; std::unique_ptr cdm_factory_; // Media resource cache, lazily initialized. diff --git a/chromium/media/renderers/default_decoder_factory.cc b/chromium/media/renderers/default_decoder_factory.cc index 755b06c9c0c..394a64b6f58 100644 --- a/chromium/media/renderers/default_decoder_factory.cc +++ b/chromium/media/renderers/default_decoder_factory.cc @@ -57,6 +57,10 @@ void DefaultDecoderFactory::CreateAudioDecoders( scoped_refptr task_runner, MediaLog* media_log, std::vector>* audio_decoders) { + base::AutoLock auto_lock(shutdown_lock_); + if (is_shutdown_) + return; + #if !defined(OS_ANDROID) // DecryptingAudioDecoder is only needed in External Clear Key testing to // cover the audio decrypt-and-decode path. @@ -84,6 +88,10 @@ void DefaultDecoderFactory::CreateVideoDecoders( const RequestOverlayInfoCB& request_overlay_info_cb, const gfx::ColorSpace& target_color_space, std::vector>* video_decoders) { + base::AutoLock auto_lock(shutdown_lock_); + if (is_shutdown_) + return; + #if !defined(OS_ANDROID) video_decoders->push_back( std::make_unique(task_runner, media_log)); @@ -123,4 +131,10 @@ void DefaultDecoderFactory::CreateVideoDecoders( #endif } +void DefaultDecoderFactory::Shutdown() { + base::AutoLock auto_lock(shutdown_lock_); + external_decoder_factory_.reset(); + is_shutdown_ = true; +} + } // namespace media diff --git a/chromium/media/renderers/default_decoder_factory.h b/chromium/media/renderers/default_decoder_factory.h index 1b1305d3617..cb471dc3fb3 100644 --- a/chromium/media/renderers/default_decoder_factory.h +++ b/chromium/media/renderers/default_decoder_factory.h @@ -7,6 +7,7 @@ #include +#include "base/synchronization/lock.h" #include "media/base/decoder_factory.h" #include "media/base/media_export.h" @@ -33,9 +34,16 @@ class MEDIA_EXPORT DefaultDecoderFactory : public DecoderFactory { const gfx::ColorSpace& target_color_space, std::vector>* video_decoders) final; + // Called from the renderer thread to prevent any more decoders from being + // vended on other threads. + void Shutdown(); + private: - std::unique_ptr external_decoder_factory_; + base::Lock shutdown_lock_; + bool is_shutdown_ GUARDED_BY(shutdown_lock_) = false; + std::unique_ptr external_decoder_factory_ + GUARDED_BY(shutdown_lock_); DISALLOW_COPY_AND_ASSIGN(DefaultDecoderFactory); }; -- cgit v1.2.1