Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
fix: Issue 907211: Heap-use-after-free in viz::HostFrameSinkManager::…
…InvalidateFrameSinkId
- Loading branch information
1 parent
492397b
commit 441a68c
Showing
2 changed files
with
127 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
126 changes: 126 additions & 0 deletions
126
patches/common/chromium/fix_re-entracy_problem_with_invalidateframesinkid.patch
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,126 @@ | ||
| From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 | ||
| From: kylechar <kylechar@chromium.org> | ||
| 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 <sadrul@chromium.org> | ||
| Commit-Queue: Sadrul Chowdhury <sadrul@chromium.org> | ||
| 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 <memory> | ||
| #include <string> | ||
| +#include <unordered_map> | ||
| #include <vector> | ||
|
|
||
| #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<FrameSinkId, FrameSinkData> frame_sink_data_map_; | ||
| + std::unordered_map<FrameSinkId, FrameSinkData, FrameSinkIdHash> | ||
| + frame_sink_data_map_; | ||
|
|
||
| // If |frame_sink_manager_ptr_| connection was lost. | ||
| bool connection_was_lost_ = false; |