From 401d69aa946b68c3d58f46c1c12b1ef5533025a5 Mon Sep 17 00:00:00 2001
From: Steven Moreland <smoreland@google.com>
Date: Tue, 18 Feb 2020 14:03:05 -0800
Subject: [PATCH] libutils: introduce sp<T>::make

This is in preparation of doing what we did for SharedRefBase (hiding
operator new) so that clients can't accidentally construct
shared_ptr/unique_ptr or any other alternative memory management scheme
which would conflict with RefBase. You can see what ultimately happened
to SharedRefBase in frameworks/native CL
10d9ddf2e3da3ba3a425fb8396aaaec728e5fbdb.

The goal for this:
- promote use of 'sp<T>::make' over 'sp<T> .. = new T'
- make 'operator new' a private member of RefBase

Bug: 138956784
Test: libutils_test
Change-Id: I47f4d28edbf7534730c7b6fb1de748dd60f34e11
---
 libutils/StrongPointer_test.cpp        |  8 +++-----
 libutils/include/utils/RefBase.h       |  5 +++++
 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) {