base::CheckedObserver - a common base class for observers.

Turns a possible UAF when trying to notify a deleted observer into a
CHECK(). This is achieved with minimal changes to ObserverList and an
adapter that gives WeakPtr-like semantics to an observer interface.

base::ObserverList<T>::Unchecked continues to use raw pointers that
are unchecked.

Bug: 842987, 808318
Change-Id: I8ab20845f4f6e1d2559490287731cea2dbf40d39
Reviewed-on: https://chromium-review.googlesource.com/1053338
Commit-Queue: Trent Apted <tapted@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584694}

base/BUILD.gn

@@ -294,6 +294,10 @@ jumbo_component("base") {
     "cpu.h",
     "critical_closure.h",
     "critical_closure_internal_ios.mm",
+    "observer_list_internal.cc",
+    "observer_list_internal.h",
+    "observer_list_types.cc",
+    "observer_list_types.h",
 
     # This file depends on files from the "debug/allocator" target,
     # but this target does not depend on "debug/allocator".
@@ -2792,6 +2796,7 @@ if (enable_nocompile_tests) {
       "memory/weak_ptr_unittest.nc",
       "metrics/field_trial_params_unittest.nc",
       "metrics/histogram_unittest.nc",
+      "observer_list_unittest.nc",
       "optional_unittest.nc",
       "strings/string16_unittest.nc",
       "task/task_traits_extension_unittest.nc",

base/memory/weak_ptr.h

@@ -268,6 +268,11 @@ class WeakPtr : public internal::WeakPtrBase {
   // instance isn't being re-assigned or reset() racily with this call.
   bool MaybeValid() const { return ref_.MaybeValid(); }
 
+  // Returns whether the object |this| points to has been invalidated. This can
+  // be used to distinguish a WeakPtr to a destroyed object from one that has
+  // been explicitly set to null. A null WeakPtr is always valid.
+  bool WasInvalidated() const { return ptr_ && !ref_.IsValid(); }
+
  private:
   friend class internal::SupportsWeakPtrBase;
   template <typename U> friend class WeakPtr;

base/memory/weak_ptr_unittest.cc

@@ -392,6 +392,49 @@ TEST(WeakPtrTest, InvalidateWeakPtrs) {
   EXPECT_FALSE(factory.HasWeakPtrs());
 }
 
+// Tests that WasInvalidated() is true only for invalidated WeakPtrs (not
+// nullptr) and doesn't DCHECK.
+TEST(WeakPtrTest, WasInvalidatedByFactoryDestruction) {
+  WeakPtr<int> ptr;
+  EXPECT_FALSE(ptr.WasInvalidated());
+
+  // Test |data| destroyed.
+  {
+    int data;
+    WeakPtrFactory<int> factory(&data);
+    ptr = factory.GetWeakPtr();
+
+    EXPECT_FALSE(ptr.WasInvalidated());
+  }
+  EXPECT_TRUE(ptr.WasInvalidated());  // Shouldn't tickle asan.
+  ptr = nullptr;
+  EXPECT_FALSE(ptr.WasInvalidated());
+}
+
+// As above, but testing InvalidateWeakPtrs().
+TEST(WeakPtrTest, WasInvalidatedByInvalidateWeakPtrs) {
+  int data;
+  WeakPtrFactory<int> factory(&data);
+  WeakPtr<int> ptr = factory.GetWeakPtr();
+  EXPECT_FALSE(ptr.WasInvalidated());
+  factory.InvalidateWeakPtrs();
+  EXPECT_TRUE(ptr.WasInvalidated());
+  ptr = nullptr;
+  EXPECT_FALSE(ptr.WasInvalidated());
+}
+
+// Test WasInvalidated() when assigning null before invalidating.
+TEST(WeakPtrTest, WasInvalidatedWhilstNull) {
+  int data;
+  WeakPtrFactory<int> factory(&data);
+  WeakPtr<int> ptr = factory.GetWeakPtr();
+  EXPECT_FALSE(ptr.WasInvalidated());
+  ptr = nullptr;
+  EXPECT_FALSE(ptr.WasInvalidated());
+  factory.InvalidateWeakPtrs();
+  EXPECT_FALSE(ptr.WasInvalidated());
+}
+
 TEST(WeakPtrTest, MaybeValidOnSameSequence) {
   int data;
   WeakPtrFactory<int> factory(&data);

base/observer_list.h

@@ -17,6 +17,7 @@
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
+#include "base/observer_list_internal.h"
 #include "base/stl_util.h"
 
 ///////////////////////////////////////////////////////////////////////////////
@@ -94,16 +95,24 @@ enum class ObserverListPolicy {
 // TODO(oshima): Change the default to non reentrant. https://crbug.com/812109
 template <class ObserverType,
           bool check_empty = false,
-          bool allow_reentrancy = true>
-class ObserverList
-    : public SupportsWeakPtr<
-          ObserverList<ObserverType, check_empty, allow_reentrancy>> {
+          bool allow_reentrancy = true,
+          class ObserverStorageType = internal::CheckedObserverAdapter>
+class ObserverList : public SupportsWeakPtr<ObserverList<ObserverType,
+                                                         check_empty,
+                                                         allow_reentrancy,
+                                                         ObserverStorageType>> {
  public:
-  // Temporary type alias for introducing base::CheckedObserver. Existing
-  // ObserverLists will be Unchecked during the migration.
-  // TODO(https://crbug.com/842987): Use the Unchecked storage type when that
-  // template param is added.
-  using Unchecked = ObserverList<ObserverType, check_empty, allow_reentrancy>;
+  // Allow declaring an ObserverList<...>::Unchecked that replaces the default
+  // ObserverStorageType to use raw pointers. This is required to support legacy
+  // observers that do not inherit from CheckedObserver. The majority of NEW
+  // CODE SHOULD NOT USE THIS, but it may be suited for performance-critical
+  // situations to avoid overheads of a CHECK(). Note the type can't be chosen
+  // based on ObserverType's definition because ObserverLists are often declared
+  // in headers using a forward-declare of ObserverType.
+  using Unchecked = ObserverList<ObserverType,
+                                 check_empty,
+                                 allow_reentrancy,
+                                 internal::UncheckedObserverAdapter>;
 
   // An iterator class that can be used to access the list of observers.
   class Iter {
@@ -187,21 +196,23 @@ class ObserverList
     }
 
    private:
-    FRIEND_TEST_ALL_PREFIXES(ObserverListTest, BasicStdIterator);
-    FRIEND_TEST_ALL_PREFIXES(ObserverListTest, StdIteratorRemoveFront);
+    friend class ObserverListTestBase;
 
     ObserverType* GetCurrent() const {
       DCHECK(list_);
       DCHECK_LT(index_, clamped_max_index());
-      return list_->observers_[index_];
+      return ObserverStorageType::template Get<ObserverType>(
+          list_->observers_[index_]);
     }
 
     void EnsureValidIndex() {
       DCHECK(list_);
       const size_t max_index = clamped_max_index();
-      while (index_ < max_index && !list_->observers_[index_])
+      while (index_ < max_index &&
+             list_->observers_[index_].IsMarkedForRemoval()) {
         ++index_;
       }
+    }
 
     size_t clamped_max_index() const {
       return std::min(max_index_, list_->observers_.size());
@@ -220,6 +231,7 @@ class ObserverList
 
   using iterator = Iter;
   using const_iterator = Iter;
+  using value_type = ObserverType;
 
   const_iterator begin() const {
     // An optimization: do not involve weak pointers for empty list.
@@ -249,20 +261,22 @@ class ObserverList
       NOTREACHED() << "Observers can only be added once!";
       return;
     }
-    observers_.push_back(obs);
+    observers_.emplace_back(ObserverStorageType(obs));
   }
 
   // Removes the given observer from this list. Does nothing if this observer is
   // not in this list.
   void RemoveObserver(const ObserverType* obs) {
     DCHECK(obs);
-    const auto it = std::find(observers_.begin(), observers_.end(), obs);
+    const auto it =
+        std::find_if(observers_.begin(), observers_.end(),
+                     [obs](const auto& o) { return o.IsEqual(obs); });
     if (it == observers_.end())
       return;
 
     DCHECK_GE(live_iterator_count_, 0);
     if (live_iterator_count_) {
-      *it = nullptr;
+      it->MarkForRemoval();
     } else {
       observers_.erase(it);
     }
@@ -270,14 +284,22 @@ class ObserverList
 
   // Determine whether a particular observer is in the list.
   bool HasObserver(const ObserverType* obs) const {
-    return ContainsValue(observers_, obs);
+    // Client code passing null could be confused by the treatment of observers
+    // removed mid-iteration. TODO(tapted): This should probably DCHECK, but
+    // some client code currently does pass null to HasObserver().
+    if (obs == nullptr)
+      return false;
+    return std::find_if(observers_.begin(), observers_.end(),
+                        [obs](const auto& o) { return o.IsEqual(obs); }) !=
+           observers_.end();
   }
 
   // Removes all the observers from this list.
   void Clear() {
     DCHECK_GE(live_iterator_count_, 0);
     if (live_iterator_count_) {
-      std::fill(observers_.begin(), observers_.end(), nullptr);
+      for (auto& observer : observers_)
+        observer.MarkForRemoval();
     } else {
       observers_.clear();
     }
@@ -286,13 +308,12 @@ class ObserverList
   bool might_have_observers() const { return !observers_.empty(); }
 
  private:
-  // Compacts list of observers by removing null pointers.
+  // Compacts list of observers by removing those marked for removal.
   void Compact() {
-    observers_.erase(std::remove(observers_.begin(), observers_.end(), nullptr),
-                     observers_.end());
+    EraseIf(observers_, [](const auto& o) { return o.IsMarkedForRemoval(); });
   }
 
-  std::vector<ObserverType*> observers_;
+  std::vector<ObserverStorageType> observers_;
 
   // Number of active iterators referencing this ObserverList.
   //

base/observer_list_internal.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/observer_list_internal.h"
+
+namespace base {
+namespace internal {
+
+CheckedObserverAdapter::CheckedObserverAdapter(const CheckedObserver* observer)
+    : weak_ptr_(observer->factory_.GetWeakPtr()) {}
+
+CheckedObserverAdapter::CheckedObserverAdapter(CheckedObserverAdapter&& other) =
+    default;
+CheckedObserverAdapter& CheckedObserverAdapter::operator=(
+    CheckedObserverAdapter&& other) = default;
+CheckedObserverAdapter::~CheckedObserverAdapter() = default;
+
+}  // namespace internal
+}  // namespace base

base/observer_list_internal.h

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef BASE_OBSERVER_LIST_INTERNAL_H_
+#define BASE_OBSERVER_LIST_INTERNAL_H_
+
+#include "base/base_export.h"
+#include "base/logging.h"
+#include "base/macros.h"
+#include "base/memory/weak_ptr.h"
+#include "base/observer_list_types.h"
+
+namespace base {
+namespace internal {
+
+// Adapter for putting raw pointers into an ObserverList<Foo>::Unchecked.
+class BASE_EXPORT UncheckedObserverAdapter {
+ public:
+  explicit UncheckedObserverAdapter(const void* observer)
+      : ptr_(const_cast<void*>(observer)) {}
+  UncheckedObserverAdapter(UncheckedObserverAdapter&& other) = default;
+  UncheckedObserverAdapter& operator=(UncheckedObserverAdapter&& other) =
+      default;
+
+  void MarkForRemoval() { ptr_ = nullptr; }
+
+  bool IsMarkedForRemoval() const { return !ptr_; }
+  bool IsEqual(const void* rhs) const { return ptr_ == rhs; }
+
+  template <class ObserverType>
+  static ObserverType* Get(const UncheckedObserverAdapter& adapter) {
+    static_assert(
+        !std::is_base_of<CheckedObserver, ObserverType>::value,
+        "CheckedObserver classes must not use ObserverList<T>::Unchecked.");
+    return static_cast<ObserverType*>(adapter.ptr_);
+  }
+
+ private:
+  void* ptr_;
+
+  // Although copying works, disallow it to be consistent with
+  // CheckedObserverAdapter.
+  DISALLOW_COPY_AND_ASSIGN(UncheckedObserverAdapter);
+};
+
+// Adapter for CheckedObserver types so that they can use the same syntax as a
+// raw pointer when stored in the std::vector of observers in an ObserverList.
+// It wraps a WeakPtr<CheckedObserver> and allows a "null" pointer via
+// destruction to be distinguished from an observer marked for deferred removal
+// whilst an iteration is in progress.
+class BASE_EXPORT CheckedObserverAdapter {
+ public:
+  explicit CheckedObserverAdapter(const CheckedObserver* observer);
+
+  // Move-only construction and assignment is required to store this in STL
+  // types.
+  CheckedObserverAdapter(CheckedObserverAdapter&& other);
+  CheckedObserverAdapter& operator=(CheckedObserverAdapter&& other);
+  ~CheckedObserverAdapter();
+
+  void MarkForRemoval() {
+    DCHECK(weak_ptr_);
+    weak_ptr_.reset();
+  }
+
+  bool IsMarkedForRemoval() const {
+    // If |weak_ptr_| was invalidated then this attempt to iterate over the
+    // pointer is a UAF. Tip: If it's unclear where the `delete` occurred, try
+    // adding CHECK(!IsInObserverList()) to the ~CheckedObserver() (destructor)
+    // override. However, note that this is not always a bug: a destroyed
+    // observer can exist in an ObserverList so long as nothing iterates over
+    // the ObserverList before the list itself is destroyed.
+    CHECK(!weak_ptr_.WasInvalidated());
+    return weak_ptr_ == nullptr;
+  }
+
+  bool IsEqual(const CheckedObserver* rhs) const {
+    // Note that inside an iteration, ObserverList::HasObserver() may call this
+    // and |weak_ptr_| may be null due to a deferred removal, which is fine.
+    return weak_ptr_.get() == rhs;
+  }
+
+  template <class ObserverType>
+  static ObserverType* Get(const CheckedObserverAdapter& adapter) {
+    static_assert(
+        std::is_base_of<CheckedObserver, ObserverType>::value,
+        "Observers should inherit from base::CheckedObserver. "
+        "Use ObserverList<T>::Unchecked to observe with raw pointers.");
+    DCHECK(adapter.weak_ptr_);
+    return static_cast<ObserverType*>(adapter.weak_ptr_.get());
+  }
+
+ private:
+  WeakPtr<CheckedObserver> weak_ptr_;
+
+  DISALLOW_COPY_AND_ASSIGN(CheckedObserverAdapter);
+};
+
+}  // namespace internal
+}  // namespace base
+
+#endif  // BASE_OBSERVER_LIST_INTERNAL_H_

base/observer_list_types.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/observer_list_types.h"
+
+namespace base {
+
+CheckedObserver::CheckedObserver() : factory_(this) {}
+CheckedObserver::~CheckedObserver() = default;
+
+bool CheckedObserver::IsInObserverList() const {
+  return factory_.HasWeakPtrs();
+}
+
+}  // namespace base

base/observer_list_types.h

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef BASE_OBSERVER_LIST_TYPES_H_
+#define BASE_OBSERVER_LIST_TYPES_H_
+
+#include "base/base_export.h"
+#include "base/macros.h"
+#include "base/memory/weak_ptr.h"
+
+namespace base {
+namespace internal {
+class CheckedObserverAdapter;
+}
+
+// A CheckedObserver serves as a base class for an observer interface designed
+// to be used with base::ObserverList. It helps detect potential use-after-free
+// issues that can occur when observers fail to remove themselves from an
+// observer list upon destruction.
+//
+// A CheckedObserver will CHECK() if an ObserverList iteration is attempted over
+// a destroyed Observer.
+//
+// Note that a CheckedObserver subclass must be deleted on the same thread as
+// the ObserverList(s) it is added to. This is DCHECK()ed via WeakPtr.
+class BASE_EXPORT CheckedObserver {
+ public:
+  CheckedObserver();
+
+ protected:
+  virtual ~CheckedObserver();
+
+  // Returns whether |this| is in any ObserverList. Subclasses can CHECK() this
+  // in their destructor to obtain a nicer stacktrace.
+  bool IsInObserverList() const;
+
+ private:
+  friend class internal::CheckedObserverAdapter;
+
+  // Must be mutable to allow ObserverList<const Foo>.
+  mutable WeakPtrFactory<CheckedObserver> factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(CheckedObserver);
+};
+
+}  // namespace base
+
+#endif  // BASE_OBSERVER_LIST_TYPES_H_

base/observer_list_unittest.nc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// This is a "No Compile Test" suite.
+// http://dev.chromium.org/developers/testing/no-compile-tests
+
+#include <type_traits>
+
+#include "base/observer_list.h"
+
+namespace base {
+
+#if defined(NCTEST_CHECKED_OBSERVER_USING_UNCHECKED_LIST)  // [r"fatal error: static_assert failed due to requirement '!std::is_base_of<CheckedObserver, Observer>::value' \"CheckedObserver classes must not use ObserverList<T>::Unchecked.\""]
+
+void WontCompile() {
+  struct Observer : public CheckedObserver {
+    void OnObserve() {}
+  };
+  ObserverList<Observer>::Unchecked list;
+  for (auto& observer: list)
+    observer.OnObserve();
+}
+
+#elif defined(NCTEST_UNCHECKED_OBSERVER_USING_CHECKED_LIST)  // [r"fatal error: static_assert failed due to requirement 'std::is_base_of<CheckedObserver, UncheckedObserver>::value' \"Observers should inherit from base::CheckedObserver. Use ObserverList<T>::Unchecked to observe with raw pointers.\""]
+
+void WontCompile() {
+  struct UncheckedObserver {
+    void OnObserve() {}
+  };
+  ObserverList<UncheckedObserver> list;
+  for (auto& observer: list)
+    observer.OnObserve();
+}
+
+#endif
+
+}  // namespace base

base/test/gtest_util.h

@@ -41,6 +41,29 @@
 #endif
 // DCHECK_IS_ON() && defined(GTEST_HAS_DEATH_TEST) && !defined(OS_ANDROID)
 
+// As above, but for CHECK().
+#if defined(GTEST_HAS_DEATH_TEST) && !defined(OS_ANDROID)
+
+// Official builds will CHECK, but also eat stream parameters. So match "".
+#if defined(OFFICIAL_BUILD) && defined(NDEBUG)
+#define EXPECT_CHECK_DEATH(statement) EXPECT_DEATH(statement, "")
+#define ASSERT_CHECK_DEATH(statement) ASSERT_DEATH(statement, "")
+#else
+#define EXPECT_CHECK_DEATH(statement) EXPECT_DEATH(statement, "Check failed")
+#define ASSERT_CHECK_DEATH(statement) ASSERT_DEATH(statement, "Check failed")
+#endif  // defined(OFFICIAL_BUILD) && defined(NDEBUG)
+
+#else  // defined(GTEST_HAS_DEATH_TEST) && !defined(OS_ANDROID)
+
+// Note GTEST_UNSUPPORTED_DEATH_TEST takes a |regex| only to see whether it is a
+// valid regex. It is never evaluated.
+#define EXPECT_CHECK_DEATH(statement) \
+  GTEST_UNSUPPORTED_DEATH_TEST(statement, "", )
+#define ASSERT_CHECK_DEATH(statement) \
+  GTEST_UNSUPPORTED_DEATH_TEST(statement, "", return )
+
+#endif  // defined(GTEST_HAS_DEATH_TEST) && !defined(OS_ANDROID)
+
 namespace base {
 
 class FilePath;

ui/views/widget/widget.h

@@ -886,7 +886,7 @@ class VIEWS_EXPORT Widget : public internal::NativeWidgetDelegate,
 
   internal::NativeWidgetPrivate* native_widget_;
 
-  base::ObserverList<WidgetObserver>::Unchecked observers_;
+  base::ObserverList<WidgetObserver> observers_;
 
   base::ObserverList<WidgetRemovalsObserver>::Unchecked removals_observers_;
 

ui/views/widget/widget_observer.h

@@ -5,6 +5,7 @@
 #ifndef UI_VIEWS_WIDGET_WIDGET_OBSERVER_H_
 #define UI_VIEWS_WIDGET_WIDGET_OBSERVER_H_
 
+#include "base/observer_list_types.h"
 #include "ui/views/views_export.h"
 
 namespace gfx {
@@ -16,7 +17,7 @@ namespace views {
 class Widget;
 
 // Observers can listen to various events on the Widgets.
-class VIEWS_EXPORT WidgetObserver {
+class VIEWS_EXPORT WidgetObserver : public base::CheckedObserver {
  public:
   // The closing notification is sent immediately in response to (i.e. in the
   // same call stack as) a request to close the Widget (via Close() or
@@ -46,7 +47,7 @@ class VIEWS_EXPORT WidgetObserver {
                                      const gfx::Rect& new_bounds) {}
 
  protected:
-  virtual ~WidgetObserver() {}
+  ~WidgetObserver() override {}
 };
 
 }  // namespace views