From 6e75ad6e137e8cbae0a70b1fc2955d3f5ca51447 Mon Sep 17 00:00:00 2001 From: Hans Boehm Date: Wed, 13 Mar 2019 17:34:46 +0000 Subject: [PATCH] Revert "Revert "Fix wp and sp comparison bugs"" 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. This reverts commit a2a2ad805775ea88f25388677aa37e0492a492c4. The original code, and my original CL, both failed to initialize m_refs in various wp<> constructors. This now became more important, since comparisons now rely more on m_refs. However I believe it was always a bug, since some comparisons always relied on m_refs. Test: Treehugger, boot AOSP, atest RefBase Bug: 126922090 This reverts commit a2a2ad805775ea88f25388677aa37e0492a492c4. Reason for revert: Reapply after constructor fixes. Change-Id: I2c8917416a2306e36d2b6bb7b397f653020e5688 --- libutils/RefBase_test.cpp | 150 +++++++++++++++++++++++++ libutils/include/utils/RefBase.h | 110 ++++++++++++------ libutils/include/utils/StrongPointer.h | 64 +++++++---- 3 files changed, 268 insertions(+), 56 deletions(-) diff --git a/libutils/RefBase_test.cpp b/libutils/RefBase_test.cpp index 2e0cf6e46..c9b489423 100644 --- a/libutils/RefBase_test.cpp +++ b/libutils/RefBase_test.cpp @@ -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,118 @@ TEST(RefBase, WeakCopies) { ASSERT_FALSE(isDeleted) << "Deletion on wp destruction should no longer occur"; } +TEST(RefBase, Comparisons) { + bool isDeleted, isDeleted2, isDeleted3; + Foo* foo = new Foo(&isDeleted); + Foo* foo2 = new Foo(&isDeleted2); + sp sp1(foo); + sp sp2(foo2); + wp wp1(sp1); + wp wp2(sp1); + wp 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; + wpwp_smaller = sp1_smaller ? wp1 : wp3; + wpwp_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); + // Try to check that null pointers are properly initialized. + { + // Try once with non-null, to maximize chances of getting junk on the + // stack. + sp sp3(new Foo(&isDeleted3)); + wp wp4(sp3); + wp wp5; + ASSERT_FALSE(wp4 == wp5); + ASSERT_TRUE(wp4 != wp5); + ASSERT_FALSE(sp3 == wp5); + ASSERT_FALSE(wp5 == sp3); + ASSERT_TRUE(sp3 != wp5); + ASSERT_TRUE(wp5 != sp3); + ASSERT_TRUE(sp3 == wp4); + } + { + sp sp3; + wp wp4(sp3); + wp wp5; + ASSERT_TRUE(wp4 == wp5); + ASSERT_FALSE(wp4 != wp5); + ASSERT_TRUE(sp3 == wp5); + ASSERT_TRUE(wp5 == sp3); + ASSERT_FALSE(sp3 != wp5); + ASSERT_FALSE(wp5 != sp3); + ASSERT_TRUE(sp3 == wp4); + } +} + +// 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 sp1(foo); + wp 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 sp2(foo2); + wp 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 diff --git a/libutils/include/utils/RefBase.h b/libutils/include/utils/RefBase.h index 1780cf22f..a105474bb 100644 --- a/libutils/include/utils/RefBase.h +++ b/libutils/include/utils/RefBase.h @@ -171,6 +171,8 @@ #define ANDROID_REF_BASE_H #include +#include +#include // for common_type. #include #include @@ -192,19 +194,26 @@ TextOutput& printWeakPointer(TextOutput& to, const void* val); // --------------------------------------------------------------------------- #define COMPARE_WEAK(_op_) \ -inline bool operator _op_ (const sp& o) const { \ - return m_ptr _op_ o.m_ptr; \ -} \ -inline bool operator _op_ (const T* o) const { \ - return m_ptr _op_ o; \ -} \ -template \ -inline bool operator _op_ (const sp& o) const { \ - return m_ptr _op_ o.m_ptr; \ -} \ template \ 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 class comparator, typename T, typename U> +static inline bool _wp_compare_(T* a, U* b) { + return comparator::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 \ +inline bool operator _op_ (const U* o) const { \ + return _wp_compare_<_compare_>(m_ptr, o); \ } // --------------------------------------------------------------------------- @@ -354,7 +363,7 @@ class wp public: typedef typename RefBase::weakref_type weakref_type; - inline wp() : m_ptr(nullptr) { } + inline wp() : m_ptr(nullptr), m_refs(nullptr) { } wp(T* other); // NOLINT(implicit) wp(const wp& other); @@ -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& o) const { - return (m_ptr == o.m_ptr) && (m_refs == o.m_refs); - } template inline bool operator == (const wp& 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& o) const { - return (m_ptr == o.m_ptr) ? (m_refs > o.m_refs) : (m_ptr > o.m_ptr); + template + inline bool operator == (const sp& 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 + inline bool operator != (const sp& o) const { + return !(*this == o); + } + template inline bool operator > (const wp& 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_(m_refs, o.m_refs); + } else { + return _wp_compare_(m_ptr, o.m_ptr); + } } - inline bool operator < (const wp& o) const { - return (m_ptr == o.m_ptr) ? (m_refs < o.m_refs) : (m_ptr < o.m_ptr); - } template inline bool operator < (const wp& 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_(m_refs, o.m_refs); + } else { + return _wp_compare_(m_ptr, o.m_ptr); + } } - inline bool operator != (const wp& o) const { return m_refs != o.m_refs; } template inline bool operator != (const wp& o) const { return !operator == (o); } - inline bool operator <= (const wp& o) const { return !operator > (o); } template inline bool operator <= (const wp& o) const { return !operator > (o); } - inline bool operator >= (const wp& o) const { return !operator < (o); } template inline bool operator >= (const wp& o) const { return !operator < (o); } private: @@ -446,11 +467,27 @@ TextOutput& operator<<(TextOutput& to, const wp& 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 wp::wp(T* other) : m_ptr(other) { - if (other) m_refs = other->createWeak(this); + m_refs = other ? m_refs = other->createWeak(this) : nullptr; } template @@ -464,16 +501,14 @@ template wp::wp(const sp& other) : m_ptr(other.m_ptr) { - if (m_ptr) { - m_refs = m_ptr->createWeak(this); - } + m_refs = m_ptr ? m_ptr->createWeak(this) : nullptr; } template template wp::wp(U* other) : m_ptr(other) { - if (other) m_refs = other->createWeak(this); + m_refs = other ? other->createWeak(this) : nullptr; } template template @@ -483,6 +518,8 @@ wp::wp(const wp& other) if (m_ptr) { m_refs = other.m_refs; m_refs->incWeak(this); + } else { + m_refs = nullptr; } } @@ -490,9 +527,7 @@ template template wp::wp(const sp& other) : m_ptr(other.m_ptr) { - if (m_ptr) { - m_refs = m_ptr->createWeak(this); - } + m_refs = m_ptr ? m_ptr->createWeak(this) : nullptr; } template @@ -595,6 +630,7 @@ void wp::clear() { if (m_ptr) { m_refs->decWeak(this); + m_refs = 0; m_ptr = 0; } } diff --git a/libutils/include/utils/StrongPointer.h b/libutils/include/utils/StrongPointer.h index 15711296d..9cd7c75fd 100644 --- a/libutils/include/utils/StrongPointer.h +++ b/libutils/include/utils/StrongPointer.h @@ -17,6 +17,9 @@ #ifndef ANDROID_STRONG_POINTER_H #define ANDROID_STRONG_POINTER_H +#include +#include // for common_type. + // --------------------------------------------------------------------------- namespace android { @@ -24,13 +27,12 @@ template class wp; // --------------------------------------------------------------------------- -#define COMPARE(_op_) \ -inline bool operator _op_ (const sp& 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 \ inline bool operator _op_ (const sp& o) const { \ return m_ptr _op_ o.m_ptr; \ @@ -39,14 +41,27 @@ template \ inline bool operator _op_ (const U* o) const { \ return m_ptr _op_ o; \ } \ -inline bool operator _op_ (const wp& o) const { \ - return m_ptr _op_ o.m_ptr; \ -} \ -template \ -inline bool operator _op_ (const wp& 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 class comparator, typename T, typename U> +static inline bool _sp_compare_(T* a, U* b) { + return comparator::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 \ +inline bool operator _op_ (const sp& o) const { \ + return _sp_compare_<_compare_>(m_ptr, o.m_ptr); \ +} \ +template \ +inline bool operator _op_ (const U* o) const { \ + return _sp_compare_<_compare_>(m_ptr, o); \ +} // --------------------------------------------------------------------------- template @@ -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 + inline bool operator == (const wp& o) const { + return o == *this; + } + + template + inline bool operator != (const wp& o) const { + return o != *this; + } private: template friend class sp;