diff options
| -rw-r--r-- | libutils/StrongPointer_test.cpp | 8 | ||||
| -rw-r--r-- | libutils/include/utils/RefBase.h | 5 | ||||
| -rw-r--r-- | libutils/include/utils/StrongPointer.h | 21 |
3 files changed, 26 insertions, 8 deletions
diff --git a/libutils/StrongPointer_test.cpp b/libutils/StrongPointer_test.cpp index 7b2e37f27..d37c1de6f 100644 --- a/libutils/StrongPointer_test.cpp +++ b/libutils/StrongPointer_test.cpp @@ -36,10 +36,8 @@ private: TEST(StrongPointer, move) { bool isDeleted; - SPFoo* foo = new SPFoo(&isDeleted); - ASSERT_EQ(0, foo->getStrongCount()); - ASSERT_FALSE(isDeleted) << "Already deleted...?"; - sp<SPFoo> sp1(foo); + sp<SPFoo> sp1 = sp<SPFoo>::make(&isDeleted); + SPFoo* foo = sp1.get(); ASSERT_EQ(1, foo->getStrongCount()); { sp<SPFoo> sp2 = std::move(sp1); @@ -65,7 +63,7 @@ TEST(StrongPointer, NullptrComparison) { TEST(StrongPointer, PointerComparison) { bool isDeleted; - sp<SPFoo> foo = new SPFoo(&isDeleted); + sp<SPFoo> foo = sp<SPFoo>::make(&isDeleted); ASSERT_EQ(foo.get(), foo); ASSERT_EQ(foo, foo.get()); ASSERT_NE(nullptr, foo); diff --git a/libutils/include/utils/RefBase.h b/libutils/include/utils/RefBase.h index 89f048db8..e7acd17d2 100644 --- a/libutils/include/utils/RefBase.h +++ b/libutils/include/utils/RefBase.h @@ -297,6 +297,11 @@ public: } protected: + // When constructing these objects, prefer using sp::make<>. Using a RefBase + // object on the stack or with other refcount mechanisms (e.g. + // std::shared_ptr) is inherently wrong. RefBase types have an implicit + // ownership model and cannot be safely used with other ownership models. + RefBase(); virtual ~RefBase(); diff --git a/libutils/include/utils/StrongPointer.h b/libutils/include/utils/StrongPointer.h index 6f4fb4788..11128f227 100644 --- a/libutils/include/utils/StrongPointer.h +++ b/libutils/include/utils/StrongPointer.h @@ -32,6 +32,12 @@ class sp { public: inline sp() : m_ptr(nullptr) { } + // TODO: switch everyone to using this over new, and make RefBase operator + // new private to that class so that we can avoid RefBase being used with + // other memory management mechanisms. + template <typename... Args> + static inline sp<T> make(Args&&... args); + sp(T* other); // NOLINT(implicit) sp(const sp<T>& other); sp(sp<T>&& other) noexcept; @@ -160,9 +166,6 @@ void sp_report_stack_pointer(); // It does not appear safe to broaden this check to include adjacent pages; apparently this code // is used in environments where there may not be a guard page below (at higher addresses than) // the bottom of the stack. -// -// TODO: Consider adding make_sp<T>() to allocate an object and wrap the resulting pointer safely -// without checking overhead. template <typename T> void sp<T>::check_not_on_stack(const void* ptr) { static constexpr int MIN_PAGE_SIZE = 0x1000; // 4K. Safer than including sys/user.h. @@ -174,6 +177,18 @@ void sp<T>::check_not_on_stack(const void* ptr) { } } +// TODO: Ideally we should find a way to increment the reference count before running the +// constructor, so that generating an sp<> to this in the constructor is no longer dangerous. +template <typename T> +template <typename... Args> +sp<T> sp<T>::make(Args&&... args) { + T* t = new T(std::forward<Args>(args)...); + sp<T> result; + result.m_ptr = t; + t->incStrong(t); // bypass check_not_on_stack for heap allocation + return result; +} + template<typename T> sp<T>::sp(T* other) : m_ptr(other) { |