From c7b81ad9251ffe8fafd6ebcdb63f6e6f47e4848f Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Mon, 8 Apr 2024 16:19:20 -0700 Subject: [PATCH] [ref-counted] Use down_cast for additional safety checks (#35579) Closes #35579 COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/35579 from ctiller:use-downcast 6f11268751fb2190ee1142ce5d321e2f0bb4347f PiperOrigin-RevId: 622981017 --- BUILD | 2 ++ CMakeLists.txt | 3 +++ build_autogenerated.yaml | 6 ++++++ src/core/BUILD | 2 ++ src/core/lib/gprpp/down_cast.h | 8 ++++---- src/core/lib/gprpp/dual_ref_counted.h | 13 +++++++++---- src/core/lib/gprpp/orphanable.h | 7 +++++-- src/core/lib/gprpp/ref_counted.h | 7 +++++-- src/core/lib/gprpp/ref_counted_ptr.h | 3 ++- src/core/lib/promise/context.h | 4 ++-- test/core/gprpp/down_cast_test.cc | 4 ++-- 11 files changed, 42 insertions(+), 17 deletions(-) diff --git a/BUILD b/BUILD index 417d9f21a94..3164a0c391e 100644 --- a/BUILD +++ b/BUILD @@ -3031,6 +3031,7 @@ grpc_cc_library( "debug_location", "gpr_platform", "ref_counted_ptr", + "//src/core:down_cast", "//src/core:ref_counted", ], ) @@ -3063,6 +3064,7 @@ grpc_cc_library( deps = [ "debug_location", "gpr_platform", + "//src/core:down_cast", ], ) diff --git a/CMakeLists.txt b/CMakeLists.txt index e5ba90c892a..f4c9825d0f3 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -7345,6 +7345,7 @@ target_include_directories(avl_test target_link_libraries(avl_test ${_gRPC_ALLTARGETS_LIBRARIES} gtest + absl::config absl::hash gpr ) @@ -12812,6 +12813,7 @@ target_include_directories(endpoint_config_test target_link_libraries(endpoint_config_test ${_gRPC_ALLTARGETS_LIBRARIES} gtest + absl::config absl::hash absl::type_traits absl::statusor @@ -30509,6 +30511,7 @@ target_include_directories(thread_quota_test target_link_libraries(thread_quota_test ${_gRPC_ALLTARGETS_LIBRARIES} gtest + absl::config absl::hash gpr ) diff --git a/build_autogenerated.yaml b/build_autogenerated.yaml index 54f1aafb067..d48f7694ce5 100644 --- a/build_autogenerated.yaml +++ b/build_autogenerated.yaml @@ -5742,12 +5742,14 @@ targets: headers: - src/core/lib/avl/avl.h - src/core/lib/gprpp/atomic_utils.h + - src/core/lib/gprpp/down_cast.h - src/core/lib/gprpp/ref_counted.h - src/core/lib/gprpp/ref_counted_ptr.h src: - test/core/avl/avl_test.cc deps: - gtest + - absl/base:config - absl/hash:hash - gpr uses_polling: false @@ -8944,6 +8946,7 @@ targets: - src/core/lib/channel/channel_args.h - src/core/lib/event_engine/channel_args_endpoint_config.h - src/core/lib/gprpp/atomic_utils.h + - src/core/lib/gprpp/down_cast.h - src/core/lib/gprpp/dual_ref_counted.h - src/core/lib/gprpp/orphanable.h - src/core/lib/gprpp/ref_counted.h @@ -8960,6 +8963,7 @@ targets: - test/core/event_engine/endpoint_config_test.cc deps: - gtest + - absl/base:config - absl/hash:hash - absl/meta:type_traits - absl/status:statusor @@ -19159,6 +19163,7 @@ targets: language: c++ headers: - src/core/lib/gprpp/atomic_utils.h + - src/core/lib/gprpp/down_cast.h - src/core/lib/gprpp/ref_counted.h - src/core/lib/gprpp/ref_counted_ptr.h - src/core/lib/resource_quota/thread_quota.h @@ -19167,6 +19172,7 @@ targets: - test/core/resource_quota/thread_quota_test.cc deps: - gtest + - absl/base:config - absl/hash:hash - gpr uses_polling: false diff --git a/src/core/BUILD b/src/core/BUILD index 7ab4b2b3f2c..1ce86ff5599 100644 --- a/src/core/BUILD +++ b/src/core/BUILD @@ -1157,6 +1157,7 @@ grpc_cc_library( public_hdrs = ["lib/gprpp/ref_counted.h"], deps = [ "atomic_utils", + "down_cast", "//:debug_location", "//:gpr", "//:ref_counted_ptr", @@ -1168,6 +1169,7 @@ grpc_cc_library( language = "c++", public_hdrs = ["lib/gprpp/dual_ref_counted.h"], deps = [ + "down_cast", "//:debug_location", "//:gpr", "//:orphanable", diff --git a/src/core/lib/gprpp/down_cast.h b/src/core/lib/gprpp/down_cast.h index cb71a1f7f6c..96a86de3265 100644 --- a/src/core/lib/gprpp/down_cast.h +++ b/src/core/lib/gprpp/down_cast.h @@ -26,10 +26,10 @@ namespace grpc_core { template -inline To down_cast(From* f) { +inline To DownCast(From* f) { static_assert( std::is_base_of::type>::value, - "down_cast requires a base-to-derived relationship"); + "DownCast requires a base-to-derived relationship"); // If we have RTTI & we're in debug, assert that the cast is legal. #if ABSL_INTERNAL_HAS_RTTI #ifndef NDEBUG @@ -40,8 +40,8 @@ inline To down_cast(From* f) { } template -inline To down_cast(From& f) { - return *down_cast::type*>(&f); +inline To DownCast(From& f) { + return *DownCast::type*>(&f); } } // namespace grpc_core diff --git a/src/core/lib/gprpp/dual_ref_counted.h b/src/core/lib/gprpp/dual_ref_counted.h index d3a126c8b44..8733d74dddd 100644 --- a/src/core/lib/gprpp/dual_ref_counted.h +++ b/src/core/lib/gprpp/dual_ref_counted.h @@ -25,6 +25,7 @@ #include #include "src/core/lib/gprpp/debug_location.h" +#include "src/core/lib/gprpp/down_cast.h" #include "src/core/lib/gprpp/orphanable.h" #include "src/core/lib/gprpp/ref_counted_ptr.h" @@ -68,7 +69,8 @@ class DualRefCounted { std::enable_if_t::value, bool> = true> RefCountedPtr RefAsSubclass() { IncrementRefCount(); - return RefCountedPtr(static_cast(this)); + return RefCountedPtr( + DownCast(static_cast(this))); } template < typename Subclass, @@ -76,7 +78,8 @@ class DualRefCounted { RefCountedPtr RefAsSubclass(const DebugLocation& location, const char* reason) { IncrementRefCount(location, reason); - return RefCountedPtr(static_cast(this)); + return RefCountedPtr( + DownCast(static_cast(this))); } void Unref() { @@ -179,7 +182,8 @@ class DualRefCounted { std::enable_if_t::value, bool> = true> WeakRefCountedPtr WeakRefAsSubclass() { IncrementWeakRefCount(); - return WeakRefCountedPtr(static_cast(this)); + return WeakRefCountedPtr( + DownCast(static_cast(this))); } template < typename Subclass, @@ -187,7 +191,8 @@ class DualRefCounted { WeakRefCountedPtr WeakRefAsSubclass(const DebugLocation& location, const char* reason) { IncrementWeakRefCount(location, reason); - return WeakRefCountedPtr(static_cast(this)); + return WeakRefCountedPtr( + DownCast(static_cast(this))); } void WeakUnref() { diff --git a/src/core/lib/gprpp/orphanable.h b/src/core/lib/gprpp/orphanable.h index f74cadfaf0c..09523d3946e 100644 --- a/src/core/lib/gprpp/orphanable.h +++ b/src/core/lib/gprpp/orphanable.h @@ -26,6 +26,7 @@ #include #include "src/core/lib/gprpp/debug_location.h" +#include "src/core/lib/gprpp/down_cast.h" #include "src/core/lib/gprpp/ref_counted.h" #include "src/core/lib/gprpp/ref_counted_ptr.h" @@ -102,7 +103,8 @@ class InternallyRefCounted : public Orphanable { std::enable_if_t::value, bool> = true> RefCountedPtr RefAsSubclass() { IncrementRefCount(); - return RefCountedPtr(static_cast(this)); + return RefCountedPtr( + DownCast(static_cast(this))); } template < typename Subclass, @@ -110,7 +112,8 @@ class InternallyRefCounted : public Orphanable { RefCountedPtr RefAsSubclass(const DebugLocation& location, const char* reason) { IncrementRefCount(location, reason); - return RefCountedPtr(static_cast(this)); + return RefCountedPtr( + DownCast(static_cast(this))); } GRPC_MUST_USE_RESULT RefCountedPtr RefIfNonZero() { diff --git a/src/core/lib/gprpp/ref_counted.h b/src/core/lib/gprpp/ref_counted.h index 9d56397fdb7..b2a90f61669 100644 --- a/src/core/lib/gprpp/ref_counted.h +++ b/src/core/lib/gprpp/ref_counted.h @@ -29,6 +29,7 @@ #include "src/core/lib/gprpp/atomic_utils.h" #include "src/core/lib/gprpp/debug_location.h" +#include "src/core/lib/gprpp/down_cast.h" #include "src/core/lib/gprpp/ref_counted_ptr.h" namespace grpc_core { @@ -310,7 +311,8 @@ class RefCounted : public Impl { std::enable_if_t::value, bool> = true> RefCountedPtr RefAsSubclass() { IncrementRefCount(); - return RefCountedPtr(static_cast(this)); + return RefCountedPtr( + DownCast(static_cast(this))); } template < typename Subclass, @@ -318,7 +320,8 @@ class RefCounted : public Impl { RefCountedPtr RefAsSubclass(const DebugLocation& location, const char* reason) { IncrementRefCount(location, reason); - return RefCountedPtr(static_cast(this)); + return RefCountedPtr( + DownCast(static_cast(this))); } // RefIfNonZero() for mutable types. diff --git a/src/core/lib/gprpp/ref_counted_ptr.h b/src/core/lib/gprpp/ref_counted_ptr.h index 3da9b7de0f4..006f1631d03 100644 --- a/src/core/lib/gprpp/ref_counted_ptr.h +++ b/src/core/lib/gprpp/ref_counted_ptr.h @@ -30,6 +30,7 @@ #include "absl/hash/hash.h" #include "src/core/lib/gprpp/debug_location.h" +#include "src/core/lib/gprpp/down_cast.h" namespace grpc_core { @@ -159,7 +160,7 @@ class RefCountedPtr { template ::value, bool> = true> RefCountedPtr TakeAsSubclass() { - return RefCountedPtr(static_cast(release())); + return RefCountedPtr(DownCast(release())); } template () will return the base, and GetContext() will -// down_cast to the derived type. +// DownCast to the derived type. // Specializations of this type should be created for each derived type, and // should have a single using statement Base pointing to the derived base class. // Example: @@ -84,7 +84,7 @@ class Context::Base>> public: using Context::Base>::Context; static T* get() { - return down_cast(Context::Base>::get()); + return DownCast(Context::Base>::get()); } }; diff --git a/test/core/gprpp/down_cast_test.cc b/test/core/gprpp/down_cast_test.cc index 87580e5d7bc..e0af7081734 100644 --- a/test/core/gprpp/down_cast_test.cc +++ b/test/core/gprpp/down_cast_test.cc @@ -32,8 +32,8 @@ class Derived : public Base { TEST(DownCastTest, DownCast) { Derived d; Base* b = &d; - EXPECT_EQ(down_cast(b)->i, 3); - EXPECT_EQ(down_cast(*b).i, 3); + EXPECT_EQ(DownCast(b)->i, 3); + EXPECT_EQ(DownCast(*b).i, 3); } } // namespace