Limit cross-kind conversions only to cases where kMayDangle gets added

Bug: 1407995 Change-Id: Ie0ceb58227dd565fe63062d2363f99e3127cc409 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4259413 Owners-Override: Bartek Nowierski <bartekn@chromium.org> Reviewed-by: Sergei Glazunov <glazunov@google.com> Commit-Queue: Bartek Nowierski <bartekn@chromium.org> Auto-Submit: Bartek Nowierski <bartekn@chromium.org> Cr-Commit-Position: refs/heads/main@{#1107616}
chromium · Feb 21, 2023 · 39912bd · 39912bd
1 parent 5ba10a4
commit 39912bd
Show file tree

Hide file tree

Showing 12 changed files with 96 additions and 42 deletions.
diff --git a/base/allocator/partition_allocator/pointers/raw_ptr.h b/base/allocator/partition_allocator/pointers/raw_ptr.h
@@ -134,6 +134,11 @@ enum class RawPtrTraits : unsigned {
   //
   // Test only.
   kUseCountingWrapperForTest = (1 << 4),
+
+  // Helper trait that can be used to test raw_ptr's behaviour or conversions.
+  //
+  // Test only.
+  kDummyForTest = (1 << 5),
 };
 
 // Used to combine RawPtrTraits:
@@ -160,12 +165,12 @@ constexpr RawPtrTraits Remove(RawPtrTraits a, RawPtrTraits b) {
 }
 
 constexpr bool AreValid(RawPtrTraits traits) {
-  return Remove(traits, RawPtrTraits::kMayDangle |
-                            RawPtrTraits::kDisableMTECheckedPtr |
-                            RawPtrTraits::kDisableHooks |
-                            RawPtrTraits::kAllowPtrArithmetic |
-                            RawPtrTraits::kUseCountingWrapperForTest) ==
-         RawPtrTraits::kEmpty;
+  return Remove(traits,
+                RawPtrTraits::kMayDangle | RawPtrTraits::kDisableMTECheckedPtr |
+                    RawPtrTraits::kDisableHooks |
+                    RawPtrTraits::kAllowPtrArithmetic |
+                    RawPtrTraits::kUseCountingWrapperForTest |
+                    RawPtrTraits::kDummyForTest) == RawPtrTraits::kEmpty;
 }
 
 template <RawPtrTraits Traits>
@@ -839,12 +844,26 @@ class PA_TRIVIAL_ABI PA_GSL_POINTER raw_ptr {
   PA_ALWAYS_INLINE explicit raw_ptr(const raw_ptr<T, PassedTraits>& p) noexcept
       : wrapped_ptr_(Impl::WrapRawPtrForDuplication(
             raw_ptr_traits::TraitsToImpl<PassedTraits>::Impl::
-                UnsafelyUnwrapPtrForDuplication(p.wrapped_ptr_))) {}
+                UnsafelyUnwrapPtrForDuplication(p.wrapped_ptr_))) {
+    // Limit cross-kind conversions only to cases where kMayDangle gets added,
+    // because that's needed for Unretained(Ref)Wrapper. Use a static_assert,
+    // instead of disabling via SFINAE, so that the compiler catches other
+    // conversions. Otherwise implicit raw_ptr<T> -> T* -> raw_ptr<> route will
+    // be taken.
+    static_assert(Traits == (PassedTraits | RawPtrTraits::kMayDangle));
+  }
 
   template <RawPtrTraits PassedTraits,
             typename Unused = std::enable_if_t<Traits != PassedTraits>>
   PA_ALWAYS_INLINE raw_ptr& operator=(
       const raw_ptr<T, PassedTraits>& p) noexcept {
+    // Limit cross-kind assignments only to cases where kMayDangle gets added,
+    // because that's needed for Unretained(Ref)Wrapper. Use a static_assert,
+    // instead of disabling via SFINAE, so that the compiler catches other
+    // conversions. Otherwise implicit raw_ptr<T> -> T* -> raw_ptr<> route will
+    // be taken.
+    static_assert(Traits == (PassedTraits | RawPtrTraits::kMayDangle));
+
     Impl::ReleaseWrappedPtr(wrapped_ptr_);
     wrapped_ptr_ = Impl::WrapRawPtrForDuplication(
         raw_ptr_traits::TraitsToImpl<PassedTraits>::Impl::

diff --git a/base/allocator/partition_allocator/pointers/raw_ptr_unittest.cc b/base/allocator/partition_allocator/pointers/raw_ptr_unittest.cc
@@ -1352,7 +1352,7 @@ TEST_F(RawPtrTest, WorksWithVariant) {
   EXPECT_EQ(&x, absl::get<raw_ptr<int>>(vary));
 }
 
-TEST_F(RawPtrTest, CrossKindConversions) {
+TEST_F(RawPtrTest, CrossKindConversion) {
   int x = 123;
   CountingRawPtr<int> ptr1 = &x;
 
@@ -1369,18 +1369,24 @@ TEST_F(RawPtrTest, CrossKindConversions) {
   EXPECT_THAT((CountingRawPtrExpectations<RawPtrCountingMayDangleImpl>{
                   .wrap_raw_ptr_cnt = 0, .wrap_raw_ptr_for_dup_cnt = 1}),
               CountersMatch());
+}
+
+TEST_F(RawPtrTest, CrossKindAssignment) {
+  int x = 123;
+  CountingRawPtr<int> ptr1 = &x;
 
   RawPtrCountingImpl::ClearCounters();
   RawPtrCountingMayDangleImpl::ClearCounters();
 
-  CountingRawPtr<int> ptr3(ptr2);
+  CountingRawPtrMayDangle<int> ptr2;
+  ptr2 = ptr1;
 
-  EXPECT_THAT((CountingRawPtrExpectations<RawPtrCountingMayDangleImpl>{
+  EXPECT_THAT((CountingRawPtrExpectations<RawPtrCountingImpl>{
                   .get_for_dereference_cnt = 0,
                   .get_for_extraction_cnt = 0,
                   .get_for_duplication_cnt = 1}),
               CountersMatch());
-  EXPECT_THAT((CountingRawPtrExpectations<RawPtrCountingImpl>{
+  EXPECT_THAT((CountingRawPtrExpectations<RawPtrCountingMayDangleImpl>{
                   .wrap_raw_ptr_cnt = 0, .wrap_raw_ptr_for_dup_cnt = 1}),
               CountersMatch());
 }
@@ -1841,48 +1847,36 @@ TEST_F(BackupRefPtrTest, DanglingPtrComparison) {
 }
 
 // Check the assignment operator works, even across raw_ptr with different
-// dangling policies.
+// dangling policies (only `not dangling` -> `dangling` direction is supported).
 TEST_F(BackupRefPtrTest, DanglingPtrAssignment) {
   ScopedInstallDanglingRawPtrChecks enable_dangling_raw_ptr_checks;
 
   void* ptr = allocator_.root()->Alloc(16, "");
 
-  raw_ptr<void, DisableDanglingPtrDetection> dangling_ptr_1;
-  raw_ptr<void, DisableDanglingPtrDetection> dangling_ptr_2;
+  raw_ptr<void, DisableDanglingPtrDetection> dangling_ptr;
   raw_ptr<void> not_dangling_ptr;
 
-  dangling_ptr_1 = ptr;
-
-  not_dangling_ptr = dangling_ptr_1;
-  dangling_ptr_1 = nullptr;
-
-  dangling_ptr_2 = not_dangling_ptr;
+  not_dangling_ptr = ptr;
+  dangling_ptr = not_dangling_ptr;
   not_dangling_ptr = nullptr;
 
   allocator_.root()->Free(ptr);
 
-  dangling_ptr_1 = dangling_ptr_2;
-  dangling_ptr_2 = nullptr;
-
-  not_dangling_ptr = dangling_ptr_1;
-  dangling_ptr_1 = nullptr;
+  dangling_ptr = nullptr;
 }
 
 // Check the copy constructor works, even across raw_ptr with different dangling
-// policies.
+// policies (only `not dangling` -> `dangling` direction is supported).
 TEST_F(BackupRefPtrTest, DanglingPtrCopyContructor) {
   ScopedInstallDanglingRawPtrChecks enable_dangling_raw_ptr_checks;
 
   void* ptr = allocator_.root()->Alloc(16, "");
 
-  raw_ptr<void, DisableDanglingPtrDetection> dangling_ptr_1(ptr);
-  raw_ptr<void> not_dangling_ptr_1(ptr);
-
-  raw_ptr<void, DisableDanglingPtrDetection> dangling_ptr_2(not_dangling_ptr_1);
-  raw_ptr<void> not_dangling_ptr_2(dangling_ptr_1);
+  raw_ptr<void> not_dangling_ptr(ptr);
+  raw_ptr<void, DisableDanglingPtrDetection> dangling_ptr(not_dangling_ptr);
 
-  not_dangling_ptr_1 = nullptr;
-  not_dangling_ptr_2 = nullptr;
+  not_dangling_ptr = nullptr;
+  dangling_ptr = nullptr;
 
   allocator_.root()->Free(ptr);
 }

diff --git a/base/allocator/partition_allocator/pointers/raw_ptr_unittest.nc b/base/allocator/partition_allocator/pointers/raw_ptr_unittest.nc
@@ -227,6 +227,36 @@ void WontCompile() {
 #endif  // !BUILDFLAG(HAS_64_BIT_POINTERS)
 }
 
+#elif defined(NCTEST_CROSS_KIND_CONVERSION_FROM_MAY_DANGLE) // [r"static assertion failed due to requirement '\(base::RawPtrTraits\)0U == \(\(base::RawPtrTraits\)1U | RawPtrTraits::kMayDangle\)'"]
+
+void WontCompile() {
+  raw_ptr<int, base::RawPtrTraits::kMayDangle> ptr = new int(3);
+  [[maybe_unused]] raw_ptr<int> ptr2(ptr);
+}
+
+#elif defined(NCTEST_CROSS_KIND_CONVERSION_FROM_DUMMY) // [r"static assertion failed due to requirement '\(base::RawPtrTraits\)0U == \(\(base::RawPtrTraits\)1U | RawPtrTraits::kMayDangle\)'"]
+
+void WontCompile() {
+  raw_ptr<int, base::RawPtrTraits::kDummyForTest> ptr = new int(3);
+  [[maybe_unused]] raw_ptr<int, base::RawPtrTraits::kMayDangle> ptr2(ptr);
+}
+
+#elif defined(NCTEST_CROSS_KIND_ASSIGNMENT_FROM_MAY_DANGLE) // [r"static assertion failed due to requirement '\(base::RawPtrTraits\)1U == \(\(base::RawPtrTraits\)16U | RawPtrTraits::kMayDangle\)'"]
+
+void WontCompile() {
+  raw_ptr<int, base::RawPtrTraits::kMayDangle> ptr = new int(3);
+  raw_ptr<int> ptr2;
+  ptr2 = ptr;
+}
+
+#elif defined(NCTEST_CROSS_KIND_ASSIGNMENT_FROM_DUMMY) // [r"static assertion failed due to requirement '\(base::RawPtrTraits\)1U == \(\(base::RawPtrTraits\)16U | RawPtrTraits::kMayDangle\)'"]
+
+void WontCompile() {
+  raw_ptr<int, base::RawPtrTraits::kDummyForTest> ptr = new int(3);
+  raw_ptr<int, base::RawPtrTraits::kMayDangle> ptr2;
+  ptr2 = ptr;
+}
+
 #endif
 
 }  // namespace
diff --git a/chrome/browser/ui/views/autofill/payments/local_card_migration_dialog_view.h b/chrome/browser/ui/views/autofill/payments/local_card_migration_dialog_view.h
@@ -63,7 +63,8 @@ class LocalCardMigrationDialogView : public LocalCardMigrationDialog,
 
   // The view containing a list of cards. It is the content of the scroll bar.
   // Owned by the LocalCardMigrationOfferView.
-  raw_ptr<views::View> card_list_view_;
+  // DanglingUntriaged because it is assigned a DanglingUntriaged pointer.
+  raw_ptr<views::View, DanglingUntriaged> card_list_view_;
 };
 
 }  // namespace autofill

diff --git a/chrome/browser/ui/web_applications/web_app_metrics.h b/chrome/browser/ui/web_applications/web_app_metrics.h
@@ -110,7 +110,9 @@ class WebAppMetrics : public KeyedService,
   int num_user_installed_apps_ = kNumUserInstalledAppsNotCounted;
 
   base::flat_map<web_app::AppId, base::Time> app_last_interacted_time_{};
-  raw_ptr<content::WebContents> foreground_web_contents_ = nullptr;
+  // DanglingUntriaged because it is assigned a DanglingUntriaged pointer.
+  raw_ptr<content::WebContents, DanglingUntriaged> foreground_web_contents_ =
+      nullptr;
   GURL last_recorded_web_app_start_url_;
 
   const raw_ptr<Profile> profile_;

diff --git a/components/sync/driver/glue/sync_engine_impl.h b/components/sync/driver/glue/sync_engine_impl.h
@@ -204,7 +204,8 @@ class SyncEngineImpl : public SyncEngine,
 
   // The host which we serve (and are owned by). Set in Initialize() and nulled
   // out in StopSyncingForShutdown().
-  raw_ptr<SyncEngineHost> host_ = nullptr;
+  // DanglingUntriaged because it is assigned a DanglingUntriaged pointer.
+  raw_ptr<SyncEngineHost, DanglingUntriaged> host_ = nullptr;
 
   raw_ptr<invalidation::InvalidationService> invalidator_ = nullptr;
   bool invalidation_handler_registered_ = false;

diff --git a/components/sync/test/fake_sync_engine.h b/components/sync/test/fake_sync_engine.h
@@ -111,7 +111,8 @@ class FakeSyncEngine : public SyncEngine,
   const bool allow_init_completion_;
   const bool is_first_time_sync_configure_;
   const base::RepeatingClosure sync_transport_data_cleared_cb_;
-  raw_ptr<SyncEngineHost> host_ = nullptr;
+  // DanglingUntriaged because it is assigned a DanglingUntriaged pointer.
+  raw_ptr<SyncEngineHost, DanglingUntriaged> host_ = nullptr;
   bool initialized_ = false;
   const SyncStatus default_sync_status_;
   CoreAccountId authenticated_account_id_;

diff --git a/gpu/command_buffer/client/query_tracker.h b/gpu/command_buffer/client/query_tracker.h
@@ -61,7 +61,8 @@ class GLES2_IMPL_EXPORT QuerySyncManager {
     uint32_t index() const { return sync - bucket->syncs.get(); }
 
     raw_ptr<Bucket, DanglingUntriaged> bucket = nullptr;
-    raw_ptr<QuerySync, DanglingUntriaged> sync = nullptr;
+    // AllowPtrArithmetic because it is assigned an AllowPtrArithmetic pointer.
+    raw_ptr<QuerySync, DanglingUntriaged | AllowPtrArithmetic> sync = nullptr;
     int32_t submit_count = 0;
   };
 

diff --git a/net/http/http_stream_parser.cc b/net/http/http_stream_parser.cc
@@ -166,7 +166,8 @@ class HttpStreamParser::SeekableIOBuffer : public IOBuffer {
     data_ = real_data_;
   }
 
-  raw_ptr<char, AllowPtrArithmetic> real_data_;
+  // DanglingUntriaged because it is assigned a DanglingUntriaged pointer.
+  raw_ptr<char, DanglingUntriaged | AllowPtrArithmetic> real_data_;
   const int capacity_;
   int size_ = 0;
   int used_ = 0;

diff --git a/net/quic/quic_http_stream.h b/net/quic/quic_http_stream.h
@@ -166,7 +166,8 @@ class NET_EXPORT_PRIVATE QuicHttpStream : public MultiplexedHttpStream {
   bool can_send_early_ = false;
 
   // The request body to send, if any, owned by the caller.
-  raw_ptr<UploadDataStream> request_body_stream_ = nullptr;
+  // DanglingUntriaged because it is assigned a DanglingUntriaged pointer.
+  raw_ptr<UploadDataStream, DanglingUntriaged> request_body_stream_ = nullptr;
   // Time the request was issued.
   base::Time request_time_;
   // The priority of the request.

diff --git a/ui/views/widget/desktop_aura/desktop_native_widget_aura.h b/ui/views/widget/desktop_aura/desktop_native_widget_aura.h
@@ -283,7 +283,8 @@ class VIEWS_EXPORT DesktopNativeWidgetAura
                    ui::mojom::DragOperation& output_drag_op);
 
   std::unique_ptr<aura::WindowTreeHost> host_;
-  raw_ptr<DesktopWindowTreeHost> desktop_window_tree_host_;
+  // DanglingUntriaged because it is assigned a DanglingUntriaged pointer.
+  raw_ptr<DesktopWindowTreeHost, DanglingUntriaged> desktop_window_tree_host_;
 
   // See class documentation for Widget in widget.h for a note about ownership.
   Widget::InitParams::Ownership ownership_ =

diff --git a/ui/views/widget/widget.h b/ui/views/widget/widget.h
@@ -369,7 +369,9 @@ class VIEWS_EXPORT Widget : public internal::NativeWidgetDelegate,
     // of the default one.
     // TODO(beng): Figure out if there's a better way to expose this, e.g. get
     // rid of NW subclasses and do this all via message handling.
-    raw_ptr<DesktopWindowTreeHost> desktop_window_tree_host = nullptr;
+    // DanglingUntriaged because it is assigned a DanglingUntriaged pointer.
+    raw_ptr<DesktopWindowTreeHost, DanglingUntriaged> desktop_window_tree_host =
+        nullptr;
 
     // Only used by NativeWidgetAura. Specifies the type of layer for the
     // aura::Window.