Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit da75cef9 authored by Steven Moreland's avatar Steven Moreland
Browse files

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
parent 767f264f

libutils/RefBase.cpp

+22 −0
Original line number Diff line number Diff line
@@ -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

+24 −0
Original line number Diff line number Diff line
@@ -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

+13 −2
Original line number Diff line number Diff line
@@ -21,7 +21,7 @@ 


using namespace android;



class SPFoo : public LightRefBase<SPFoo> {

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

+79 −1
Original line number Diff line number Diff line
@@ -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

+39 −0
Original line number Diff line number Diff line
@@ -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) {