From 441a68c19e403c041be1f4016f0594ba7acb83d3 Mon Sep 17 00:00:00 2001 From: Milan Burda Date: Tue, 2 Apr 2019 17:36:04 +0200 Subject: [PATCH] fix: Issue 907211: Heap-use-after-free in viz::HostFrameSinkManager::InvalidateFrameSinkId --- patches/common/chromium/.patches | 1 + ...y_problem_with_invalidateframesinkid.patch | 126 ++++++++++++++++++ 2 files changed, 127 insertions(+) create mode 100644 patches/common/chromium/fix_re-entracy_problem_with_invalidateframesinkid.patch diff --git a/patches/common/chromium/.patches b/patches/common/chromium/.patches index 15dec256bd02d..4e8125b7c23e3 100644 --- a/patches/common/chromium/.patches +++ b/patches/common/chromium/.patches @@ -98,3 +98,4 @@ intersection-observer.patch keyboard-lock-service-impl.patch make_--explicitly-allowed-ports_work_with_networkservice.patch fix_crashes_in_renderframeimpl_onselectpopupmenuitem_s.patch +fix_re-entracy_problem_with_invalidateframesinkid.patch diff --git a/patches/common/chromium/fix_re-entracy_problem_with_invalidateframesinkid.patch b/patches/common/chromium/fix_re-entracy_problem_with_invalidateframesinkid.patch new file mode 100644 index 0000000000000..cd10db467e35f --- /dev/null +++ b/patches/common/chromium/fix_re-entracy_problem_with_invalidateframesinkid.patch @@ -0,0 +1,126 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: kylechar +Date: Mon, 3 Dec 2018 18:17:17 +0000 +Subject: Fix re-entracy problem with InvalidateFrameSinkId(). + +HostFrameSinkManager::InvalidateFrameSinkId() can make a synchronous IPC +to ensure whatever is drawing the platform window is destroyed before +the platform window gets destroyed. However, while waiting for the +synchronous IPC response other synchronous IPCs continue to get +processed. |frame_sink_data_map_| can get mutated in response to a +different synchronous IPC and |data| may no longer point to a valid +memory location when DestroyCompositorFrameSink() returns. + +Change the order so that all modifications to |data| happen before the +synchronous IPC. Also add a comment to clarify that FrameSinkIds +shouldn't be reused after they are invalidated. We don't do this today +and it would cause more re-entrancy problems. + +Also switch |frame_sink_data_map_| to use std::unordered_map instead of +base::flat_map. This map can get large (tens of elements per open tab) +so std::unordered_map is probably a better choice. + +Bug: 907211 +Change-Id: Iea42d1eca290f5b61f215c66da9b9021f671c1e0 +Reviewed-on: https://chromium-review.googlesource.com/c/1352525 +Reviewed-by: Sadrul Chowdhury +Commit-Queue: Sadrul Chowdhury +Cr-Commit-Position: refs/heads/master@{#613160} + +diff --git a/components/viz/host/host_frame_sink_manager.cc b/components/viz/host/host_frame_sink_manager.cc +index 53ccb9f26f8ce6bb472487d4650c0b1a51b9a4c6..bceb1bd9d96e11080928014e2c30674690e28a06 100644 +--- a/components/viz/host/host_frame_sink_manager.cc ++++ b/components/viz/host/host_frame_sink_manager.cc +@@ -79,15 +79,9 @@ void HostFrameSinkManager::InvalidateFrameSinkId( + FrameSinkData& data = frame_sink_data_map_[frame_sink_id]; + DCHECK(data.IsFrameSinkRegistered()); + +- if (data.has_created_compositor_frame_sink && data.is_root) { +- // This synchronous call ensures that the GL context/surface that draw to +- // the platform window (eg. XWindow or HWND) get destroyed before the +- // platform window is destroyed. +- mojo::SyncCallRestrictions::ScopedAllowSyncCall allow_sync_call; +- frame_sink_manager_->DestroyCompositorFrameSink(frame_sink_id); +- } ++ const bool destroy_synchronously = ++ data.has_created_compositor_frame_sink && data.is_root; + +- frame_sink_manager_->InvalidateFrameSinkId(frame_sink_id); + data.has_created_compositor_frame_sink = false; + data.client = nullptr; + +@@ -96,6 +90,21 @@ void HostFrameSinkManager::InvalidateFrameSinkId( + frame_sink_data_map_.erase(frame_sink_id); + + display_hit_test_query_.erase(frame_sink_id); ++ ++ if (destroy_synchronously) { ++ // This synchronous call ensures that the GL context/surface that draw to ++ // the platform window (eg. XWindow or HWND) get destroyed before the ++ // platform window is destroyed. ++ mojo::SyncCallRestrictions::ScopedAllowSyncCall allow_sync_call; ++ frame_sink_manager_->DestroyCompositorFrameSink(frame_sink_id); ++ ++ // Other synchronous IPCs continue to get processed while ++ // DestroyCompositorFrameSink() is happening, so it's possible ++ // HostFrameSinkManager has been mutated. |data| might not be a valid ++ // reference at this point. ++ } ++ ++ frame_sink_manager_->InvalidateFrameSinkId(frame_sink_id); + } + + void HostFrameSinkManager::EnableSynchronizationReporting( +diff --git a/components/viz/host/host_frame_sink_manager.h b/components/viz/host/host_frame_sink_manager.h +index 1b6a6aca6d5cd58400bd0dd6784085f9dfd77ddb..68d0a251b0b60aac77fe04e1e910b51364357ae7 100644 +--- a/components/viz/host/host_frame_sink_manager.h ++++ b/components/viz/host/host_frame_sink_manager.h +@@ -7,6 +7,7 @@ + + #include + #include ++#include + #include + + #include "base/compiler_specific.h" +@@ -72,15 +73,24 @@ class VIZ_HOST_EXPORT HostFrameSinkManager + // Sets a callback to be notified after Viz sent bad message to Viz host. + void SetBadMessageReceivedFromGpuCallback(base::RepeatingClosure callback); + +- // Registers |frame_sink_id| will be used. This must be called before +- // CreateCompositorFrameSink(Support) is called. ++ // Registers |frame_sink_id| so that a client can submit CompositorFrames ++ // using it. This must be called before creating a CompositorFrameSink or ++ // registering FrameSinkId hierarchy. ++ // ++ // When the client is done submitting CompositorFrames to |frame_sink_id| then ++ // InvalidateFrameSink() should be called. + void RegisterFrameSinkId(const FrameSinkId& frame_sink_id, + HostFrameSinkClient* client); + +- // Invalidates |frame_sink_id| which cleans up any dangling temporary +- // references assigned to it. If there is a CompositorFrameSink for +- // |frame_sink_id| then it will be destroyed and the message pipe to the +- // client will be closed. ++ // Invalidates |frame_sink_id| when the client is done submitting ++ // CompositorFrames. If there is a CompositorFrameSink for |frame_sink_id| ++ // then it will be destroyed and the message pipe to the client will be ++ // closed. ++ // ++ // It's expected, but not enforced, that RegisterFrameSinkId() will never be ++ // called for |frame_sink_id| again. This is to avoid problems with re-entrant ++ // code. If the same client wants to submit CompositorFrames later a new ++ // FrameSinkId should be allocated. + void InvalidateFrameSinkId(const FrameSinkId& frame_sink_id); + + // Tells FrameSinkManger to report when a synchronization event completes via +@@ -256,7 +266,8 @@ class VIZ_HOST_EXPORT HostFrameSinkManager + FrameSinkManagerImpl* frame_sink_manager_impl_ = nullptr; + + // Per CompositorFrameSink data. +- base::flat_map frame_sink_data_map_; ++ std::unordered_map ++ frame_sink_data_map_; + + // If |frame_sink_manager_ptr_| connection was lost. + bool connection_was_lost_ = false;