From da75cef983fb1dc44c6221dbd35f670a885046e5 Mon Sep 17 00:00:00 2001 From: Steven Moreland Date: Wed, 31 Mar 2021 16:05:04 +0000 Subject: [PATCH] ANDROID_UTILS_REF_BASE_DISABLE_IMPLICIT_CONSTRUCTION In form, inspired by ANDROID_BASE_UNIQUE_FD_DISABLE_IMPLICIT_CONVERSION. We get occasional bugs about sp double-ownership. When this flag is enabled, we have: - you must construct RefBase objects using sp<>::make - you must construct wp<> objects by converting them to sp<> - if you want to convert a raw pointer to an sp<> object (this is possible since the refcount is used internally, and is used commonly on this*), then you must use 'assertStrongRefExists' semantics which aborts if there is no strong ref held. That is, if a client uses std::make_shared and then calls a function which internally used to call `sp(this)`, you would now call `sp::assertStrongRefExists(this)`, and the double ownership problem would become a runtime error. Bug: 184190315 Test: libutils_test Change-Id: Ie18d3146420df1808e3733027070ec234dda4e9d --- libutils/RefBase.cpp | 22 +++++++ libutils/RefBase_test.cpp | 24 ++++++++ libutils/StrongPointer_test.cpp | 15 ++++- libutils/include/utils/RefBase.h | 80 +++++++++++++++++++++++++- libutils/include/utils/StrongPointer.h | 39 +++++++++++++ 5 files changed, 177 insertions(+), 3 deletions(-) diff --git a/libutils/RefBase.cpp b/libutils/RefBase.cpp index 8e45226c5..b57e28741 100644 --- a/libutils/RefBase.cpp +++ b/libutils/RefBase.cpp @@ -443,6 +443,20 @@ void RefBase::incStrong(const void* id) const refs->mBase->onFirstRef(); } +void RefBase::incStrongRequireStrong(const void* id) const { + weakref_impl* const refs = mRefs; + refs->incWeak(id); + + refs->addStrongRef(id); + const int32_t c = refs->mStrong.fetch_add(1, std::memory_order_relaxed); + + LOG_ALWAYS_FATAL_IF(c <= 0 || c == INITIAL_STRONG_VALUE, + "incStrongRequireStrong() called on %p which isn't already owned", refs); +#if PRINT_REFS + ALOGD("incStrong (requiring strong) of %p from %p: cnt=%d\n", this, id, c); +#endif +} + void RefBase::decStrong(const void* id) const { weakref_impl* const refs = mRefs; @@ -521,6 +535,14 @@ void RefBase::weakref_type::incWeak(const void* id) ALOG_ASSERT(c >= 0, "incWeak called on %p after last weak ref", this); } +void RefBase::weakref_type::incWeakRequireWeak(const void* id) +{ + weakref_impl* const impl = static_cast(this); + impl->addWeakRef(id); + const int32_t c __unused = impl->mWeak.fetch_add(1, + std::memory_order_relaxed); + LOG_ALWAYS_FATAL_IF(c <= 0, "incWeakRequireWeak called on %p which has no weak refs", this); +} void RefBase::weakref_type::decWeak(const void* id) { diff --git a/libutils/RefBase_test.cpp b/libutils/RefBase_test.cpp index c9b489423..dcc469e48 100644 --- a/libutils/RefBase_test.cpp +++ b/libutils/RefBase_test.cpp @@ -241,6 +241,30 @@ TEST(RefBase, ReplacedComparison) { ASSERT_FALSE(wp1 != wp2); } +TEST(RefBase, AssertWeakRefExistsSuccess) { + // uses some other refcounting method, or non at all + bool isDeleted; + sp foo = sp::make(&isDeleted); + wp weakFoo = foo; + + EXPECT_EQ(weakFoo, wp::fromExisting(foo.get())); + + EXPECT_FALSE(isDeleted); + foo = nullptr; + EXPECT_TRUE(isDeleted); +} + +TEST(RefBase, AssertWeakRefExistsDeath) { + // uses some other refcounting method, or non at all + bool isDeleted; + Foo* foo = new Foo(&isDeleted); + + // can only get a valid wp<> object when you construct it from an sp<> + EXPECT_DEATH(wp::fromExisting(foo), ""); + + delete foo; +} + // Set up a situation in which we race with visit2AndRremove() to delete // 2 strong references. Bar destructor checks that there are no early // deletions and prior updates are visible to destructor. diff --git a/libutils/StrongPointer_test.cpp b/libutils/StrongPointer_test.cpp index d37c1de6f..29f6bd4bb 100644 --- a/libutils/StrongPointer_test.cpp +++ b/libutils/StrongPointer_test.cpp @@ -21,8 +21,8 @@ using namespace android; -class SPFoo : public LightRefBase { -public: +class SPFoo : virtual public RefBase { + public: explicit SPFoo(bool* deleted_check) : mDeleted(deleted_check) { *mDeleted = false; } @@ -69,3 +69,14 @@ TEST(StrongPointer, PointerComparison) { ASSERT_NE(nullptr, foo); ASSERT_NE(foo, nullptr); } + +TEST(StrongPointer, AssertStrongRefExists) { + // uses some other refcounting method, or non at all + bool isDeleted; + SPFoo* foo = new SPFoo(&isDeleted); + + // can only get a valid sp<> object when you construct it as an sp<> object + EXPECT_DEATH(sp::fromExisting(foo), ""); + + delete foo; +} diff --git a/libutils/include/utils/RefBase.h b/libutils/include/utils/RefBase.h index e7acd17d2..5a5bd56e5 100644 --- a/libutils/include/utils/RefBase.h +++ b/libutils/include/utils/RefBase.h @@ -140,7 +140,9 @@ // count, and accidentally passed to f(sp), a strong pointer to the object // will be temporarily constructed and destroyed, prematurely deallocating the // object, and resulting in heap corruption. None of this would be easily -// visible in the source. +// visible in the source. See below on +// ANDROID_UTILS_REF_BASE_DISABLE_IMPLICIT_CONSTRUCTION for a compile time +// option which helps avoid this case. // Extra Features: @@ -167,6 +169,42 @@ // to THE SAME sp<> or wp<>. In effect, their thread-safety properties are // exactly like those of T*, NOT atomic. +// Safety option: ANDROID_UTILS_REF_BASE_DISABLE_IMPLICIT_CONSTRUCTION +// +// This flag makes the semantics for using a RefBase object with wp<> and sp<> +// much stricter by disabling implicit conversion from raw pointers to these +// objects. In order to use this, apply this flag in Android.bp like so: +// +// cflags: [ +// "-DANDROID_UTILS_REF_BASE_DISABLE_IMPLICIT_CONSTRUCTION", +// ], +// +// REGARDLESS of whether this flag is on, best usage of sp<> is shown below. If +// this flag is on, no other usage is possible (directly calling RefBase methods +// is possible, but seeing code using 'incStrong' instead of 'sp<>', for +// instance, should already set off big alarm bells. With carefully constructed +// data structures, it should NEVER be necessary to directly use RefBase +// methods). Proper RefBase usage: +// +// class Foo : virtual public RefBase { ... }; +// +// // always construct an sp object with sp::make +// sp myFoo = sp::make(/*args*/); +// +// // if you need a weak pointer, it must be constructed from a strong +// // pointer +// wp weakFoo = myFoo; // NOT myFoo.get() +// +// // If you are inside of a method of Foo and need access to a strong +// // explicitly call this function. This documents your intention to code +// // readers, and it will give a runtime error for what otherwise would +// // be potential double ownership +// .... Foo::someMethod(...) { +// // asserts if there is a memory issue +// sp thiz = sp::fromExisting(this); +// } +// + #ifndef ANDROID_REF_BASE_H #define ANDROID_REF_BASE_H @@ -244,6 +282,7 @@ class RefBase { public: void incStrong(const void* id) const; + void incStrongRequireStrong(const void* id) const; void decStrong(const void* id) const; void forceIncStrong(const void* id) const; @@ -257,6 +296,7 @@ public: RefBase* refBase() const; void incWeak(const void* id); + void incWeakRequireWeak(const void* id); void decWeak(const void* id); // acquires a strong reference if there is already one. @@ -365,10 +405,24 @@ public: inline wp() : m_ptr(nullptr), m_refs(nullptr) { } + // if nullptr, returns nullptr + // + // if a weak pointer is already available, this will retrieve it, + // otherwise, this will abort + static inline wp fromExisting(T* other); + + // for more information about this flag, see above +#if defined(ANDROID_UTILS_REF_BASE_DISABLE_IMPLICIT_CONSTRUCTION) + wp(std::nullptr_t) : wp() {} +#else wp(T* other); // NOLINT(implicit) +#endif wp(const wp& other); explicit wp(const sp& other); + +#if !defined(ANDROID_UTILS_REF_BASE_DISABLE_IMPLICIT_CONSTRUCTION) template wp(U* other); // NOLINT(implicit) +#endif template wp(const sp& other); // NOLINT(implicit) template wp(const wp& other); // NOLINT(implicit) @@ -376,11 +430,15 @@ public: // Assignment +#if !defined(ANDROID_UTILS_REF_BASE_DISABLE_IMPLICIT_CONSTRUCTION) wp& operator = (T* other); +#endif wp& operator = (const wp& other); wp& operator = (const sp& other); +#if !defined(ANDROID_UTILS_REF_BASE_DISABLE_IMPLICIT_CONSTRUCTION) template wp& operator = (U* other); +#endif template wp& operator = (const wp& other); template wp& operator = (const sp& other); @@ -481,12 +539,26 @@ private: // Note that the above comparison operations go out of their way to provide an ordering consistent // with ordinary pointer comparison; otherwise they could ignore m_ptr, and just compare m_refs. +template +wp wp::fromExisting(T* other) { + if (!other) return nullptr; + + auto refs = other->getWeakRefs(); + refs->incWeakRequireWeak(other); + + wp ret; + ret.m_refs = refs; + return ret; +} + +#if !defined(ANDROID_UTILS_REF_BASE_DISABLE_IMPLICIT_CONSTRUCTION) template wp::wp(T* other) : m_ptr(other) { m_refs = other ? m_refs = other->createWeak(this) : nullptr; } +#endif template wp::wp(const wp& other) @@ -502,12 +574,14 @@ wp::wp(const sp& other) m_refs = m_ptr ? m_ptr->createWeak(this) : nullptr; } +#if !defined(ANDROID_UTILS_REF_BASE_DISABLE_IMPLICIT_CONSTRUCTION) template template wp::wp(U* other) : m_ptr(other) { m_refs = other ? other->createWeak(this) : nullptr; } +#endif template template wp::wp(const wp& other) @@ -534,6 +608,7 @@ wp::~wp() if (m_ptr) m_refs->decWeak(this); } +#if !defined(ANDROID_UTILS_REF_BASE_DISABLE_IMPLICIT_CONSTRUCTION) template wp& wp::operator = (T* other) { @@ -544,6 +619,7 @@ wp& wp::operator = (T* other) m_refs = newRefs; return *this; } +#endif template wp& wp::operator = (const wp& other) @@ -569,6 +645,7 @@ wp& wp::operator = (const sp& other) return *this; } +#if !defined(ANDROID_UTILS_REF_BASE_DISABLE_IMPLICIT_CONSTRUCTION) template template wp& wp::operator = (U* other) { @@ -579,6 +656,7 @@ wp& wp::operator = (U* other) m_refs = newRefs; return *this; } +#endif template template wp& wp::operator = (const wp& other) diff --git a/libutils/include/utils/StrongPointer.h b/libutils/include/utils/StrongPointer.h index 1fa46fe5d..1f070524a 100644 --- a/libutils/include/utils/StrongPointer.h +++ b/libutils/include/utils/StrongPointer.h @@ -50,10 +50,25 @@ public: template static inline sp make(Args&&... args); + // if nullptr, returns nullptr + // + // if a strong pointer is already available, this will retrieve it, + // otherwise, this will abort + static inline sp fromExisting(T* other); + + // for more information about this macro and correct RefBase usage, see + // the comment at the top of utils/RefBase.h +#if defined(ANDROID_UTILS_REF_BASE_DISABLE_IMPLICIT_CONSTRUCTION) + sp(std::nullptr_t) : sp() {} +#else sp(T* other); // NOLINT(implicit) +#endif sp(const sp& other); sp(sp&& other) noexcept; + +#if !defined(ANDROID_UTILS_REF_BASE_DISABLE_IMPLICIT_CONSTRUCTION) template sp(U* other); // NOLINT(implicit) +#endif template sp(const sp& other); // NOLINT(implicit) template sp(sp&& other); // NOLINT(implicit) @@ -61,13 +76,17 @@ public: // Assignment +#if !defined(ANDROID_UTILS_REF_BASE_DISABLE_IMPLICIT_CONSTRUCTION) sp& operator = (T* other); +#endif sp& operator = (const sp& other); sp& operator=(sp&& other) noexcept; template sp& operator = (const sp& other); template sp& operator = (sp&& other); +#if !defined(ANDROID_UTILS_REF_BASE_DISABLE_IMPLICIT_CONSTRUCTION) template sp& operator = (U* other); +#endif //! Special optimization for use by ProcessState (and nobody else). void force_set(T* other); @@ -201,6 +220,19 @@ sp sp::make(Args&&... args) { return result; } +template +sp sp::fromExisting(T* other) { + if (other) { + check_not_on_stack(other); + other->incStrongRequireStrong(other); + sp result; + result.m_ptr = other; + return result; + } + return nullptr; +} + +#if !defined(ANDROID_UTILS_REF_BASE_DISABLE_IMPLICIT_CONSTRUCTION) template sp::sp(T* other) : m_ptr(other) { @@ -209,6 +241,7 @@ sp::sp(T* other) other->incStrong(this); } } +#endif template sp::sp(const sp& other) @@ -222,6 +255,7 @@ sp::sp(sp&& other) noexcept : m_ptr(other.m_ptr) { other.m_ptr = nullptr; } +#if !defined(ANDROID_UTILS_REF_BASE_DISABLE_IMPLICIT_CONSTRUCTION) template template sp::sp(U* other) : m_ptr(other) { @@ -230,6 +264,7 @@ sp::sp(U* other) (static_cast(other))->incStrong(this); } } +#endif template template sp::sp(const sp& other) @@ -272,6 +307,7 @@ sp& sp::operator=(sp&& other) noexcept { return *this; } +#if !defined(ANDROID_UTILS_REF_BASE_DISABLE_IMPLICIT_CONSTRUCTION) template sp& sp::operator =(T* other) { T* oldPtr(*const_cast(&m_ptr)); @@ -284,6 +320,7 @@ sp& sp::operator =(T* other) { m_ptr = other; return *this; } +#endif template template sp& sp::operator =(const sp& other) { @@ -306,6 +343,7 @@ sp& sp::operator =(sp&& other) { return *this; } +#if !defined(ANDROID_UTILS_REF_BASE_DISABLE_IMPLICIT_CONSTRUCTION) template template sp& sp::operator =(U* other) { T* oldPtr(*const_cast(&m_ptr)); @@ -315,6 +353,7 @@ sp& sp::operator =(U* other) { m_ptr = other; return *this; } +#endif template void sp::force_set(T* other) {