Merge "ANDROID_UTILS_REF_BASE_DISABLE_IMPLICIT_CONSTRUCTION" am: 99037347d6 am: d8c4c860ef am: e1a51daf9b

Original change: https://android-review.googlesource.com/c/platform/system/core/+/1660499

Change-Id: I72e661607a70876fa8f63f95318c3805cb26fb4b

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<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)
 {

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> 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.

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 @@ 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<SPFoo>::fromExisting(foo), "");
+
+    delete foo;
+}

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 @@ 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<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 @@ public:
 
     // 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 @@ 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 <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 @@ wp<T>::wp(const sp<T>& other)
     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 @@ wp<T>::~wp()
     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 @@ wp<T>& wp<T>::operator = (T* other)
     m_refs = newRefs;
     return *this;
 }
+#endif
 
 template<typename T>
 wp<T>& wp<T>::operator = (const wp<T>& other)
@@ -569,6 +645,7 @@ wp<T>& wp<T>::operator = (const sp<T>& other)
     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 @@ wp<T>& wp<T>::operator = (U* other)
     m_refs = newRefs;
     return *this;
 }
+#endif
 
 template<typename T> template<typename U>
 wp<T>& wp<T>::operator = (const wp<U>& other)

libutils/include/utils/StrongPointer.h

@@ -50,10 +50,25 @@ public:
     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 @@ public:
 
     // 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 @@ sp<T> sp<T>::make(Args&&... args) {
     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 @@ sp<T>::sp(T* other)
         other->incStrong(this);
     }
 }
+#endif
 
 template<typename T>
 sp<T>::sp(const sp<T>& other)
@@ -222,6 +255,7 @@ sp<T>::sp(sp<T>&& other) noexcept : m_ptr(other.m_ptr) {
     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 @@ sp<T>::sp(U* other)
         (static_cast<T*>(other))->incStrong(this);
     }
 }
+#endif
 
 template<typename T> template<typename U>
 sp<T>::sp(const sp<U>& other)
@@ -272,6 +307,7 @@ sp<T>& sp<T>::operator=(sp<T>&& other) noexcept {
     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 @@ sp<T>& sp<T>::operator =(T* other) {
     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 @@ sp<T>& sp<T>::operator =(sp<U>&& other) {
     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 @@ sp<T>& sp<T>::operator =(U* other) {
     m_ptr = other;
     return *this;
 }
+#endif
 
 template<typename T>
 void sp<T>::force_set(T* other) {