libbinder_ndk: pointer equality of AIBinder.

This means that there is always a 1-1 correspondance between any object
(remote or local) and its AIBinder in a given process. Two cases:

1). For local objects created by the libbinder_ndk:

    AIBinder_new is called once. Whenever a binder is sent and then
    later received, ABBinderTag is used to identify and recover the
    object immediately.

2). For local objects created by libbinder and all remote objects
regardless of their origin (this is what this CL fixes):

    ABpBinderTag is used to tag the object. This tag holds a weak
    pointer to ABpBinder which is used to recover the object.

Bug: 111445392
Test: ./ndk/runtests.sh

Change-Id: I0d52ed5e356b26a62cfbc8e822f274c878d1112d

6 files changed, 109 insertions, 10 deletions

diff --git a/libs/binder/ndk/AIBinder.cpp b/libs/binder/ndk/AIBinder.cpp
index ee1d48a822..c02a77a3e1 100644
--- a/libs/binder/ndk/AIBinder.cpp
+++ b/libs/binder/ndk/AIBinder.cpp
@@ -34,10 +34,10 @@ namespace ABBinderTag {
 
 static const void* kId = "ABBinder";
 static void* kValue = static_cast<void*>(new bool{true});
-void cleanId(const void* /*id*/, void* /*obj*/, void* /*cookie*/){/* do nothing */};
+void clean(const void* /*id*/, void* /*obj*/, void* /*cookie*/){/* do nothing */};
 
 static void attach(const sp<IBinder>& binder) {
-    binder->attachObject(kId, kValue, nullptr /*cookie*/, cleanId);
+    binder->attachObject(kId, kValue, nullptr /*cookie*/, clean);
 }
 static bool has(const sp<IBinder>& binder) {
     return binder != nullptr && binder->findObject(kId) == kValue;
@@ -45,6 +45,21 @@ static bool has(const sp<IBinder>& binder) {
 
 } // namespace ABBinderTag
 
+namespace ABpBinderTag {
+
+static std::mutex gLock;
+static const void* kId = "ABpBinder";
+struct Value {
+    wp<ABpBinder> binder;
+};
+void clean(const void* id, void* obj, void* cookie) {
+    CHECK(id == kId) << id << " " << obj << " " << cookie;
+
+    delete static_cast<Value*>(obj);
+};
+
+} // namespace ABpBinderTag
+
 AIBinder::AIBinder(const AIBinder_Class* clazz) : mClazz(clazz) {}
 AIBinder::~AIBinder() {}
 
@@ -123,14 +138,51 @@ ABpBinder::ABpBinder(const ::android::sp<::android::IBinder>& binder)
 }
 ABpBinder::~ABpBinder() {}
 
-sp<AIBinder> ABpBinder::fromBinder(const ::android::sp<::android::IBinder>& binder) {
+void ABpBinder::onLastStrongRef(const void* id) {
+    {
+        std::lock_guard<std::mutex> lock(ABpBinderTag::gLock);
+        // Since ABpBinder is OBJECT_LIFETIME_WEAK, we must remove this weak reference in order for
+        // the ABpBinder to be deleted. Since a strong reference to this ABpBinder object should no
+        // longer be able to exist at the time of this method call, there is no longer a need to
+        // recover it.
+
+        ABpBinderTag::Value* value =
+                static_cast<ABpBinderTag::Value*>(remote()->findObject(ABpBinderTag::kId));
+        if (value != nullptr) {
+            value->binder = nullptr;
+        }
+    }
+
+    BpRefBase::onLastStrongRef(id);
+}
+
+sp<AIBinder> ABpBinder::lookupOrCreateFromBinder(const ::android::sp<::android::IBinder>& binder) {
     if (binder == nullptr) {
         return nullptr;
     }
     if (ABBinderTag::has(binder)) {
         return static_cast<ABBinder*>(binder.get());
     }
-    return new ABpBinder(binder);
+
+    // The following code ensures that for a given binder object (remote or local), if it is not an
+    // ABBinder then at most one ABpBinder object exists in a given process representing it.
+    std::lock_guard<std::mutex> lock(ABpBinderTag::gLock);
+
+    ABpBinderTag::Value* value =
+            static_cast<ABpBinderTag::Value*>(binder->findObject(ABpBinderTag::kId));
+    if (value == nullptr) {
+        value = new ABpBinderTag::Value;
+        binder->attachObject(ABpBinderTag::kId, static_cast<void*>(value), nullptr /*cookie*/,
+                             ABpBinderTag::clean);
+    }
+
+    sp<ABpBinder> ret = value->binder.promote();
+    if (ret == nullptr) {
+        ret = new ABpBinder(binder);
+        value->binder = ret;
+    }
+
+    return ret;
 }
 
 struct AIBinder_Weak {
diff --git a/libs/binder/ndk/AIBinder_internal.h b/libs/binder/ndk/AIBinder_internal.h
index f4c32491b4..30009d253e 100644
--- a/libs/binder/ndk/AIBinder_internal.h
+++ b/libs/binder/ndk/AIBinder_internal.h
@@ -81,13 +81,18 @@ private:
     void* mUserData;
 };
 
-// This binder object may be remote or local (even though it is 'Bp'). It is not yet associated with
-// a class.
+// This binder object may be remote or local (even though it is 'Bp'). The implication if it is
+// local is that it is an IBinder object created outside of the domain of libbinder_ndk.
 struct ABpBinder : public AIBinder, public ::android::BpRefBase {
-    static ::android::sp<AIBinder> fromBinder(const ::android::sp<::android::IBinder>& binder);
+    // Looks up to see if this object has or is an existing ABBinder or ABpBinder object, otherwise
+    // it creates an ABpBinder object.
+    static ::android::sp<AIBinder> lookupOrCreateFromBinder(
+            const ::android::sp<::android::IBinder>& binder);
 
     virtual ~ABpBinder();
 
+    void onLastStrongRef(const void* id) override;
+
     ::android::sp<::android::IBinder> getBinder() override { return remote(); }
     ABpBinder* asABpBinder() override { return this; }
 
diff --git a/libs/binder/ndk/AParcel.cpp b/libs/binder/ndk/AParcel.cpp
index 93384d64fa..f5b4befa14 100644
--- a/libs/binder/ndk/AParcel.cpp
+++ b/libs/binder/ndk/AParcel.cpp
@@ -43,7 +43,7 @@ binder_status_t AParcel_readStrongBinder(const AParcel* parcel, AIBinder** binde
     if (status != EX_NONE) {
         return status;
     }
-    sp<AIBinder> ret = ABpBinder::fromBinder(readBinder);
+    sp<AIBinder> ret = ABpBinder::lookupOrCreateFromBinder(readBinder);
     AIBinder_incStrong(ret.get());
     *binder = ret.get();
     return status;
@@ -54,7 +54,7 @@ binder_status_t AParcel_readNullableStrongBinder(const AParcel* parcel, AIBinder
     if (status != EX_NONE) {
         return status;
     }
-    sp<AIBinder> ret = ABpBinder::fromBinder(readBinder);
+    sp<AIBinder> ret = ABpBinder::lookupOrCreateFromBinder(readBinder);
     AIBinder_incStrong(ret.get());
     *binder = ret.get();
     return status;
diff --git a/libs/binder/ndk/AServiceManager.cpp b/libs/binder/ndk/AServiceManager.cpp
index 3979945e45..90dd1c810b 100644
--- a/libs/binder/ndk/AServiceManager.cpp
+++ b/libs/binder/ndk/AServiceManager.cpp
@@ -41,7 +41,7 @@ AIBinder* AServiceManager_getService(const char* instance) {
     sp<IServiceManager> sm = defaultServiceManager();
     sp<IBinder> binder = sm->getService(String16(instance));
 
-    sp<AIBinder> ret = ABpBinder::fromBinder(binder);
+    sp<AIBinder> ret = ABpBinder::lookupOrCreateFromBinder(binder);
     AIBinder_incStrong(ret.get());
     return ret.get();
 }
diff --git a/libs/binder/ndk/include_ndk/android/binder_ibinder.h b/libs/binder/ndk/include_ndk/android/binder_ibinder.h
index 8f6672f605..c91388428a 100644
--- a/libs/binder/ndk/include_ndk/android/binder_ibinder.h
+++ b/libs/binder/ndk/include_ndk/android/binder_ibinder.h
@@ -88,6 +88,12 @@ typedef struct AIBinder_Class AIBinder_Class;
  * If the process containing an AIBinder dies, it is possible to be holding a strong reference to
  * an object which does not exist. In this case, transactions to this binder will return
  * EX_DEAD_OBJECT. See also AIBinder_linkToDeath, AIBinder_unlinkToDeath, and AIBinder_isAlive.
+ *
+ * Once an AIBinder is created, anywhere it is passed (remotely or locally), there is a 1-1
+ * correspondence between the address of an AIBinder and the object it represents. This means that
+ * when two AIBinder pointers point to the same address, they represent the same object (whether
+ * that object is local or remote). This correspondance can be broken accidentally if AIBinder_new
+ * is erronesouly called to create the same object multiple times.
  */
 struct AIBinder;
 typedef struct AIBinder AIBinder;
@@ -148,6 +154,14 @@ __attribute__((warn_unused_result)) AIBinder_Class* AIBinder_Class_define(
  *
  * When this is called, the refcount is implicitly 1. So, calling decStrong exactly one time is
  * required to delete this object.
+ *
+ * Once an AIBinder object is created using this API, re-creating that AIBinder for the same
+ * instance of the same class will break pointer equality for that specific AIBinder object. For
+ * instance, if someone erroneously created two AIBinder instances representing the same callback
+ * object and passed one to a hypothetical addCallback function and then later another one to a
+ * hypothetical removeCallback function, the remote process would have no way to determine that
+ * these two objects are actually equal using the AIBinder pointer alone (which they should be able
+ * to do). Also see the suggested memory ownership model suggested above.
  */
 __attribute__((warn_unused_result)) AIBinder* AIBinder_new(const AIBinder_Class* clazz, void* args);
 
diff --git a/libs/binder/ndk/test/main_client.cpp b/libs/binder/ndk/test/main_client.cpp
index 519db5c71a..b8518d7842 100644
--- a/libs/binder/ndk/test/main_client.cpp
+++ b/libs/binder/ndk/test/main_client.cpp
@@ -87,6 +87,34 @@ TEST(NdkBinder, GetServiceInProcess) {
     EXPECT_EQ(2, getFoo->doubleNumber(1));
 }
 
+TEST(NdkBinder, EqualityOfRemoteBinderPointer) {
+    AIBinder* binderA = AServiceManager_getService(kExistingNonNdkService);
+    ASSERT_NE(nullptr, binderA);
+
+    AIBinder* binderB = AServiceManager_getService(kExistingNonNdkService);
+    ASSERT_NE(nullptr, binderB);
+
+    EXPECT_EQ(binderA, binderB);
+
+    AIBinder_decStrong(binderA);
+    AIBinder_decStrong(binderB);
+}
+
+TEST(NdkBinder, ABpBinderRefCount) {
+    AIBinder* binder = AServiceManager_getService(kExistingNonNdkService);
+    AIBinder_Weak* wBinder = AIBinder_Weak_new(binder);
+
+    ASSERT_NE(nullptr, binder);
+    EXPECT_EQ(1, AIBinder_debugGetRefCount(binder));
+
+    AIBinder_decStrong(binder);
+
+    // assert because would need to decStrong if non-null and we shouldn't need to add a no-op here
+    ASSERT_NE(nullptr, AIBinder_Weak_promote(wBinder));
+
+    AIBinder_Weak_delete(&wBinder);
+}
+
 TEST(NdkBinder, AddServiceMultipleTimes) {
     static const char* kInstanceName1 = "test-multi-1";
     static const char* kInstanceName2 = "test-multi-2";