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
diff --git a/libs/binder/ndk/AIBinder.cpp b/libs/binder/ndk/AIBinder.cpp
index ee1d48a..c02a77a 100644
--- a/libs/binder/ndk/AIBinder.cpp
+++ b/libs/binder/ndk/AIBinder.cpp
@@ -34,10 +34,10 @@
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 @@
} // 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() {}
-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 f4c3249..30009d2 100644
--- a/libs/binder/ndk/AIBinder_internal.h
+++ b/libs/binder/ndk/AIBinder_internal.h
@@ -81,13 +81,18 @@
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 93384d6..f5b4bef 100644
--- a/libs/binder/ndk/AParcel.cpp
+++ b/libs/binder/ndk/AParcel.cpp
@@ -43,7 +43,7 @@
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 @@
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 3979945..90dd1c8 100644
--- a/libs/binder/ndk/AServiceManager.cpp
+++ b/libs/binder/ndk/AServiceManager.cpp
@@ -41,7 +41,7 @@
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 8f6672f..c913884 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 @@
* 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 @@
*
* 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 519db5c..b8518d7 100644
--- a/libs/binder/ndk/test/main_client.cpp
+++ b/libs/binder/ndk/test/main_client.cpp
@@ -87,6 +87,34 @@
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";