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
diff --git a/libutils/RefBase_test.cpp b/libutils/RefBase_test.cpp
index 2e0cf6e..b594ca5 100644
--- a/libutils/RefBase_test.cpp
+++ b/libutils/RefBase_test.cpp
@@ -45,6 +45,44 @@
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 @@
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
diff --git a/libutils/include/utils/RefBase.h b/libutils/include/utils/RefBase.h
index 1780cf2..730d631 100644
--- a/libutils/include/utils/RefBase.h
+++ b/libutils/include/utils/RefBase.h
@@ -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 @@
// ---------------------------------------------------------------------------
#define COMPARE_WEAK(_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; \
-} \
-template<typename U> \
-inline bool operator _op_ (const sp<U>& 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<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 _wp_compare_<_compare_>(m_ptr, o); \
}
// ---------------------------------------------------------------------------
@@ -395,39 +404,51 @@
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 sp<U>& o) const {
+ return !(*this == o);
+ }
+
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::greater>(m_refs, o.m_refs);
+ } else {
+ return _wp_compare_<std::greater>(m_ptr, o.m_ptr);
+ }
}
- 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 {
- 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 @@
// ---------------------------------------------------------------------------
// 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 @@
{
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 1571129..9cd7c75 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 <functional>
+#include <type_traits> // for common_type.
+
// ---------------------------------------------------------------------------
namespace android {
@@ -24,13 +27,12 @@
// ---------------------------------------------------------------------------
-#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 @@
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; \
-} \
-template<typename U> \
-inline bool operator _op_ (const wp<U>& 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 U* o) const { \
+ return _sp_compare_<_compare_>(m_ptr, o); \
+}
// ---------------------------------------------------------------------------
template<typename T>
@@ -89,12 +104,23 @@
// 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;