From ed0d79223b73e51e26e03ceff8bbc33d633ad79b Mon Sep 17 00:00:00 2001
From: Steven Moreland <smoreland@google.com>
Date: Fri, 30 Jul 2021 18:06:20 -0700
Subject: binder: SharedRefBase uses enable_shared_from_this

So that make_shared of a SharedRefBase object will not cause
double-ownership. For code compat reasons, we can't actually remove this
class (many places reference it).

Fixes: 194905504
Test: CtsNdkBinderTestCases
Change-Id: I04671965d97960d6630004883df77c17de066d4b
---
 .../include_cpp/android/binder_interface_utils.h   | 37 +++++++++-------------
 libs/binder/ndk/tests/libbinder_ndk_unit_test.cpp  | 31 ++++++++++++++++++
 2 files changed, 46 insertions(+), 22 deletions(-)

diff --git a/libs/binder/ndk/include_cpp/android/binder_interface_utils.h b/libs/binder/ndk/include_cpp/android/binder_interface_utils.h
index 6c4472632d..70a3906a4a 100644
--- a/libs/binder/ndk/include_cpp/android/binder_interface_utils.h
+++ b/libs/binder/ndk/include_cpp/android/binder_interface_utils.h
@@ -43,31 +43,23 @@
 namespace ndk {
 
 /**
- * analog using std::shared_ptr for internally held refcount
- *
  * ref must be called at least one time during the lifetime of this object. The recommended way to
  * construct this object is with SharedRefBase::make.
+ *
+ * Note - this class used to not inherit from enable_shared_from_this, so
+ * std::make_shared works, but it won't be portable against old copies of this
+ * class.
  */
-class SharedRefBase {
+class SharedRefBase : public std::enable_shared_from_this<SharedRefBase> {
    public:
     SharedRefBase() {}
-    virtual ~SharedRefBase() {
-        std::call_once(mFlagThis, [&]() {
-            __assert(__FILE__, __LINE__, "SharedRefBase: no ref created during lifetime");
-        });
-    }
+    virtual ~SharedRefBase() {}
 
     /**
      * A shared_ptr must be held to this object when this is called. This must be called once during
      * the lifetime of this object.
      */
-    std::shared_ptr<SharedRefBase> ref() {
-        std::shared_ptr<SharedRefBase> thiz = mThis.lock();
-
-        std::call_once(mFlagThis, [&]() { mThis = thiz = std::shared_ptr<SharedRefBase>(this); });
-
-        return thiz;
-    }
+    std::shared_ptr<SharedRefBase> ref() { return shared_from_this(); }
 
     /**
      * Convenience method for a ref (see above) which automatically casts to the desired child type.
@@ -86,8 +78,13 @@ class SharedRefBase {
 #pragma clang diagnostic ignored "-Wdeprecated-declarations"
         T* t = new T(std::forward<Args>(args)...);
 #pragma clang diagnostic pop
-        // warning: Potential leak of memory pointed to by 't' [clang-analyzer-unix.Malloc]
-        return t->template ref<T>();  // NOLINT(clang-analyzer-unix.Malloc)
+
+        // T may have a private (though necessarily virtual!) destructor, so we
+        // can't use refbase. For getting the first ref, we don't use ref()
+        // since the internal shared_ptr isn't guaranteed to exist yet.
+        SharedRefBase* base = static_cast<SharedRefBase*>(t);
+        std::shared_ptr<SharedRefBase> strongBase(base);
+        return std::static_pointer_cast<T>(strongBase);
     }
 
     static void operator delete(void* p) { std::free(p); }
@@ -100,13 +97,9 @@ class SharedRefBase {
 #if !defined(__ANDROID_API__) || __ANDROID_API__ >= 30 || defined(__ANDROID_APEX__)
    private:
 #else
-    [[deprecated("Prefer SharedRefBase::make<T>(...) if possible.")]]
+    [[deprecated("Prefer SharedRefBase::make<T>(...) or std::make_shared<T>() if possible.")]]
 #endif
     static void* operator new(size_t s) { return std::malloc(s); }
-
-   private:
-    std::once_flag mFlagThis;
-    std::weak_ptr<SharedRefBase> mThis;
 };
 
 /**
diff --git a/libs/binder/ndk/tests/libbinder_ndk_unit_test.cpp b/libs/binder/ndk/tests/libbinder_ndk_unit_test.cpp
index 5ad390e107..94cc086874 100644
--- a/libs/binder/ndk/tests/libbinder_ndk_unit_test.cpp
+++ b/libs/binder/ndk/tests/libbinder_ndk_unit_test.cpp
@@ -54,6 +54,15 @@ constexpr unsigned int kShutdownWaitTime = 10;
 constexpr uint64_t kContextTestValue = 0xb4e42fb4d9a1d715;
 
 class MyBinderNdkUnitTest : public aidl::BnBinderNdkUnitTest {
+   public:
+    MyBinderNdkUnitTest() = default;
+    MyBinderNdkUnitTest(bool* deleted) : deleted(deleted) {}
+    ~MyBinderNdkUnitTest() {
+        if (deleted) {
+            *deleted = true;
+        }
+    }
+
     ndk::ScopedAStatus repeatInt(int32_t in, int32_t* out) {
         *out = in;
         return ndk::ScopedAStatus::ok();
@@ -122,6 +131,7 @@ class MyBinderNdkUnitTest : public aidl::BnBinderNdkUnitTest {
     }
 
     uint64_t contextTestValue = kContextTestValue;
+    bool* deleted = nullptr;
 };
 
 int generatedService() {
@@ -224,6 +234,27 @@ bool isServiceRunning(const char* serviceName) {
     return true;
 }
 
+TEST(NdkBinder, MakeShared) {
+    const char* kInstance = "make_shared_test_instance";
+    bool deleted = false;
+
+    {
+        auto service = std::make_shared<MyBinderNdkUnitTest>(&deleted);
+        auto binder = service->asBinder();
+        ASSERT_EQ(EX_NONE, AServiceManager_addService(binder.get(), kInstance));
+        auto binder2 = ndk::SpAIBinder(AServiceManager_checkService(kInstance));
+        ASSERT_EQ(binder.get(), binder2.get());
+
+        // overwrite service
+        ASSERT_EQ(EX_NONE,
+                  AServiceManager_addService(
+                          std::make_shared<MyBinderNdkUnitTest>(&deleted)->asBinder().get(),
+                          kInstance));
+    }
+
+    EXPECT_TRUE(deleted);
+}
+
 TEST(NdkBinder, GetServiceThatDoesntExist) {
     sp<IFoo> foo = IFoo::getService("asdfghkl;");
     EXPECT_EQ(nullptr, foo.get());
-- 
cgit v1.2.3-59-g8ed1b