From d3461a768c0b7bd79c90aa8e020a281ab5db3816 Mon Sep 17 00:00:00 2001 From: Dale Curtis Date: Mon, 20 Feb 2023 13:36:56 +0000 Subject: [Backport] CVE-2023-0931: Use after free in Video (1/2) Cherry-pick of patch originally reviewed https://chromium-review.googlesource.com/c/chromium/src/+/4227731: Simplify WebMediaPlayerMSCompositor destruction. The code was only sometimes calling StopUsingProvider() and posted the submitter destruction unnecessarily. Destruction now works the same as in VideoFrameCompositor, where the class itself is responsible for calling StopUsingProvider() during its own destruction. (cherry picked from commit cbd238e85903b7d94910bd2c6362ff9abf9908cc) Fixed: 1407701 Change-Id: Ia649cb5532519468eea34e12745ed9c990580d82 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4195824 Auto-Submit: Dale Curtis Commit-Queue: Tony Herre Cr-Original-Commit-Position: refs/heads/main@{#1098505} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4227731 Owners-Override: Michael Ershov Commit-Queue: Roger Felipe Zanoni da Silva Reviewed-by: Michael Ershov Cr-Commit-Position: refs/branch-heads/5005@{#1435} Cr-Branched-From: 5b4d9450fee01f821b6400e947b3839727643a71-refs/heads/main@{#992738} Reviewed-on: https://codereview.qt-project.org/c/qt/qtwebengine-chromium/+/462767 Reviewed-by: Michal Klocek --- .../modules/mediastream/webmediaplayer_ms.cc | 3 --- .../mediastream/webmediaplayer_ms_compositor.cc | 27 +++------------------- .../mediastream/webmediaplayer_ms_compositor.h | 5 ---- 3 files changed, 3 insertions(+), 32 deletions(-) diff --git a/chromium/third_party/blink/renderer/modules/mediastream/webmediaplayer_ms.cc b/chromium/third_party/blink/renderer/modules/mediastream/webmediaplayer_ms.cc index d73c02e43fb..4d4d7e98871 100644 --- a/chromium/third_party/blink/renderer/modules/mediastream/webmediaplayer_ms.cc +++ b/chromium/third_party/blink/renderer/modules/mediastream/webmediaplayer_ms.cc @@ -403,9 +403,6 @@ WebMediaPlayerMS::~WebMediaPlayerMS() { if (frame_deliverer_) io_task_runner_->DeleteSoon(FROM_HERE, frame_deliverer_.release()); - if (compositor_) - compositor_->StopUsingProvider(); - if (video_frame_provider_) video_frame_provider_->Stop(); diff --git a/chromium/third_party/blink/renderer/modules/mediastream/webmediaplayer_ms_compositor.cc b/chromium/third_party/blink/renderer/modules/mediastream/webmediaplayer_ms_compositor.cc index ccec5c67f5b..7ac02b2b986 100644 --- a/chromium/third_party/blink/renderer/modules/mediastream/webmediaplayer_ms_compositor.cc +++ b/chromium/third_party/blink/renderer/modules/mediastream/webmediaplayer_ms_compositor.cc @@ -236,13 +236,8 @@ WebMediaPlayerMSCompositor::WebMediaPlayerMSCompositor( WebMediaPlayerMSCompositor::~WebMediaPlayerMSCompositor() { // Ensured by destructor traits. DCHECK(video_frame_compositor_task_runner_->BelongsToCurrentThread()); - - if (submitter_) { - video_frame_compositor_task_runner_->DeleteSoon(FROM_HERE, - std::move(submitter_)); - } else { - DCHECK(!video_frame_provider_client_) - << "Must call StopUsingProvider() before dtor!"; + if (video_frame_provider_client_) { + video_frame_provider_client_->StopUsingProvider(); } } @@ -262,7 +257,7 @@ void WebMediaPlayerMSCompositorTraits::Destruct( void WebMediaPlayerMSCompositor::InitializeSubmitter() { DCHECK(video_frame_compositor_task_runner_->BelongsToCurrentThread()); - submitter_->Initialize(this, /* is_media_stream = */ true); + submitter_->Initialize(this, /*is_media_stream=*/true); } void WebMediaPlayerMSCompositor::SetIsSurfaceVisible( @@ -609,15 +604,6 @@ void WebMediaPlayerMSCompositor::ReplaceCurrentFrameWithACopy() { WrapRefCounted(this)))); } -void WebMediaPlayerMSCompositor::StopUsingProvider() { - DCHECK_CALLED_ON_VALID_THREAD(thread_checker_); - PostCrossThreadTask( - *video_frame_compositor_task_runner_, FROM_HERE, - CrossThreadBindOnce( - &WebMediaPlayerMSCompositor::StopUsingProviderInternal, - WrapRefCounted(this))); -} - bool WebMediaPlayerMSCompositor::MapTimestampsToRenderTimeTicks( const std::vector& timestamps, std::vector* wall_clock_times) { @@ -853,13 +839,6 @@ void WebMediaPlayerMSCompositor::StopRenderingInternal() { video_frame_provider_client_->StopRendering(); } -void WebMediaPlayerMSCompositor::StopUsingProviderInternal() { - DCHECK(video_frame_compositor_task_runner_->BelongsToCurrentThread()); - if (video_frame_provider_client_) - video_frame_provider_client_->StopUsingProvider(); - video_frame_provider_client_ = nullptr; -} - void WebMediaPlayerMSCompositor::ReplaceCurrentFrameWithACopyInternal() { DCHECK_CALLED_ON_VALID_THREAD(thread_checker_); scoped_refptr current_frame_ref; diff --git a/chromium/third_party/blink/renderer/modules/mediastream/webmediaplayer_ms_compositor.h b/chromium/third_party/blink/renderer/modules/mediastream/webmediaplayer_ms_compositor.h index 59b1a60b7bc..a2d8761d561 100644 --- a/chromium/third_party/blink/renderer/modules/mediastream/webmediaplayer_ms_compositor.h +++ b/chromium/third_party/blink/renderer/modules/mediastream/webmediaplayer_ms_compositor.h @@ -116,10 +116,6 @@ class MODULES_EXPORT WebMediaPlayerMSCompositor void StopRendering(); void ReplaceCurrentFrameWithACopy(); - // Tell |video_frame_provider_client_| to stop using this instance in - // preparation for dtor. - void StopUsingProvider(); - // Sets a hook to be notified when a new frame is presented, to fulfill a // prending video.requestAnimationFrame() request. // Can be called from any thread. @@ -202,7 +198,6 @@ class MODULES_EXPORT WebMediaPlayerMSCompositor void StartRenderingInternal(); void StopRenderingInternal(); - void StopUsingProviderInternal(); void ReplaceCurrentFrameWithACopyInternal(); void SetAlgorithmEnabledForTesting(bool algorithm_enabled); -- cgit v1.2.1