diff --git a/include/utils/RefBase.h b/include/utils/RefBase.h index a232a657e..3c318c4b2 100644 --- a/include/utils/RefBase.h +++ b/include/utils/RefBase.h @@ -105,16 +105,14 @@ // Other more specific restrictions for wp<> and sp<>: -// Constructing a strong or weak pointer to "this" in its constructors is almost -// always wrong. In the case of strong pointers. it is always wrong with RefBase -// because the onFirstRef() callback will be mode on an incompletely constructed -// object. In either case, it is wrong if such a pointer does not outlive the -// constructor, since destruction of the smart pointer will attempt to destroy the -// object before construction is finished, normally resulting in a pointer to a -// destroyed object being returned from a new expression. - -// In the case of weak pointers, this occurs because an object that has never been -// referenced by a strong pointer is destroyed when the last weak pointer disappears. +// Do not construct a strong pointer to "this" in an object's constructor. +// The onFirstRef() callback would be made on an incompletely constructed +// object. +// Construction of a weak pointer to "this" in an object's constructor is also +// discouraged. But the implementation was recently changed so that, in the +// absence of extendObjectLifetime() calls, weak pointers no longer impact +// object lifetime, and hence this no longer risks premature deallocation, +// and hence usually works correctly. // Such strong or weak pointers can be safely created in the RefBase onFirstRef() // callback. @@ -126,8 +124,23 @@ // is a longer-lived sp<>, why not use an sp<> directly?) A wp<> should only be // dereferenced by using promote(). +// Any object inheriting from RefBase should always be destroyed as the result +// of a reference count decrement, not via any other means. Such objects +// should never be stack allocated, or appear directly as data members in other +// objects. Objects inheriting from RefBase should have their strong reference +// count incremented as soon as possible after construction. Usually this +// will be done via construction of an sp<> to the object, but may instead +// involve other means of calling RefBase::incStrong(). // Explicitly deleting or otherwise destroying a RefBase object with outstanding -// wp<> or sp<> pointers to it will result in heap corruption. +// wp<> or sp<> pointers to it will result in an abort or heap corruption. + +// It is particularly important not to mix sp<> and direct storage management +// since the sp from raw pointer constructor is implicit. Thus if a RefBase- +// -derived object of type T is managed without ever incrementing its strong +// count, and accidentally passed to f(sp), 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. // Extra Features: @@ -144,7 +157,7 @@ // events, as well as some debugging facilities. // Debugging support can be enabled by turning on DEBUG_REFS in RefBase.cpp. -// Otherwise essentially no checking is provided. +// Otherwise little checking is provided. // Thread safety: diff --git a/libutils/RefBase.cpp b/libutils/RefBase.cpp index df49a2f94..fee9984a7 100644 --- a/libutils/RefBase.cpp +++ b/libutils/RefBase.cpp @@ -84,15 +84,16 @@ namespace android { // // A weakref_impl is allocated as the value of mRefs in a RefBase object on // construction. -// In the OBJECT_LIFETIME_STRONG case, it is deallocated in the RefBase -// destructor iff the strong reference count was never incremented. The -// destructor can be invoked either from decStrong, or from decWeak if there -// was never a strong reference. If the reference count had been incremented, -// it is deallocated directly in decWeak, and hence still lives as long as -// the last weak reference. -// In the OBJECT_LIFETIME_WEAK case, it is always deallocated from the RefBase -// destructor, which is always invoked by decWeak. DecStrong explicitly avoids -// the deletion in this case. +// In the OBJECT_LIFETIME_STRONG case, it is normally deallocated in decWeak, +// and hence lives as long as the last weak reference. (It can also be +// deallocated in the RefBase destructor iff the strong reference count was +// never incremented and the weak count is zero, e.g. if the RefBase object is +// explicitly destroyed without decrementing the strong count. This should be +// avoided.) In this case, the RefBase destructor should be invoked from +// decStrong. +// In the OBJECT_LIFETIME_WEAK case, the weakref_impl is always deallocated in +// the RefBase destructor, which is always invoked by decWeak. DecStrong +// explicitly avoids the deletion in this case. // // Memory ordering: // The client must ensure that every inc() call, together with all other @@ -126,6 +127,19 @@ namespace android { #define INITIAL_STRONG_VALUE (1<<28) +#define MAX_COUNT 0xfffff + +// Test whether the argument is a clearly invalid strong reference count. +// Used only for error checking on the value before an atomic decrement. +// Intended to be very cheap. +// Note that we cannot just check for excess decrements by comparing to zero +// since the object would be deallocated before that. +#define BAD_STRONG(c) \ + ((c) == 0 || ((c) & (~(MAX_COUNT | INITIAL_STRONG_VALUE))) != 0) + +// Same for weak counts. +#define BAD_WEAK(c) ((c) == 0 || ((c) & (~MAX_COUNT)) != 0) + // --------------------------------------------------------------------------- class RefBase::weakref_impl : public RefBase::weakref_type @@ -421,15 +435,15 @@ void RefBase::decStrong(const void* id) const #if PRINT_REFS ALOGD("decStrong of %p from %p: cnt=%d\n", this, id, c); #endif - ALOG_ASSERT(c >= 1, "decStrong() called on %p too many times", refs); + LOG_ALWAYS_FATAL_IF(BAD_STRONG(c), "decStrong() called on %p too many times", + refs); if (c == 1) { std::atomic_thread_fence(std::memory_order_acquire); refs->mBase->onLastStrongRef(id); int32_t flags = refs->mFlags.load(std::memory_order_relaxed); if ((flags&OBJECT_LIFETIME_MASK) == OBJECT_LIFETIME_STRONG) { delete this; - // Since mStrong had been incremented, the destructor did not - // delete refs. + // The destructor does not delete refs in this case. } } // Note that even with only strong reference operations, the thread @@ -492,7 +506,8 @@ void RefBase::weakref_type::decWeak(const void* id) weakref_impl* const impl = static_cast(this); impl->removeWeakRef(id); const int32_t c = impl->mWeak.fetch_sub(1, std::memory_order_release); - ALOG_ASSERT(c >= 1, "decWeak called on %p too many times", this); + LOG_ALWAYS_FATAL_IF(BAD_WEAK(c), "decWeak called on %p too many times", + this); if (c != 1) return; atomic_thread_fence(std::memory_order_acquire); @@ -500,13 +515,19 @@ void RefBase::weakref_type::decWeak(const void* id) if ((flags&OBJECT_LIFETIME_MASK) == OBJECT_LIFETIME_STRONG) { // This is the regular lifetime case. The object is destroyed // when the last strong reference goes away. Since weakref_impl - // outlive the object, it is not destroyed in the dtor, and + // outlives the object, it is not destroyed in the dtor, and // we'll have to do it here. if (impl->mStrong.load(std::memory_order_relaxed) == INITIAL_STRONG_VALUE) { - // Special case: we never had a strong reference, so we need to - // destroy the object now. - delete impl->mBase; + // Decrementing a weak count to zero when object never had a strong + // reference. We assume it acquired a weak reference early, e.g. + // in the constructor, and will eventually be properly destroyed, + // usually via incrementing and decrementing the strong count. + // Thus we no longer do anything here. We log this case, since it + // seems to be extremely rare, and should not normally occur. We + // used to deallocate mBase here, so this may now indicate a leak. + ALOGW("RefBase: Object at %p lost last weak reference " + "before it had a strong reference", impl->mBase); } else { // ALOGV("Freeing refs %p of old RefBase %p\n", this, impl->mBase); delete impl; @@ -675,25 +696,28 @@ RefBase::RefBase() RefBase::~RefBase() { - if (mRefs->mStrong.load(std::memory_order_relaxed) + int32_t flags = mRefs->mFlags.load(std::memory_order_relaxed); + // Life-time of this object is extended to WEAK, in + // which case weakref_impl doesn't out-live the object and we + // can free it now. + if ((flags & OBJECT_LIFETIME_MASK) == OBJECT_LIFETIME_WEAK) { + // It's possible that the weak count is not 0 if the object + // re-acquired a weak reference in its destructor + if (mRefs->mWeak.load(std::memory_order_relaxed) == 0) { + delete mRefs; + } + } else if (mRefs->mStrong.load(std::memory_order_relaxed) == INITIAL_STRONG_VALUE) { // We never acquired a strong reference on this object. - // We assume there are no outstanding weak references. + LOG_ALWAYS_FATAL_IF(mRefs->mWeak.load() != 0, + "RefBase: Explicit destruction with non-zero weak " + "reference count"); + // TODO: Always report if we get here. Currently MediaMetadataRetriever + // C++ objects are inconsistently managed and sometimes get here. + // There may be other cases, but we believe they should all be fixed. delete mRefs; - } else { - // life-time of this object is extended to WEAK, in - // which case weakref_impl doesn't out-live the object and we - // can free it now. - int32_t flags = mRefs->mFlags.load(std::memory_order_relaxed); - if ((flags & OBJECT_LIFETIME_MASK) != OBJECT_LIFETIME_STRONG) { - // It's possible that the weak count is not 0 if the object - // re-acquired a weak reference in its destructor - if (mRefs->mWeak.load(std::memory_order_relaxed) == 0) { - delete mRefs; - } - } } - // for debugging purposes, clear this. + // For debugging purposes, clear mRefs. Ineffective against outstanding wp's. const_cast(mRefs) = NULL; } diff --git a/libutils/tests/RefBase_test.cpp b/libutils/tests/RefBase_test.cpp index 224c2ca72..2e0cf6e46 100644 --- a/libutils/tests/RefBase_test.cpp +++ b/libutils/tests/RefBase_test.cpp @@ -87,7 +87,7 @@ TEST(RefBase, WeakCopies) { EXPECT_EQ(1, foo->getWeakRefs()->getWeakCount()); ASSERT_FALSE(isDeleted) << "deleted too early! still has a reference!"; wp1 = nullptr; - ASSERT_TRUE(isDeleted) << "foo2 was leaked!"; + ASSERT_FALSE(isDeleted) << "Deletion on wp destruction should no longer occur"; } @@ -121,8 +121,33 @@ static inline void waitFor(bool val) { cpu_set_t otherCpus; +// Divide the cpus we're allowed to run on into myCpus and otherCpus. +// Set origCpus to the processors we were originally allowed to run on. +// Return false if origCpus doesn't include at least processors 0 and 1. +static bool setExclusiveCpus(cpu_set_t* origCpus /* out */, + cpu_set_t* myCpus /* out */, cpu_set_t* otherCpus) { + if (sched_getaffinity(0, sizeof(cpu_set_t), origCpus) != 0) { + return false; + } + if (!CPU_ISSET(0, origCpus) || !CPU_ISSET(1, origCpus)) { + return false; + } + CPU_ZERO(myCpus); + CPU_ZERO(otherCpus); + CPU_OR(myCpus, myCpus, origCpus); + CPU_OR(otherCpus, otherCpus, origCpus); + for (unsigned i = 0; i < CPU_SETSIZE; ++i) { + // I get the even cores, the other thread gets the odd ones. + if (i & 1) { + CPU_CLR(i, myCpus); + } else { + CPU_CLR(i, otherCpus); + } + } + return true; +} + static void visit2AndRemove() { - EXPECT_TRUE(CPU_ISSET(1, &otherCpus)); if (sched_setaffinity(0, sizeof(cpu_set_t), &otherCpus) != 0) { FAIL() << "setaffinity returned:" << errno; } @@ -139,27 +164,10 @@ TEST(RefBase, RacingDestructors) { cpu_set_t myCpus; // Restrict us and the helper thread to disjoint cpu sets. // This prevents us from getting scheduled against each other, - // which would be atrociously slow. We fail if that's impossible. - if (sched_getaffinity(0, sizeof(cpu_set_t), &origCpus) != 0) { - FAIL(); - } - EXPECT_TRUE(CPU_ISSET(0, &origCpus)); - if (CPU_ISSET(1, &origCpus)) { - CPU_ZERO(&myCpus); - CPU_ZERO(&otherCpus); - CPU_OR(&myCpus, &myCpus, &origCpus); - CPU_OR(&otherCpus, &otherCpus, &origCpus); - for (unsigned i = 0; i < CPU_SETSIZE; ++i) { - // I get the even cores, the other thread gets the odd ones. - if (i & 1) { - CPU_CLR(i, &myCpus); - } else { - CPU_CLR(i, &otherCpus); - } - } + // which would be atrociously slow. + if (setExclusiveCpus(&origCpus, &myCpus, &otherCpus)) { std::thread t(visit2AndRemove); std::atomic deleteCount(0); - EXPECT_TRUE(CPU_ISSET(0, &myCpus)); if (sched_setaffinity(0, sizeof(cpu_set_t), &myCpus) != 0) { FAIL() << "setaffinity returned:" << errno; } @@ -182,3 +190,69 @@ TEST(RefBase, RacingDestructors) { ASSERT_EQ(NITERS, deleteCount) << "Deletions missed!"; } // Otherwise this is slow and probably pointless on a uniprocessor. } + +static wp wpBuffer; +static std::atomic wpBufferFull(false); + +// Wait until wpBufferFull has value val. +static inline void wpWaitFor(bool val) { + while (wpBufferFull != val) {} +} + +static void visit3AndRemove() { + if (sched_setaffinity(0, sizeof(cpu_set_t), &otherCpus) != 0) { + FAIL() << "setaffinity returned:" << errno; + } + for (int i = 0; i < NITERS; ++i) { + wpWaitFor(true); + { + sp sp1 = wpBuffer.promote(); + // We implicitly check that sp1 != NULL + sp1->mVisited2 = true; + } + wpBuffer = nullptr; + wpBufferFull = false; + } +} + +TEST(RefBase, RacingPromotions) { + cpu_set_t origCpus; + cpu_set_t myCpus; + // Restrict us and the helper thread to disjoint cpu sets. + // This prevents us from getting scheduled against each other, + // which would be atrociously slow. + if (setExclusiveCpus(&origCpus, &myCpus, &otherCpus)) { + std::thread t(visit3AndRemove); + std::atomic deleteCount(0); + if (sched_setaffinity(0, sizeof(cpu_set_t), &myCpus) != 0) { + FAIL() << "setaffinity returned:" << errno; + } + for (int i = 0; i < NITERS; ++i) { + Bar* bar = new Bar(&deleteCount); + wp wp1(bar); + bar->mVisited1 = true; + if (i % (NITERS / 10) == 0) { + // Do this rarely, since it generates a log message. + wp1 = nullptr; // No longer destroys the object. + wp1 = bar; + } + wpBuffer = wp1; + ASSERT_EQ(bar->getWeakRefs()->getWeakCount(), 2); + wpBufferFull = true; + // Promotion races with that in visit3AndRemove. + // This may or may not succeed, but it shouldn't interfere with + // the concurrent one. + sp sp1 = wp1.promote(); + wpWaitFor(false); // Waits for other thread to drop strong pointer. + sp1 = nullptr; + // No strong pointers here. + sp1 = wp1.promote(); + ASSERT_EQ(sp1.get(), nullptr) << "Dead wp promotion succeeded!"; + } + t.join(); + if (sched_setaffinity(0, sizeof(cpu_set_t), &origCpus) != 0) { + FAIL(); + } + ASSERT_EQ(NITERS, deleteCount) << "Deletions missed!"; + } // Otherwise this is slow and probably pointless on a uniprocessor. +}