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<T>(this)`, you would now call
`sp<T>::assertStrongRefExists(this)`, and the double ownership
problem would become a runtime error.
Bug: 184190315
Test: libutils_test
Change-Id: Ie18d3146420df1808e3733027070ec234dda4e9d
diff --git a/libutils/RefBase.cpp b/libutils/RefBase.cpp
index 8e45226..b57e287 100644
--- a/libutils/RefBase.cpp
+++ b/libutils/RefBase.cpp
@@ -443,6 +443,20 @@
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 @@
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<weakref_impl*>(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 c9b4894..dcc469e 100644
--- a/libutils/RefBase_test.cpp
+++ b/libutils/RefBase_test.cpp
@@ -241,6 +241,30 @@
ASSERT_FALSE(wp1 != wp2);
}
+TEST(RefBase, AssertWeakRefExistsSuccess) {
+ // uses some other refcounting method, or non at all
+ bool isDeleted;
+ sp<Foo> foo = sp<Foo>::make(&isDeleted);
+ wp<Foo> weakFoo = foo;
+
+ EXPECT_EQ(weakFoo, wp<Foo>::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<Foo>::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 d37c1de..29f6bd4 100644
--- a/libutils/StrongPointer_test.cpp
+++ b/libutils/StrongPointer_test.cpp
@@ -21,8 +21,8 @@
using namespace android;
-class SPFoo : public LightRefBase<SPFoo> {
-public:
+class SPFoo : virtual public RefBase {
+ public:
explicit SPFoo(bool* deleted_check) : mDeleted(deleted_check) {
*mDeleted = false;
}
@@ -69,3 +69,14 @@
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<SPFoo>::fromExisting(foo), "");
+
+ delete foo;
+}
diff --git a/libutils/include/utils/RefBase.h b/libutils/include/utils/RefBase.h
index e7acd17..5a5bd56 100644
--- a/libutils/include/utils/RefBase.h
+++ b/libutils/include/utils/RefBase.h
@@ -140,7 +140,9 @@
// count, and accidentally passed to f(sp<T>), 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<T*>.
+// 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<Foo> myFoo = sp<Foo>::make(/*args*/);
+//
+// // if you need a weak pointer, it must be constructed from a strong
+// // pointer
+// wp<Foo> 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<Foo> thiz = sp<Foo>::fromExisting(this);
+// }
+//
+
#ifndef ANDROID_REF_BASE_H
#define ANDROID_REF_BASE_H
@@ -244,6 +282,7 @@
{
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 @@
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 @@
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<T> 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<T>& other);
explicit wp(const sp<T>& other);
+
+#if !defined(ANDROID_UTILS_REF_BASE_DISABLE_IMPLICIT_CONSTRUCTION)
template<typename U> wp(U* other); // NOLINT(implicit)
+#endif
template<typename U> wp(const sp<U>& other); // NOLINT(implicit)
template<typename U> wp(const wp<U>& other); // NOLINT(implicit)
@@ -376,11 +430,15 @@
// Assignment
+#if !defined(ANDROID_UTILS_REF_BASE_DISABLE_IMPLICIT_CONSTRUCTION)
wp& operator = (T* other);
+#endif
wp& operator = (const wp<T>& other);
wp& operator = (const sp<T>& other);
+#if !defined(ANDROID_UTILS_REF_BASE_DISABLE_IMPLICIT_CONSTRUCTION)
template<typename U> wp& operator = (U* other);
+#endif
template<typename U> wp& operator = (const wp<U>& other);
template<typename U> wp& operator = (const sp<U>& other);
@@ -481,12 +539,26 @@
// 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 <typename T>
+wp<T> wp<T>::fromExisting(T* other) {
+ if (!other) return nullptr;
+
+ auto refs = other->getWeakRefs();
+ refs->incWeakRequireWeak(other);
+
+ wp<T> ret;
+ ret.m_refs = refs;
+ return ret;
+}
+
+#if !defined(ANDROID_UTILS_REF_BASE_DISABLE_IMPLICIT_CONSTRUCTION)
template<typename T>
wp<T>::wp(T* other)
: m_ptr(other)
{
m_refs = other ? m_refs = other->createWeak(this) : nullptr;
}
+#endif
template<typename T>
wp<T>::wp(const wp<T>& other)
@@ -502,12 +574,14 @@
m_refs = m_ptr ? m_ptr->createWeak(this) : nullptr;
}
+#if !defined(ANDROID_UTILS_REF_BASE_DISABLE_IMPLICIT_CONSTRUCTION)
template<typename T> template<typename U>
wp<T>::wp(U* other)
: m_ptr(other)
{
m_refs = other ? other->createWeak(this) : nullptr;
}
+#endif
template<typename T> template<typename U>
wp<T>::wp(const wp<U>& other)
@@ -534,6 +608,7 @@
if (m_ptr) m_refs->decWeak(this);
}
+#if !defined(ANDROID_UTILS_REF_BASE_DISABLE_IMPLICIT_CONSTRUCTION)
template<typename T>
wp<T>& wp<T>::operator = (T* other)
{
@@ -544,6 +619,7 @@
m_refs = newRefs;
return *this;
}
+#endif
template<typename T>
wp<T>& wp<T>::operator = (const wp<T>& other)
@@ -569,6 +645,7 @@
return *this;
}
+#if !defined(ANDROID_UTILS_REF_BASE_DISABLE_IMPLICIT_CONSTRUCTION)
template<typename T> template<typename U>
wp<T>& wp<T>::operator = (U* other)
{
@@ -579,6 +656,7 @@
m_refs = newRefs;
return *this;
}
+#endif
template<typename T> template<typename U>
wp<T>& wp<T>::operator = (const wp<U>& other)
diff --git a/libutils/include/utils/StrongPointer.h b/libutils/include/utils/StrongPointer.h
index 1fa46fe..1f07052 100644
--- a/libutils/include/utils/StrongPointer.h
+++ b/libutils/include/utils/StrongPointer.h
@@ -50,10 +50,25 @@
template <typename... Args>
static inline sp<T> 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<T> 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<T>& other);
sp(sp<T>&& other) noexcept;
+
+#if !defined(ANDROID_UTILS_REF_BASE_DISABLE_IMPLICIT_CONSTRUCTION)
template<typename U> sp(U* other); // NOLINT(implicit)
+#endif
template<typename U> sp(const sp<U>& other); // NOLINT(implicit)
template<typename U> sp(sp<U>&& other); // NOLINT(implicit)
@@ -61,13 +76,17 @@
// Assignment
+#if !defined(ANDROID_UTILS_REF_BASE_DISABLE_IMPLICIT_CONSTRUCTION)
sp& operator = (T* other);
+#endif
sp& operator = (const sp<T>& other);
sp& operator=(sp<T>&& other) noexcept;
template<typename U> sp& operator = (const sp<U>& other);
template<typename U> sp& operator = (sp<U>&& other);
+#if !defined(ANDROID_UTILS_REF_BASE_DISABLE_IMPLICIT_CONSTRUCTION)
template<typename U> sp& operator = (U* other);
+#endif
//! Special optimization for use by ProcessState (and nobody else).
void force_set(T* other);
@@ -201,6 +220,19 @@
return result;
}
+template <typename T>
+sp<T> sp<T>::fromExisting(T* other) {
+ if (other) {
+ check_not_on_stack(other);
+ other->incStrongRequireStrong(other);
+ sp<T> result;
+ result.m_ptr = other;
+ return result;
+ }
+ return nullptr;
+}
+
+#if !defined(ANDROID_UTILS_REF_BASE_DISABLE_IMPLICIT_CONSTRUCTION)
template<typename T>
sp<T>::sp(T* other)
: m_ptr(other) {
@@ -209,6 +241,7 @@
other->incStrong(this);
}
}
+#endif
template<typename T>
sp<T>::sp(const sp<T>& other)
@@ -222,6 +255,7 @@
other.m_ptr = nullptr;
}
+#if !defined(ANDROID_UTILS_REF_BASE_DISABLE_IMPLICIT_CONSTRUCTION)
template<typename T> template<typename U>
sp<T>::sp(U* other)
: m_ptr(other) {
@@ -230,6 +264,7 @@
(static_cast<T*>(other))->incStrong(this);
}
}
+#endif
template<typename T> template<typename U>
sp<T>::sp(const sp<U>& other)
@@ -272,6 +307,7 @@
return *this;
}
+#if !defined(ANDROID_UTILS_REF_BASE_DISABLE_IMPLICIT_CONSTRUCTION)
template<typename T>
sp<T>& sp<T>::operator =(T* other) {
T* oldPtr(*const_cast<T* volatile*>(&m_ptr));
@@ -284,6 +320,7 @@
m_ptr = other;
return *this;
}
+#endif
template<typename T> template<typename U>
sp<T>& sp<T>::operator =(const sp<U>& other) {
@@ -306,6 +343,7 @@
return *this;
}
+#if !defined(ANDROID_UTILS_REF_BASE_DISABLE_IMPLICIT_CONSTRUCTION)
template<typename T> template<typename U>
sp<T>& sp<T>::operator =(U* other) {
T* oldPtr(*const_cast<T* volatile*>(&m_ptr));
@@ -315,6 +353,7 @@
m_ptr = other;
return *this;
}
+#endif
template<typename T>
void sp<T>::force_set(T* other) {