Browse Source

chore: cherry-pick 94a8bdafc8c6 from chromium (#35237)

* chore: cherry-pick 94a8bdafc8c6 from chromium

* chore: update patches

Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>
Jeremy Rose 2 years ago
parent
commit
fb6de2d52b
2 changed files with 415 additions and 0 deletions
  1. 1 0
      patches/chromium/.patches
  2. 414 0
      patches/chromium/cherry-pick-94a8bdafc8c6.patch

+ 1 - 0
patches/chromium/.patches

@@ -126,3 +126,4 @@ chore_add_electron_deps_to_gitignores.patch
 chore_allow_chromium_to_handle_synthetic_mouse_events_for_touch.patch
 disable_gpu_acceleration_on_vmware_on_linux.patch
 add_maximized_parameter_to_linuxui_getwindowframeprovider.patch
+cherry-pick-94a8bdafc8c6.patch

+ 414 - 0
patches/chromium/cherry-pick-94a8bdafc8c6.patch

@@ -0,0 +1,414 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Keren Zhu <[email protected]>
+Date: Thu, 9 Jun 2022 14:50:26 +0000
+Subject: Fix use-after-free vulnerability in ComboboxModel
+MIME-Version: 1.0
+Content-Type: text/plain; charset=UTF-8
+Content-Transfer-Encoding: 8bit
+
+Stop the Combobox from observing the model when the model gets
+destroyed. This is done by adding a virtual OnComboboxModelDestroying()
+to the base observer class and calling it in ~ComboboxModel().
+
+This fix will prevent the use-after-free that happens when the combobox outlives the model, usually because the model lifetime
+is managed by non-UI component.
+
+Bug: 1264288
+Change-Id: Ia00881a9b674bbc83bbf54dd228490c1cc1290bc
+Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3693825
+Commit-Queue: Keren Zhu <[email protected]>
+Reviewed-by: Matthias Körber <[email protected]>
+Auto-Submit: Keren Zhu <[email protected]>
+Reviewed-by: Peter Boström <[email protected]>
+Cr-Commit-Position: refs/heads/main@{#1012504}
+
+diff --git a/chrome/browser/ui/bookmarks/recently_used_folders_combo_model.cc b/chrome/browser/ui/bookmarks/recently_used_folders_combo_model.cc
+index 323f85651611ff98e45884e3b0b7211eaa25039b..0d0bb0d21b1d96841a30bfea336af918b6da83f8 100644
+--- a/chrome/browser/ui/bookmarks/recently_used_folders_combo_model.cc
++++ b/chrome/browser/ui/bookmarks/recently_used_folders_combo_model.cc
+@@ -130,16 +130,6 @@ int RecentlyUsedFoldersComboModel::GetDefaultIndex() const {
+   return it == items_.end() ? 0 : static_cast<int>(it - items_.begin());
+ }
+ 
+-void RecentlyUsedFoldersComboModel::AddObserver(
+-    ui::ComboboxModelObserver* observer) {
+-  observers_.AddObserver(observer);
+-}
+-
+-void RecentlyUsedFoldersComboModel::RemoveObserver(
+-    ui::ComboboxModelObserver* observer) {
+-  observers_.RemoveObserver(observer);
+-}
+-
+ void RecentlyUsedFoldersComboModel::BookmarkModelLoaded(BookmarkModel* model,
+                                                         bool ids_reassigned) {}
+ 
+@@ -176,7 +166,7 @@ void RecentlyUsedFoldersComboModel::OnWillRemoveBookmarks(
+     }
+   }
+   if (changed) {
+-    for (ui::ComboboxModelObserver& observer : observers_)
++    for (ui::ComboboxModelObserver& observer : observers())
+       observer.OnComboboxModelChanged(this);
+   }
+ }
+@@ -219,7 +209,7 @@ void RecentlyUsedFoldersComboModel::BookmarkAllUserNodesRemoved(
+     }
+   }
+   if (changed) {
+-    for (ui::ComboboxModelObserver& observer : observers_)
++    for (ui::ComboboxModelObserver& observer : observers())
+       observer.OnComboboxModelChanged(this);
+   }
+ }
+diff --git a/chrome/browser/ui/bookmarks/recently_used_folders_combo_model.h b/chrome/browser/ui/bookmarks/recently_used_folders_combo_model.h
+index 8682e750f4407e615264986024e2a7dd0bae038f..0c02c685a02875c7b91ca9e6228864bc04e45c10 100644
+--- a/chrome/browser/ui/bookmarks/recently_used_folders_combo_model.h
++++ b/chrome/browser/ui/bookmarks/recently_used_folders_combo_model.h
+@@ -38,8 +38,6 @@ class RecentlyUsedFoldersComboModel : public ui::ComboboxModel,
+   std::u16string GetItemAt(int index) const override;
+   bool IsItemSeparatorAt(int index) const override;
+   int GetDefaultIndex() const override;
+-  void AddObserver(ui::ComboboxModelObserver* observer) override;
+-  void RemoveObserver(ui::ComboboxModelObserver* observer) override;
+ 
+   // Overriden from bookmarks::BookmarkModelObserver:
+   void BookmarkModelLoaded(bookmarks::BookmarkModel* model,
+@@ -90,8 +88,6 @@ class RecentlyUsedFoldersComboModel : public ui::ComboboxModel,
+   const raw_ptr<bookmarks::BookmarkModel> bookmark_model_;
+ 
+   const raw_ptr<const bookmarks::BookmarkNode> parent_node_;
+-
+-  base::ObserverList<ui::ComboboxModelObserver> observers_;
+ };
+ 
+ #endif  // CHROME_BROWSER_UI_BOOKMARKS_RECENTLY_USED_FOLDERS_COMBO_MODEL_H_
+diff --git a/chrome/browser/ui/bookmarks/recently_used_folders_combo_model_unittest.cc b/chrome/browser/ui/bookmarks/recently_used_folders_combo_model_unittest.cc
+index 60455386b7d5f1da72c92424ccf95fd971e7590a..3bfe6852b8a54a53aafea5eeec2da850097b2f0a 100644
+--- a/chrome/browser/ui/bookmarks/recently_used_folders_combo_model_unittest.cc
++++ b/chrome/browser/ui/bookmarks/recently_used_folders_combo_model_unittest.cc
+@@ -41,6 +41,8 @@ class TestComboboxModelObserver : public ui::ComboboxModelObserver {
+     changed_ = true;
+   }
+ 
++  void OnComboboxModelDestroying(ui::ComboboxModel* model) override {}
++
+  private:
+   bool changed_;
+ };
+diff --git a/components/autofill/core/browser/ui/address_combobox_model.cc b/components/autofill/core/browser/ui/address_combobox_model.cc
+index c7e6f906ff32cb198f8723d6678971ae6cff28a2..d52dc0e925c54634452f86ca90792b318776e895 100644
+--- a/components/autofill/core/browser/ui/address_combobox_model.cc
++++ b/components/autofill/core/browser/ui/address_combobox_model.cc
+@@ -84,14 +84,6 @@ int AddressComboboxModel::GetDefaultIndex() const {
+   return ui::ComboboxModel::GetDefaultIndex();
+ }
+ 
+-void AddressComboboxModel::AddObserver(ui::ComboboxModelObserver* observer) {
+-  observers_.AddObserver(observer);
+-}
+-
+-void AddressComboboxModel::RemoveObserver(ui::ComboboxModelObserver* observer) {
+-  observers_.RemoveObserver(observer);
+-}
+-
+ int AddressComboboxModel::AddNewProfile(const AutofillProfile& profile) {
+   profiles_cache_.push_back(std::make_unique<AutofillProfile>(profile));
+   UpdateAddresses();
+@@ -131,7 +123,7 @@ void AddressComboboxModel::UpdateAddresses() {
+   for (size_t i = 0; i < profiles_cache_.size(); ++i)
+     addresses_.emplace_back(profiles_cache_[i]->guid(), labels[i]);
+ 
+-  for (auto& observer : observers_) {
++  for (auto& observer : observers()) {
+     observer.OnComboboxModelChanged(this);
+   }
+ }
+diff --git a/components/autofill/core/browser/ui/address_combobox_model.h b/components/autofill/core/browser/ui/address_combobox_model.h
+index 02d65209d2729deae608481411e1539ba08f5f7e..3a41b30c92543633da29e40e2c64cc4597e322d2 100644
+--- a/components/autofill/core/browser/ui/address_combobox_model.h
++++ b/components/autofill/core/browser/ui/address_combobox_model.h
+@@ -40,8 +40,6 @@ class AddressComboboxModel : public ui::ComboboxModel {
+   std::u16string GetItemAt(int index) const override;
+   bool IsItemSeparatorAt(int index) const override;
+   int GetDefaultIndex() const override;
+-  void AddObserver(ui::ComboboxModelObserver* observer) override;
+-  void RemoveObserver(ui::ComboboxModelObserver* observer) override;
+ 
+   // Adds |profile| to model and return its combobox index. The lifespan of
+   // |profile| beyond this call is undefined so a copy must be made.
+@@ -72,9 +70,6 @@ class AddressComboboxModel : public ui::ComboboxModel {
+ 
+   // If non empty, the guid of the address that should be selected by default.
+   std::string default_selected_guid_;
+-
+-  // To be called when the data for the given country code was loaded.
+-  base::ObserverList<ui::ComboboxModelObserver> observers_;
+ };
+ 
+ }  // namespace autofill
+diff --git a/components/autofill/core/browser/ui/region_combobox_model.cc b/components/autofill/core/browser/ui/region_combobox_model.cc
+index 6439ccb1f074f1c3965dfb2b45ab6f6636437c17..42626aff427f1b3f3400aba399e3f9c89faa06ba 100644
+--- a/components/autofill/core/browser/ui/region_combobox_model.cc
++++ b/components/autofill/core/browser/ui/region_combobox_model.cc
+@@ -67,14 +67,6 @@ bool RegionComboboxModel::IsItemSeparatorAt(int index) const {
+   return regions_[index].first.empty();
+ }
+ 
+-void RegionComboboxModel::AddObserver(ui::ComboboxModelObserver* observer) {
+-  observers_.AddObserver(observer);
+-}
+-
+-void RegionComboboxModel::RemoveObserver(ui::ComboboxModelObserver* observer) {
+-  observers_.RemoveObserver(observer);
+-}
+-
+ void RegionComboboxModel::OnRegionDataLoaded(
+     const std::vector<const ::i18n::addressinput::RegionData*>& regions) {
+   // The RegionDataLoader will eventually self destruct after this call.
+@@ -95,7 +87,7 @@ void RegionComboboxModel::OnRegionDataLoaded(
+     failed_to_load_data_ = true;
+   }
+ 
+-  for (auto& observer : observers_) {
++  for (auto& observer : observers()) {
+     observer.OnComboboxModelChanged(this);
+   }
+ }
+diff --git a/components/autofill/core/browser/ui/region_combobox_model.h b/components/autofill/core/browser/ui/region_combobox_model.h
+index d1b5d8b9db0c4d9d15e4f2a50a6cba38b33f1be8..52cffd5d00649f8e2be7189bca128ba4c3e8a97d 100644
+--- a/components/autofill/core/browser/ui/region_combobox_model.h
++++ b/components/autofill/core/browser/ui/region_combobox_model.h
+@@ -54,8 +54,6 @@ class RegionComboboxModel : public ui::ComboboxModel {
+   int GetItemCount() const override;
+   std::u16string GetItemAt(int index) const override;
+   bool IsItemSeparatorAt(int index) const override;
+-  void AddObserver(ui::ComboboxModelObserver* observer) override;
+-  void RemoveObserver(ui::ComboboxModelObserver* observer) override;
+ 
+  private:
+   // Callback for the RegionDataLoader.
+@@ -72,9 +70,6 @@ class RegionComboboxModel : public ui::ComboboxModel {
+   // List of <code, name> pairs for ADDRESS_HOME_STATE combobox values;
+   std::vector<std::pair<std::string, std::string>> regions_;
+ 
+-  // To be called when the data for the given country code was loaded.
+-  base::ObserverList<ui::ComboboxModelObserver> observers_;
+-
+   // Weak pointer factory.
+   base::WeakPtrFactory<RegionComboboxModel> weak_factory_{this};
+ };
+diff --git a/ui/base/models/combobox_model.cc b/ui/base/models/combobox_model.cc
+index d307ee0ec592ae01a1d04cef85c0a492b7f7e29e..413adebd0349e304006de0cc0c4e2a4536ebbf06 100644
+--- a/ui/base/models/combobox_model.cc
++++ b/ui/base/models/combobox_model.cc
+@@ -4,10 +4,18 @@
+ 
+ #include "ui/base/models/combobox_model.h"
+ 
++#include "ui/base/models/combobox_model_observer.h"
+ #include "ui/base/models/image_model.h"
+ 
+ namespace ui {
+ 
++ComboboxModel::ComboboxModel() = default;
++
++ComboboxModel::~ComboboxModel() {
++  for (auto& observer : observers_)
++    observer.OnComboboxModelDestroying(this);
++}
++
+ std::u16string ComboboxModel::GetDropDownTextAt(int index) const {
+   return GetItemAt(index);
+ }
+@@ -36,4 +44,12 @@ bool ComboboxModel::IsItemEnabledAt(int index) const {
+   return true;
+ }
+ 
++void ComboboxModel::AddObserver(ComboboxModelObserver* observer) {
++  observers_.AddObserver(observer);
++}
++
++void ComboboxModel::RemoveObserver(ComboboxModelObserver* observer) {
++  observers_.RemoveObserver(observer);
++}
++
+ }  // namespace ui
+diff --git a/ui/base/models/combobox_model.h b/ui/base/models/combobox_model.h
+index 5264aaa5550859fce39a8485feff16c8fd83d897..eeff7dc8444e77fd598eb7ce05ab18259a9d7ef5 100644
+--- a/ui/base/models/combobox_model.h
++++ b/ui/base/models/combobox_model.h
+@@ -8,6 +8,7 @@
+ #include <string>
+ 
+ #include "base/component_export.h"
++#include "base/observer_list.h"
+ 
+ namespace ui {
+ 
+@@ -17,7 +18,8 @@ class ImageModel;
+ // A data model for a combo box.
+ class COMPONENT_EXPORT(UI_BASE) ComboboxModel {
+  public:
+-  virtual ~ComboboxModel() {}
++  ComboboxModel();
++  virtual ~ComboboxModel();
+ 
+   // Returns the number of items in the combo box.
+   virtual int GetItemCount() const = 0;
+@@ -52,9 +54,17 @@ class COMPONENT_EXPORT(UI_BASE) ComboboxModel {
+   // Returns true if the item at |index| is enabled.
+   virtual bool IsItemEnabledAt(int index) const;
+ 
+-  // Adds/removes an observer. Override if model supports mutation.
+-  virtual void AddObserver(ComboboxModelObserver* observer) {}
+-  virtual void RemoveObserver(ComboboxModelObserver* observer) {}
++  // Adds/removes an observer.
++  void AddObserver(ComboboxModelObserver* observer);
++  void RemoveObserver(ComboboxModelObserver* observer);
++
++ protected:
++  base::ObserverList<ui::ComboboxModelObserver>& observers() {
++    return observers_;
++  }
++
++ private:
++  base::ObserverList<ui::ComboboxModelObserver> observers_;
+ };
+ 
+ }  // namespace ui
+diff --git a/ui/base/models/combobox_model_observer.h b/ui/base/models/combobox_model_observer.h
+index 5e00ab68a0c14d0b48317ca287e3f105351a102c..44325ff0aa94b75eb662be1637f44aa5524a33e8 100644
+--- a/ui/base/models/combobox_model_observer.h
++++ b/ui/base/models/combobox_model_observer.h
+@@ -16,10 +16,13 @@ class ComboboxModel;
+ class COMPONENT_EXPORT(UI_BASE) ComboboxModelObserver
+     : public base::CheckedObserver {
+  public:
+-  // Invoked when |model| has changed in some way. The observer should assume
++  // Invoked when `model` has changed in some way. The observer should assume
+   // everything changed.
+   virtual void OnComboboxModelChanged(ComboboxModel* model) = 0;
+ 
++  // Invoked when `model` is destroyed. The observer should stop observing.
++  virtual void OnComboboxModelDestroying(ComboboxModel* model) = 0;
++
+  protected:
+   ~ComboboxModelObserver() override = default;
+ };
+diff --git a/ui/views/controls/combobox/combobox.cc b/ui/views/controls/combobox/combobox.cc
+index aa135250e469d2405fba618c96b0bf38acaf59f0..b8d41cfeb67f3b4c098b2b329b2dfa7fe246dabb 100644
+--- a/ui/views/controls/combobox/combobox.cc
++++ b/ui/views/controls/combobox/combobox.cc
+@@ -578,6 +578,10 @@ void Combobox::OnComboboxModelChanged(ui::ComboboxModel* model) {
+   OnContentSizeMaybeChanged();
+ }
+ 
++void Combobox::OnComboboxModelDestroying(ui::ComboboxModel* model) {
++  SetModel(nullptr);
++}
++
+ const base::RepeatingClosure& Combobox::GetCallback() const {
+   return callback_;
+ }
+diff --git a/ui/views/controls/combobox/combobox.h b/ui/views/controls/combobox/combobox.h
+index fc77f8e49cead5fb689fba3d56c65c1b8e69f298..1a3f533256eca91e4fd4552ee5425b7ca8e7ad7e 100644
+--- a/ui/views/controls/combobox/combobox.h
++++ b/ui/views/controls/combobox/combobox.h
+@@ -131,6 +131,7 @@ class VIEWS_EXPORT Combobox : public View,
+  protected:
+   // Overridden from ComboboxModelObserver:
+   void OnComboboxModelChanged(ui::ComboboxModel* model) override;
++  void OnComboboxModelDestroying(ui::ComboboxModel* model) override;
+ 
+   // Getters to be used by metadata.
+   const base::RepeatingClosure& GetCallback() const;
+diff --git a/ui/views/controls/combobox/combobox_unittest.cc b/ui/views/controls/combobox/combobox_unittest.cc
+index 7ce20acc4b3ecd6470d5f017f9171d0be35c09d2..91661f33074d3453b4f28a410ecc918b793f86c6 100644
+--- a/ui/views/controls/combobox/combobox_unittest.cc
++++ b/ui/views/controls/combobox/combobox_unittest.cc
+@@ -84,13 +84,6 @@ class TestComboboxModel : public ui::ComboboxModel {
+     return 0;
+   }
+ 
+-  void AddObserver(ui::ComboboxModelObserver* observer) override {
+-    observers_.AddObserver(observer);
+-  }
+-  void RemoveObserver(ui::ComboboxModelObserver* observer) override {
+-    observers_.RemoveObserver(observer);
+-  }
+-
+   void SetSeparators(const std::set<int>& separators) {
+     separators_ = separators;
+     OnModelChanged();
+@@ -103,11 +96,10 @@ class TestComboboxModel : public ui::ComboboxModel {
+ 
+  private:
+   void OnModelChanged() {
+-    for (auto& observer : observers_)
++    for (auto& observer : observers())
+       observer.OnComboboxModelChanged(this);
+   }
+ 
+-  base::ObserverList<ui::ComboboxModelObserver> observers_;
+   std::set<int> separators_;
+   int item_count_ = kItemCount;
+ };
+@@ -134,20 +126,13 @@ class VectorComboboxModel : public ui::ComboboxModel {
+   }
+   bool IsItemSeparatorAt(int index) const override { return false; }
+   int GetDefaultIndex() const override { return default_index_; }
+-  void AddObserver(ui::ComboboxModelObserver* observer) override {
+-    observers_.AddObserver(observer);
+-  }
+-  void RemoveObserver(ui::ComboboxModelObserver* observer) override {
+-    observers_.RemoveObserver(observer);
+-  }
+ 
+   void ValuesChanged() {
+-    for (auto& observer : observers_)
++    for (auto& observer : observers())
+       observer.OnComboboxModelChanged(this);
+   }
+ 
+  private:
+-  base::ObserverList<ui::ComboboxModelObserver> observers_;
+   int default_index_ = 0;
+   const raw_ptr<std::vector<std::string>> values_;
+ };
+@@ -884,6 +869,15 @@ TEST_F(ComboboxTest, SetTooltipTextNotifiesAccessibilityEvent) {
+   EXPECT_EQ(test_tooltip_text, ASCIIToUTF16(name));
+ }
+ 
++// Regression test for crbug.com/1264288.
++// Should fail in ASan build before the fix.
++TEST_F(ComboboxTest, NoCrashWhenComboboxOutlivesModel) {
++  auto model = std::make_unique<TestComboboxModel>();
++  auto combobox = std::make_unique<TestCombobox>(model.get());
++  model.reset();
++  combobox.reset();
++}
++
+ namespace {
+ 
+ using ComboboxDefaultTest = ViewsTestBase;
+diff --git a/ui/views/controls/editable_combobox/editable_combobox.cc b/ui/views/controls/editable_combobox/editable_combobox.cc
+index b5301bfb0a3c8da3daa1d3646c2f4872cef2cdab..faac607fefcc8caf572eb83456e7520146c1d0ad 100644
+--- a/ui/views/controls/editable_combobox/editable_combobox.cc
++++ b/ui/views/controls/editable_combobox/editable_combobox.cc
+@@ -190,10 +190,15 @@ class EditableCombobox::EditableComboboxMenuModel
+     return combobox_model_->GetDropDownIconAt(items_shown_[index].index);
+   }
+ 
++  // ComboboxModelObserver:
+   void OnComboboxModelChanged(ui::ComboboxModel* model) override {
+     UpdateItemsShown();
+   }
+ 
++  void OnComboboxModelDestroying(ui::ComboboxModel* model) override {
++    observation_.Reset();
++  }
++
+   int GetItemCount() const override { return items_shown_.size(); }
+ 
+  private: