From d0c1f5ee1f56c165bdf550c9e3be0d7313587b80 Mon Sep 17 00:00:00 2001 From: Elly Fong-Jones Date: Wed, 18 Jan 2023 22:33:11 +0000 Subject: [PATCH] media: untangle MediaRouterUI lifetimes Currently, MediaRouterUI is owned by MediaItemUIDeviceSelectorView. There is an observer method named "OnControllerInvalidated" which MediaItemUIDeviceSelectorView reacts to by deleting the MediaRouterUI it owns. However, OnControllerInvalidated can actually be called in two different situations: * From MediaRouterUI::TakeMediaRouteStarter(), in which case the MediaRouterUI object is *not* being destroyed, but should be, because it can't be safely used after TakeMediaRouteStarter() ends; * From MediaRouterUI::~MediaRouterUI(), in which case the MediaRouterUI object *is* being destroyed already and should not be. In the second case, only the fact that libc++ nulls out unique_ptr before destroying the pointed-to object saves us from a use-after-free; under libstdc++, we UaF immediately by re-entering the destructor. Even under libc++ though this is still very dangerous, because any observers that happened to be registered after MediaItemUIDeviceSelectorView will be invoked after the destruction of the object they're observing. Right now there are no such other observers, but the fact remains that this interface is basically a UaF timebomb. This change separates "this object is about to be destroyed" (an observable state) from "please destroy this object, it is no longer useful" (a callback that is made to the object's owner) by: 1. Renaming OnControllerInvalidated to OnControllerDestroying, to make it very clear what is happening to the object, and 2. Adding a RegisterDestructor method to CastDialogController, which allows MediaItemUIDeviceSelectorView to pass a callback into MediaRouterUI which MediaRouterUI can use to arrange for its own destruction. This is still a bit tangled and ungainly, but it's safe. A fuller writeup is on the linked bug. Fixed: 1407202 Change-Id: Id9410de1fbf2cb42f13957dde316b7c9259f192f Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4165967 Reviewed-by: Peter Kasting Reviewed-by: Takumi Fujimoto Commit-Queue: Elly Fong-Jones Cr-Commit-Position: refs/heads/main@{#1094110} --- diff --git a/chrome/browser/ui/media_router/cast_dialog_controller.h b/chrome/browser/ui/media_router/cast_dialog_controller.h index 2a8de976..c3c0553 100644 --- a/chrome/browser/ui/media_router/cast_dialog_controller.h +++ b/chrome/browser/ui/media_router/cast_dialog_controller.h @@ -24,10 +24,12 @@ public: virtual ~Observer() = default; - virtual void OnModelUpdated(const CastDialogModel& model) = 0; + virtual void OnModelUpdated(const CastDialogModel& model) {} - // Observer should drop its reference to the controller when this is called. - virtual void OnControllerInvalidated() = 0; + // Notifies observers that the observed object is being destroyed. Observers + // MUST NOT try to destroy the observed object in response - to manage the + // lifetime of a CastDialogController, use RegisterDestructor() below. + virtual void OnControllerDestroying() {} }; virtual ~CastDialogController() = default; @@ -55,6 +57,16 @@ // intended that this API should only be used to transfer ownership to some // new component that will want to start casting on this dialog box's behalf. virtual std::unique_ptr TakeMediaRouteStarter() = 0; + + // Registers a callback for when the CastDialogController has given up + // ownership of its MediaRouteStarter and is no longer safe to use. The + // provided closure must destroy |this| or otherwise ensure it is never used + // again. This method can only be called once. + // + // TODO(https://crbug.com/1408494): It's awkward that CastDialogController has + // a state where it exists but is unsafe to use, and doubly awkward that we + // have to paper over that with this callback. Can that be fixed? + virtual void RegisterDestructor(base::OnceClosure destructor) = 0; }; } // namespace media_router diff --git a/chrome/browser/ui/media_router/media_router_ui.cc b/chrome/browser/ui/media_router/media_router_ui.cc index 1865115f..644d131 100644 --- a/chrome/browser/ui/media_router/media_router_ui.cc +++ b/chrome/browser/ui/media_router/media_router_ui.cc @@ -83,6 +83,9 @@ MediaRouterUI::~MediaRouterUI() { if (media_route_starter_) DetachFromMediaRouteStarter(); + for (CastDialogController::Observer& observer : observers_) { + observer.OnControllerDestroying(); + } } // static @@ -145,9 +148,6 @@ } void MediaRouterUI::DetachFromMediaRouteStarter() { - for (CastDialogController::Observer& observer : observers_) - observer.OnControllerInvalidated(); - media_route_starter()->RemovePresentationRequestSourceObserver(this); media_route_starter()->RemoveMediaSinkWithCastModesObserver(this); } @@ -181,8 +181,16 @@ std::unique_ptr MediaRouterUI::TakeMediaRouteStarter() { DCHECK(media_route_starter_) << "MediaRouteStarter already taken!"; - DetachFromMediaRouteStarter(); - return std::move(media_route_starter_); + auto starter = std::move(media_route_starter_); + if (destructor_) { + std::move(destructor_).Run(); // May destroy `this`. + } + return starter; +} + +void MediaRouterUI::RegisterDestructor(base::OnceClosure destructor) { + DCHECK(!destructor_); + destructor_ = std::move(destructor); } bool MediaRouterUI::CreateRoute(const MediaSink::Id& sink_id, diff --git a/chrome/browser/ui/media_router/media_router_ui.h b/chrome/browser/ui/media_router/media_router_ui.h index 5c2f14e..7afe775 100644 --- a/chrome/browser/ui/media_router/media_router_ui.h +++ b/chrome/browser/ui/media_router/media_router_ui.h @@ -100,8 +100,10 @@ void StopCasting(const std::string& route_id) override; void ClearIssue(const Issue::Id& issue_id) override; // Note that |MediaRouterUI| should not be used after |TakeMediaRouteStarter| - // is called. + // is called. To enforce that, |TakeMediaRouteStarter| calls the destructor + // callback given to |RegisterDestructor| to destroy itself. std::unique_ptr TakeMediaRouteStarter() override; + void RegisterDestructor(base::OnceClosure destructor) override; // Requests a route be created from the source mapped to // |cast_mode|, to the sink given by |sink_id|. @@ -337,6 +339,8 @@ raw_ptr router_; raw_ptr logger_; + base::OnceClosure destructor_; + // NOTE: Weak pointers must be invalidated before all other member variables. // Therefore |weak_factory_| must be placed at the end. base::WeakPtrFactory weak_factory_{this}; diff --git a/chrome/browser/ui/media_router/media_router_ui_unittest.cc b/chrome/browser/ui/media_router/media_router_ui_unittest.cc index 2cc243d1..c33437b 100644 --- a/chrome/browser/ui/media_router/media_router_ui_unittest.cc +++ b/chrome/browser/ui/media_router/media_router_ui_unittest.cc @@ -80,11 +80,11 @@ } MOCK_METHOD1(OnModelUpdated, void(const CastDialogModel& model)); - void OnControllerInvalidated() override { + void OnControllerDestroying() override { controller_ = nullptr; - OnControllerInvalidatedInternal(); + OnControllerDestroyingInternal(); } - MOCK_METHOD0(OnControllerInvalidatedInternal, void()); + MOCK_METHOD0(OnControllerDestroyingInternal, void()); private: raw_ptr controller_ = nullptr; @@ -295,7 +295,7 @@ }))); NotifyUiOnRoutesUpdated({route}); - EXPECT_CALL(observer, OnControllerInvalidatedInternal()); + EXPECT_CALL(observer, OnControllerDestroyingInternal()); ui_.reset(); } diff --git a/chrome/browser/ui/views/global_media_controls/media_item_ui_device_selector_view.cc b/chrome/browser/ui/views/global_media_controls/media_item_ui_device_selector_view.cc index 34dad46..d843bba 100644 --- a/chrome/browser/ui/views/global_media_controls/media_item_ui_device_selector_view.cc +++ b/chrome/browser/ui/views/global_media_controls/media_item_ui_device_selector_view.cc @@ -222,6 +222,11 @@ if (cast_controller) { cast_controller_ = std::move(cast_controller); cast_controller_->AddObserver(this); + cast_controller_->RegisterDestructor( + base::BindOnce(&MediaItemUIDeviceSelectorView::DestroyCastController, + // Unretained is safe: this callback is held by + // cast_controller_, which is owned by this object. + base::Unretained(this))); } } @@ -499,10 +504,6 @@ observer.OnMediaItemUIDeviceSelectorUpdated(device_entry_ui_map_); } -void MediaItemUIDeviceSelectorView::OnControllerInvalidated() { - cast_controller_.reset(); -} - void MediaItemUIDeviceSelectorView::OnDeviceSelected(int tag) { auto it = device_entry_ui_map_.find(tag); DCHECK(it != device_entry_ui_map_.end()); @@ -658,5 +659,9 @@ weak_ptr_factory_.GetWeakPtr())); } +void MediaItemUIDeviceSelectorView::DestroyCastController() { + cast_controller_.reset(); +} + BEGIN_METADATA(MediaItemUIDeviceSelectorView, views::View) END_METADATA diff --git a/chrome/browser/ui/views/global_media_controls/media_item_ui_device_selector_view.h b/chrome/browser/ui/views/global_media_controls/media_item_ui_device_selector_view.h index e950565..222fc20 100644 --- a/chrome/browser/ui/views/global_media_controls/media_item_ui_device_selector_view.h +++ b/chrome/browser/ui/views/global_media_controls/media_item_ui_device_selector_view.h @@ -81,7 +81,6 @@ // media_router::CastDialogController::Observer void OnModelUpdated(const media_router::CastDialogModel& model) override; - void OnControllerInvalidated() override; // MediaItemUIFooterView::Delegate void OnDeviceSelected(int tag) override; @@ -121,6 +120,7 @@ void RecordCastDeviceCount(); DeviceEntryUI* GetDeviceEntryUI(views::View* view) const; void RegisterAudioDeviceCallbacks(); + void DestroyCastController(); bool has_expand_button_been_shown_ = false; bool have_devices_been_shown_ = false; diff --git a/chrome/browser/ui/views/global_media_controls/media_item_ui_device_selector_view_unittest.cc b/chrome/browser/ui/views/global_media_controls/media_item_ui_device_selector_view_unittest.cc index c3bcc6cc..6ae3dde8 100644 --- a/chrome/browser/ui/views/global_media_controls/media_item_ui_device_selector_view_unittest.cc +++ b/chrome/browser/ui/views/global_media_controls/media_item_ui_device_selector_view_unittest.cc @@ -156,6 +156,7 @@ MOCK_METHOD1(ClearIssue, void(const media_router::Issue::Id& issue_id)); MOCK_METHOD0(TakeMediaRouteStarter, std::unique_ptr()); + MOCK_METHOD1(RegisterDestructor, void(base::OnceClosure)); }; } // anonymous namespace diff --git a/chrome/browser/ui/views/media_router/cast_dialog_coordinator_unittest.cc b/chrome/browser/ui/views/media_router/cast_dialog_coordinator_unittest.cc index f6c80d6a..2dedc7e 100644 --- a/chrome/browser/ui/views/media_router/cast_dialog_coordinator_unittest.cc +++ b/chrome/browser/ui/views/media_router/cast_dialog_coordinator_unittest.cc @@ -40,6 +40,7 @@ MOCK_METHOD(void, StopCasting, (const std::string& route_id)); MOCK_METHOD(void, ClearIssue, (const Issue::Id& issue_id)); MOCK_METHOD(std::unique_ptr, TakeMediaRouteStarter, ()); + MOCK_METHOD(void, RegisterDestructor, (base::OnceClosure)); }; class CastDialogCoordinatorTest : public TestWithBrowserView { diff --git a/chrome/browser/ui/views/media_router/cast_dialog_view.cc b/chrome/browser/ui/views/media_router/cast_dialog_view.cc index e3c7dadb..711d081 100644 --- a/chrome/browser/ui/views/media_router/cast_dialog_view.cc +++ b/chrome/browser/ui/views/media_router/cast_dialog_view.cc @@ -125,9 +125,9 @@ observer.OnDialogModelUpdated(this); } -void CastDialogView::OnControllerInvalidated() { +void CastDialogView::OnControllerDestroying() { controller_ = nullptr; - // We don't destroy the dialog here because if the invalidation was caused by + // We don't destroy the dialog here because if the destruction was caused by // activating the toolbar icon in order to close the dialog, then it would // cause the dialog to immediately open again. } diff --git a/chrome/browser/ui/views/media_router/cast_dialog_view.h b/chrome/browser/ui/views/media_router/cast_dialog_view.h index d87fdda..d44d4e0 100644 --- a/chrome/browser/ui/views/media_router/cast_dialog_view.h +++ b/chrome/browser/ui/views/media_router/cast_dialog_view.h @@ -66,7 +66,7 @@ // CastDialogController::Observer: void OnModelUpdated(const CastDialogModel& model) override; - void OnControllerInvalidated() override; + void OnControllerDestroying() override; // views::BubbleDialogDelegateView: void OnPaint(gfx::Canvas* canvas) override; diff --git a/chrome/browser/ui/views/media_router/cast_dialog_view_browsertest.cc b/chrome/browser/ui/views/media_router/cast_dialog_view_browsertest.cc index 1c584120..a7af3c8 100644 --- a/chrome/browser/ui/views/media_router/cast_dialog_view_browsertest.cc +++ b/chrome/browser/ui/views/media_router/cast_dialog_view_browsertest.cc @@ -70,6 +70,7 @@ override { return nullptr; } + void RegisterDestructor(base::OnceClosure destructor) override {} }; } // namespace diff --git a/chrome/browser/ui/views/media_router/cast_dialog_view_unittest.cc b/chrome/browser/ui/views/media_router/cast_dialog_view_unittest.cc index 5326467..988cb07a 100644 --- a/chrome/browser/ui/views/media_router/cast_dialog_view_unittest.cc +++ b/chrome/browser/ui/views/media_router/cast_dialog_view_unittest.cc @@ -91,6 +91,7 @@ MOCK_METHOD1(StopCasting, void(const std::string& route_id)); MOCK_METHOD1(ClearIssue, void(const Issue::Id& issue_id)); MOCK_METHOD0(TakeMediaRouteStarter, std::unique_ptr()); + MOCK_METHOD1(RegisterDestructor, void(base::OnceClosure)); }; class CastDialogViewTest : public ChromeViewsTestBase { diff --git a/chrome/browser/ui/views/media_router/media_router_dialog_controller_views.cc b/chrome/browser/ui/views/media_router/media_router_dialog_controller_views.cc index ad379b2..244d523 100644 --- a/chrome/browser/ui/views/media_router/media_router_dialog_controller_views.cc +++ b/chrome/browser/ui/views/media_router/media_router_dialog_controller_views.cc @@ -51,7 +51,7 @@ std::move(context)); } - ShowGlobalMeidaControlsDialog(std::move(context)); + ShowGlobalMediaControlsDialog(std::move(context)); return true; } @@ -155,9 +155,20 @@ initiator(), std::move(start_presentation_context_)) : MediaRouterUI::CreateWithDefaultMediaSourceAndMirroring( initiator()); + ui_->RegisterDestructor( + base::BindOnce(&MediaRouterDialogControllerViews::DestroyMediaRouterUI, + // Safe to use base::Unretained here: the callback being + // bound is held by the MediaRouterUI we are creating and + // owning, and ownership of |ui_| is never transferred + // away from this object. + base::Unretained(this))); } -void MediaRouterDialogControllerViews::ShowGlobalMeidaControlsDialog( +void MediaRouterDialogControllerViews::DestroyMediaRouterUI() { + ui_.reset(); +} + +void MediaRouterDialogControllerViews::ShowGlobalMediaControlsDialog( std::unique_ptr context) { // Show the WebContents requesting a dialog. initiator()->GetDelegate()->ActivateContents(initiator()); diff --git a/chrome/browser/ui/views/media_router/media_router_dialog_controller_views.h b/chrome/browser/ui/views/media_router/media_router_dialog_controller_views.h index 0a5fdb1..7c97211 100644 --- a/chrome/browser/ui/views/media_router/media_router_dialog_controller_views.h +++ b/chrome/browser/ui/views/media_router/media_router_dialog_controller_views.h @@ -69,13 +69,14 @@ // MediaRouterUIService::Observer: void OnServiceDisabled() override; - // Initializes |ui_|. + // Initializes and destroys |ui_| respectively. void InitializeMediaRouterUI(); + void DestroyMediaRouterUI(); // If there exists a media button, show the GMC dialog anchored to the media // button. Otherwise, show the dialog anchored to the top center of the web // contents. - void ShowGlobalMeidaControlsDialog( + void ShowGlobalMediaControlsDialog( std::unique_ptr context); // Returns the media button from the browser that initiates the request to