diff options
author | Dale Curtis <dalecurtis@chromium.org> | 2020-01-10 20:56:56 +0000 |
---|---|---|
committer | Michael Brüning <michael.bruning@qt.io> | 2020-03-05 11:27:38 +0000 |
commit | e87caa4598d70afedda68428f15419e40131245e (patch) | |
tree | 923e4d72e209f095e46aba8ccaa08464ac3f42d0 | |
parent | d8724284f471b3d3d6c4cf2246aa8a84d7fbc6c5 (diff) |
[Backport] Security bug 1029865
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=<U+200B>xhwang
(cherry picked from commit 7b100c28d219f682763522ed0a3e30e231c1176b)
Fixed: 1029865
Change-Id: I3cde99102863565c05f9da042f27eaac982bcc28
Reviewed-by: Jüri Valdmann <juri.valdmann@qt.io>
-rw-r--r-- | chromium/content/renderer/media/media_factory.cc | 5 | ||||
-rw-r--r-- | chromium/content/renderer/media/media_factory.h | 3 | ||||
-rw-r--r-- | chromium/media/renderers/default_decoder_factory.cc | 14 | ||||
-rw-r--r-- | 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<media::DecoderFactory> decoder_factory_; + std::unique_ptr<media::DefaultDecoderFactory> decoder_factory_; std::unique_ptr<media::CdmFactory> 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<base::SingleThreadTaskRunner> task_runner, MediaLog* media_log, std::vector<std::unique_ptr<AudioDecoder>>* 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<std::unique_ptr<VideoDecoder>>* video_decoders) { + base::AutoLock auto_lock(shutdown_lock_); + if (is_shutdown_) + return; + #if !defined(OS_ANDROID) video_decoders->push_back( std::make_unique<DecryptingVideoDecoder>(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 <memory> +#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<std::unique_ptr<VideoDecoder>>* 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<DecoderFactory> external_decoder_factory_; + base::Lock shutdown_lock_; + bool is_shutdown_ GUARDED_BY(shutdown_lock_) = false; + std::unique_ptr<DecoderFactory> external_decoder_factory_ + GUARDED_BY(shutdown_lock_); DISALLOW_COPY_AND_ASSIGN(DefaultDecoderFactory); }; |