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

Commit 029b12eb authored by Hans Boehm's avatar Hans Boehm
Browse files

Fix wp and sp comparison bugs 

Make clear() actually clear wp m_refs, so that nulls compare equal.

Make equality consistent with < and >, ensuring that a weak pointer
cannot be both equal to and greater than another.

Don't rely on the built-in < and > operators to correctly order
different objects. The standard does not guarantee that, and there is
a risk of compiler relying on that lack of guarantee.

Remove unnecessary comparison overloads, especially those
comparing a wp<> to an sp<>.

Change the remaining wp<> to sp<> comparisons to check for equivalence
of the mRefs pointer instead of the object address, thus eliminating
the dubious equal comparison result for a dead wp<> and an sp<> that
happen to point to the same object address.

Add comparison tests.

Test: Treehugger, boot AOSP, atest RefBase
Bug: 126922090
Change-Id: I15911150e0fc85ace2c4b77d337826e12793c690
parent 949a561c

libutils/RefBase_test.cpp

+123 −0
Original line number Diff line number Diff line
@@ -45,6 +45,44 @@ private: 
    bool* mDeleted;

};



// A version of Foo that ensures that all objects are allocated at the same

// address. No more than one can be allocated at a time. Thread-hostile.

class FooFixedAlloc : public RefBase {

public:

    static void* operator new(size_t size) {

        if (mAllocCount != 0) {

            abort();

        }

        mAllocCount = 1;

        if (theMemory == nullptr) {

            theMemory = malloc(size);

        }

        return theMemory;

    }



    static void operator delete(void *p) {

        if (mAllocCount != 1 || p != theMemory) {

            abort();

        }

        mAllocCount = 0;

    }



    FooFixedAlloc(bool* deleted_check) : mDeleted(deleted_check) {

        *mDeleted = false;

    }



    ~FooFixedAlloc() {

        *mDeleted = true;

    }

private:

    bool* mDeleted;

    static int mAllocCount;

    static void* theMemory;

};



int FooFixedAlloc::mAllocCount(0);

void* FooFixedAlloc::theMemory(nullptr);



TEST(RefBase, StrongMoves) {

    bool isDeleted;

    Foo* foo = new Foo(&isDeleted);
@@ -90,6 +128,91 @@ TEST(RefBase, WeakCopies) { 
    ASSERT_FALSE(isDeleted) << "Deletion on wp destruction should no longer occur";

}



TEST(RefBase, Comparisons) {

    bool isDeleted, isDeleted2;

    Foo* foo = new Foo(&isDeleted);

    Foo* foo2 = new Foo(&isDeleted2);

    sp<Foo> sp1(foo);

    sp<Foo> sp2(foo2);

    wp<Foo> wp1(sp1);

    wp<Foo> wp2(sp1);

    wp<Foo> wp3(sp2);

    ASSERT_TRUE(wp1 == wp2);

    ASSERT_TRUE(wp1 == sp1);

    ASSERT_TRUE(wp3 == sp2);

    ASSERT_TRUE(wp1 != sp2);

    ASSERT_TRUE(wp1 <= wp2);

    ASSERT_TRUE(wp1 >= wp2);

    ASSERT_FALSE(wp1 != wp2);

    ASSERT_FALSE(wp1 > wp2);

    ASSERT_FALSE(wp1 < wp2);

    ASSERT_FALSE(sp1 == sp2);

    ASSERT_TRUE(sp1 != sp2);

    bool sp1_smaller = sp1 < sp2;

    wp<Foo>wp_smaller = sp1_smaller ? wp1 : wp3;

    wp<Foo>wp_larger = sp1_smaller ? wp3 : wp1;

    ASSERT_TRUE(wp_smaller < wp_larger);

    ASSERT_TRUE(wp_smaller != wp_larger);

    ASSERT_TRUE(wp_smaller <= wp_larger);

    ASSERT_FALSE(wp_smaller == wp_larger);

    ASSERT_FALSE(wp_smaller > wp_larger);

    ASSERT_FALSE(wp_smaller >= wp_larger);

    sp2 = nullptr;

    ASSERT_TRUE(isDeleted2);

    ASSERT_FALSE(isDeleted);

    ASSERT_FALSE(wp3 == sp2);

    // Comparison results on weak pointers should not be affected.

    ASSERT_TRUE(wp_smaller < wp_larger);

    ASSERT_TRUE(wp_smaller != wp_larger);

    ASSERT_TRUE(wp_smaller <= wp_larger);

    ASSERT_FALSE(wp_smaller == wp_larger);

    ASSERT_FALSE(wp_smaller > wp_larger);

    ASSERT_FALSE(wp_smaller >= wp_larger);

    wp2 = nullptr;

    ASSERT_FALSE(wp1 == wp2);

    ASSERT_TRUE(wp1 != wp2);

    wp1.clear();

    ASSERT_TRUE(wp1 == wp2);

    ASSERT_FALSE(wp1 != wp2);

    wp3.clear();

    ASSERT_TRUE(wp1 == wp3);

    ASSERT_FALSE(wp1 != wp3);

    ASSERT_FALSE(isDeleted);

    sp1.clear();

    ASSERT_TRUE(isDeleted);

    ASSERT_TRUE(sp1 == sp2);

}



// Check whether comparison against dead wp works, even if the object referenced

// by the new wp happens to be at the same address.

TEST(RefBase, ReplacedComparison) {

    bool isDeleted, isDeleted2;

    FooFixedAlloc* foo = new FooFixedAlloc(&isDeleted);

    sp<FooFixedAlloc> sp1(foo);

    wp<FooFixedAlloc> wp1(sp1);

    ASSERT_TRUE(wp1 == sp1);

    sp1.clear();  // Deallocates the object.

    ASSERT_TRUE(isDeleted);

    FooFixedAlloc* foo2 = new FooFixedAlloc(&isDeleted2);

    ASSERT_FALSE(isDeleted2);

    ASSERT_EQ(foo, foo2);  // Not technically a legal comparison, but ...

    sp<FooFixedAlloc> sp2(foo2);

    wp<FooFixedAlloc> wp2(sp2);

    ASSERT_TRUE(sp2 == wp2);

    ASSERT_FALSE(sp2 != wp2);

    ASSERT_TRUE(sp2 != wp1);

    ASSERT_FALSE(sp2 == wp1);

    ASSERT_FALSE(sp2 == sp1);  // sp1 is null.

    ASSERT_FALSE(wp1 == wp2);  // wp1 refers to old object.

    ASSERT_TRUE(wp1 != wp2);

    ASSERT_TRUE(wp1 > wp2 || wp1 < wp2);

    ASSERT_TRUE(wp1 >= wp2 || wp1 <= wp2);

    ASSERT_FALSE(wp1 >= wp2 && wp1 <= wp2);

    ASSERT_FALSE(wp1 == nullptr);

    wp1 = sp2;

    ASSERT_TRUE(wp1 == wp2);

    ASSERT_FALSE(wp1 != wp2);

}



// Set up a situation in which we race with visit2AndRremove() to delete

// 2 strong references.  Bar destructor checks that there are no early

libutils/include/utils/RefBase.h

+66 −28
Original line number Diff line number Diff line
@@ -171,6 +171,8 @@ 
#define ANDROID_REF_BASE_H



#include <atomic>

#include <functional>

#include <type_traits>  // for common_type.



#include <stdint.h>

#include <sys/types.h>
@@ -192,19 +194,26 @@ TextOutput& printWeakPointer(TextOutput& to, const void* val); 
// ---------------------------------------------------------------------------



#define COMPARE_WEAK(_op_)                                      \

inline bool operator _op_ (const sp<T>& o) const {              \

    return m_ptr _op_ o.m_ptr;                                  \

template<typename U>                                            \

inline bool operator _op_ (const U* o) const {                  \

    return m_ptr _op_ o;                                        \

}                                                               \

/* Needed to handle type inference for nullptr: */              \

inline bool operator _op_ (const T* o) const {                  \

    return m_ptr _op_ o;                                        \

}                                                               \

template<typename U>                                            \

inline bool operator _op_ (const sp<U>& o) const {              \

    return m_ptr _op_ o.m_ptr;                                  \

}                                                               \

}



template<template<typename C> class comparator, typename T, typename U>

static inline bool _wp_compare_(T* a, U* b) {

    return comparator<typename std::common_type<T*, U*>::type>()(a, b);

}



// Use std::less and friends to avoid undefined behavior when ordering pointers

// to different objects.

#define COMPARE_WEAK_FUNCTIONAL(_op_, _compare_)                 \

template<typename U>                                             \

inline bool operator _op_ (const U* o) const {                   \

    return m_ptr _op_ o;                                        \

    return _wp_compare_<_compare_>(m_ptr, o);                    \

}



// ---------------------------------------------------------------------------
@@ -395,39 +404,51 @@ public: 


    COMPARE_WEAK(==)

    COMPARE_WEAK(!=)

    COMPARE_WEAK(>)

    COMPARE_WEAK(<)

    COMPARE_WEAK(<=)

    COMPARE_WEAK(>=)

    COMPARE_WEAK_FUNCTIONAL(>, std::greater)

    COMPARE_WEAK_FUNCTIONAL(<, std::less)

    COMPARE_WEAK_FUNCTIONAL(<=, std::less_equal)

    COMPARE_WEAK_FUNCTIONAL(>=, std::greater_equal)



    inline bool operator == (const wp<T>& o) const {

        return (m_ptr == o.m_ptr) && (m_refs == o.m_refs);

    }

    template<typename U>

    inline bool operator == (const wp<U>& o) const {

        return m_ptr == o.m_ptr;

        return m_refs == o.m_refs;  // Implies m_ptr == o.mptr; see invariants below.

    }



    inline bool operator > (const wp<T>& o) const {

        return (m_ptr == o.m_ptr) ? (m_refs > o.m_refs) : (m_ptr > o.m_ptr);

    template<typename U>

    inline bool operator == (const sp<U>& o) const {

        // Just comparing m_ptr fields is often dangerous, since wp<> may refer to an older

        // object at the same address.

        if (o == nullptr) {

          return m_ptr == nullptr;

        } else {

          return m_refs == o->getWeakRefs();  // Implies m_ptr == o.mptr.

        }

    }



    template<typename U>

    inline bool operator > (const wp<U>& o) const {

        return (m_ptr == o.m_ptr) ? (m_refs > o.m_refs) : (m_ptr > o.m_ptr);

    inline bool operator != (const sp<U>& o) const {

        return !(*this == o);

    }



    inline bool operator < (const wp<T>& o) const {

        return (m_ptr == o.m_ptr) ? (m_refs < o.m_refs) : (m_ptr < o.m_ptr);

    template<typename U>

    inline bool operator > (const wp<U>& o) const {

        if (m_ptr == o.m_ptr) {

            return _wp_compare_<std::greater>(m_refs, o.m_refs);

        } else {

            return _wp_compare_<std::greater>(m_ptr, o.m_ptr);

        }

    }



    template<typename U>

    inline bool operator < (const wp<U>& o) const {

        return (m_ptr == o.m_ptr) ? (m_refs < o.m_refs) : (m_ptr < o.m_ptr);

        if (m_ptr == o.m_ptr) {

            return _wp_compare_<std::less>(m_refs, o.m_refs);

        } else {

            return _wp_compare_<std::less>(m_ptr, o.m_ptr);

        }

    }

                         inline bool operator != (const wp<T>& o) const { return m_refs != o.m_refs; }

    template<typename U> inline bool operator != (const wp<U>& o) const { return !operator == (o); }

                         inline bool operator <= (const wp<T>& o) const { return !operator > (o); }

    template<typename U> inline bool operator <= (const wp<U>& o) const { return !operator > (o); }

                         inline bool operator >= (const wp<T>& o) const { return !operator < (o); }

    template<typename U> inline bool operator >= (const wp<U>& o) const { return !operator < (o); }



private:
@@ -446,6 +467,22 @@ TextOutput& operator<<(TextOutput& to, const wp<T>& val); 
// ---------------------------------------------------------------------------

// No user serviceable parts below here.



// Implementation invariants:

// Either

// 1) m_ptr and m_refs are both null, or

// 2) m_refs == m_ptr->mRefs, or

// 3) *m_ptr is no longer live, and m_refs points to the weakref_type object that corresponded

//    to m_ptr while it was live. *m_refs remains live while a wp<> refers to it.

//

// The m_refs field in a RefBase object is allocated on construction, unique to that RefBase

// object, and never changes. Thus if two wp's have identical m_refs fields, they are either both

// null or point to the same object. If two wp's have identical m_ptr fields, they either both

// point to the same live object and thus have the same m_ref fields, or at least one of the

// objects is no longer live.

//

// 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* other)

    : m_ptr(other)
@@ -595,6 +632,7 @@ void wp<T>::clear() 
{

    if (m_ptr) {

        m_refs->decWeak(this);

        m_refs = 0;

        m_ptr = 0;

    }

}

libutils/include/utils/StrongPointer.h

+45 −19
Original line number Diff line number Diff line
@@ -17,6 +17,9 @@ 
#ifndef ANDROID_STRONG_POINTER_H

#define ANDROID_STRONG_POINTER_H



#include <functional>

#include <type_traits>  // for common_type.



// ---------------------------------------------------------------------------

namespace android {
@@ -24,13 +27,12 @@ template<typename T> class wp; 


// ---------------------------------------------------------------------------



#define COMPARE(_op_)                                           \

inline bool operator _op_ (const sp<T>& o) const {              \

    return m_ptr _op_ o.m_ptr;                                  \

}                                                               \

inline bool operator _op_ (const T* o) const {                  \

    return m_ptr _op_ o;                                        \

}                                                               \

// TODO: Maybe remove sp<> ? wp<> comparison? These are dangerous: If the wp<>

// was created before the sp<>, and they point to different objects, they may

// compare equal even if they are entirely unrelated. E.g. CameraService

// currently performa such comparisons.



#define COMPARE_STRONG(_op_)                                           \

template<typename U>                                            \

inline bool operator _op_ (const sp<U>& o) const {              \

    return m_ptr _op_ o.m_ptr;                                  \
@@ -39,14 +41,27 @@ template<typename U> \ 
inline bool operator _op_ (const U* o) const {                  \

    return m_ptr _op_ o;                                        \

}                                                               \

inline bool operator _op_ (const wp<T>& o) const {              \

    return m_ptr _op_ o.m_ptr;                                  \

/* Needed to handle type inference for nullptr: */              \

inline bool operator _op_ (const T* o) const {                  \

    return m_ptr _op_ o;                                        \

}



template<template<typename C> class comparator, typename T, typename U>

static inline bool _sp_compare_(T* a, U* b) {

    return comparator<typename std::common_type<T*, U*>::type>()(a, b);

}



// Use std::less and friends to avoid undefined behavior when ordering pointers

// to different objects.

#define COMPARE_STRONG_FUNCTIONAL(_op_, _compare_)               \

template<typename U>                                             \

inline bool operator _op_ (const sp<U>& o) const {               \

    return _sp_compare_<_compare_>(m_ptr, o.m_ptr);              \

}                                                                \

template<typename U>                                             \

inline bool operator _op_ (const wp<U>& o) const {              \

    return m_ptr _op_ o.m_ptr;                                  \

inline bool operator _op_ (const U* o) const {                   \

    return _sp_compare_<_compare_>(m_ptr, o);                    \

}



// ---------------------------------------------------------------------------



template<typename T>
@@ -89,12 +104,23 @@ public: 


    // Operators



    COMPARE(==)

    COMPARE(!=)

    COMPARE(>)

    COMPARE(<)

    COMPARE(<=)

    COMPARE(>=)

    COMPARE_STRONG(==)

    COMPARE_STRONG(!=)

    COMPARE_STRONG_FUNCTIONAL(>, std::greater)

    COMPARE_STRONG_FUNCTIONAL(<, std::less)

    COMPARE_STRONG_FUNCTIONAL(<=, std::less_equal)

    COMPARE_STRONG_FUNCTIONAL(>=, std::greater_equal)



    // Punt these to the wp<> implementation.

    template<typename U>

    inline bool operator == (const wp<U>& o) const {

        return o == *this;

    }



    template<typename U>

    inline bool operator != (const wp<U>& o) const {

        return o != *this;

    }



private:    

    template<typename Y> friend class sp;