libbinder_ndk: ABpBinder linkern'space muliplicity
Previously, if the same process loaded the same class in two different
linker namespaces, you could not have an ABpBinder which pointed to the
same service in both of them.
I kind of did this intentionally, because at the time I was still
figuring out a lot of this stuff, and I didn't want to make the
implementation unnecessarily complicated. I also wanted a chance to
investigate these issues. In some cases, this may mean that an ODR
issue is less noticeable (however - the way we should fix these is
by having the build system and linkers detect when we overwrite a
symbol which is not binary identical - a very difficult a noisy
task, but a neglected and unfortunate one). Anyway, we have other
checks for ODR issues now in the AIDL build system. It's okay.
Anyway - it turns out the stability mechanism in AIDL is really
awesome and enables some awesome usecases, so we need this support
now. Well.
Fixes: 262463798
Test: libbinder_ndk_unit_test
Change-Id: Idce05e13768cd4ad10c64873f393535d74cd4d3e
diff --git a/libs/binder/ndk/ibinder.cpp b/libs/binder/ndk/ibinder.cpp
index b29f22e..d0de7b9 100644
--- a/libs/binder/ndk/ibinder.cpp
+++ b/libs/binder/ndk/ibinder.cpp
@@ -101,7 +101,22 @@
std::lock_guard<std::mutex> lock(mClazzMutex);
if (mClazz == clazz) return true;
- if (mClazz != nullptr) {
+ // If this is an ABpBinder, the first class object becomes the canonical one. The implication
+ // of this is that no API can require a proxy information to get information on how to behave.
+ // from the class itself - which should only store the interface descriptor. The functionality
+ // should be implemented by adding AIBinder_* APIs to set values on binders themselves, by
+ // setting things on AIBinder_Class which get transferred along with the binder, so that they
+ // can be read along with the BpBinder, or by modifying APIs directly (e.g. an option in
+ // onTransact).
+ //
+ // While this check is required to support linkernamespaces, one downside of it is that
+ // you may parcel code to communicate between things in the same process. However, comms
+ // between linkernamespaces like this already happen for cross-language calls like Java<->C++
+ // or Rust<->Java, and there are good stability guarantees here. This interacts with
+ // binder Stability checks exactly like any other in-process call. The stability is known
+ // to the IBinder object, so that it doesn't matter if a class object comes from
+ // a different stability level.
+ if (mClazz != nullptr && !asABpBinder()) {
const String16& currentDescriptor = mClazz->getInterfaceDescriptor();
if (newDescriptor == currentDescriptor) {
LOG(ERROR) << __func__ << ": Class descriptors '" << currentDescriptor
@@ -115,7 +130,6 @@
}
// always a failure because we know mClazz != clazz
- // TODO(b/262463798): support multiple ABpBinder in different namespaces
return false;
}
@@ -136,8 +150,13 @@
return false;
}
- // if this is a local object, it's not one known to libbinder_ndk
- mClazz = clazz;
+ // A local binder being set for the first time OR
+ // ignoring a proxy binder which is set multiple time, by considering the first
+ // associated class as the canonical one.
+ if (mClazz == nullptr) {
+ mClazz = clazz;
+ }
+
return true;
}
@@ -322,6 +341,10 @@
return lhs->binder < rhs->binder;
}
+// WARNING: When multiple classes exist with the same interface descriptor in different
+// linkernamespaces, the first one to be associated with mClazz becomes the canonical one
+// and the only requirement on this is that the interface descriptors match. If this
+// is an ABpBinder, no other state can be referenced from mClazz.
AIBinder_Class::AIBinder_Class(const char* interfaceDescriptor, AIBinder_Class_onCreate onCreate,
AIBinder_Class_onDestroy onDestroy,
AIBinder_Class_onTransact onTransact)
@@ -629,6 +652,10 @@
(*in)->get()->markForBinder(binder->getBinder());
status_t status = android::OK;
+
+ // note - this is the only read of a value in clazz, and it comes with a warning
+ // on the API itself. Do not copy this design. Instead, attach data in a new
+ // version of the prepareTransaction function.
if (clazz->writeHeader) {
status = (*in)->get()->writeInterfaceToken(clazz->getInterfaceDescriptor());
}
diff --git a/libs/binder/ndk/ibinder_internal.h b/libs/binder/ndk/ibinder_internal.h
index 8699f85..67bb092 100644
--- a/libs/binder/ndk/ibinder_internal.h
+++ b/libs/binder/ndk/ibinder_internal.h
@@ -56,6 +56,11 @@
// AIBinder instance is instance of this class for a local object. In order to transact on a
// remote object, this also must be set for simplicity (although right now, only the
// interfaceDescriptor from it is used).
+ //
+ // WARNING: When multiple classes exist with the same interface descriptor in different
+ // linkernamespaces, the first one to be associated with mClazz becomes the canonical one
+ // and the only requirement on this is that the interface descriptors match. If this
+ // is an ABpBinder, no other state can be referenced from mClazz.
const AIBinder_Class* mClazz;
std::mutex mClazzMutex;
};
diff --git a/libs/binder/ndk/include_ndk/android/binder_ibinder.h b/libs/binder/ndk/include_ndk/android/binder_ibinder.h
index 1af2119..05677a8 100644
--- a/libs/binder/ndk/include_ndk/android/binder_ibinder.h
+++ b/libs/binder/ndk/include_ndk/android/binder_ibinder.h
@@ -229,6 +229,11 @@
*
* Available since API level 33.
*
+ * WARNING: this API interacts badly with linkernamespaces. For correct behavior, you must
+ * use it on all instances of a class in the same process which share the same interface
+ * descriptor. In general, it is recommended you do not use this API, because it is disabling
+ * type safety.
+ *
* \param clazz class to disable interface header on.
*/
void AIBinder_Class_disableInterfaceTokenHeader(AIBinder_Class* clazz) __INTRODUCED_IN(33);
diff --git a/libs/binder/ndk/tests/iface.cpp b/libs/binder/ndk/tests/iface.cpp
index 2afe5d2..76acff5 100644
--- a/libs/binder/ndk/tests/iface.cpp
+++ b/libs/binder/ndk/tests/iface.cpp
@@ -72,6 +72,11 @@
AIBinder_Class* IFoo::kClass = AIBinder_Class_define(kIFooDescriptor, IFoo_Class_onCreate,
IFoo_Class_onDestroy, IFoo_Class_onTransact);
+// Defines the same class. Ordinarly, you would never want to do this, but it's done here
+// to simulate what would happen when multiple linker namespaces interact.
+AIBinder_Class* IFoo::kClassDupe = AIBinder_Class_define(
+ kIFooDescriptor, IFoo_Class_onCreate, IFoo_Class_onDestroy, IFoo_Class_onTransact);
+
class BpFoo : public IFoo {
public:
explicit BpFoo(AIBinder* binder) : mBinder(binder) {}
diff --git a/libs/binder/ndk/tests/include/iface/iface.h b/libs/binder/ndk/tests/include/iface/iface.h
index 7408d0c..0a562f0 100644
--- a/libs/binder/ndk/tests/include/iface/iface.h
+++ b/libs/binder/ndk/tests/include/iface/iface.h
@@ -30,8 +30,13 @@
static const char* kIFooDescriptor;
static AIBinder_Class* kClass;
+ static AIBinder_Class* kClassDupe;
// binder representing this interface with one reference count
+ // NOTE - this will create a new binder if it already exists. If you use
+ // getService for instance, you must pull outBinder. Don't use this without
+ // verifying isRemote or pointer equality. This is not a very good testing API - don't
+ // copy it - consider the AIDL-generated APIs instead.
AIBinder* getBinder();
// Takes ownership of IFoo
diff --git a/libs/binder/ndk/tests/libbinder_ndk_unit_test.cpp b/libs/binder/ndk/tests/libbinder_ndk_unit_test.cpp
index 9d5ef68..5b2532a 100644
--- a/libs/binder/ndk/tests/libbinder_ndk_unit_test.cpp
+++ b/libs/binder/ndk/tests/libbinder_ndk_unit_test.cpp
@@ -55,6 +55,18 @@
constexpr unsigned int kShutdownWaitTime = 10;
constexpr uint64_t kContextTestValue = 0xb4e42fb4d9a1d715;
+class MyTestFoo : public IFoo {
+ binder_status_t doubleNumber(int32_t in, int32_t* out) override {
+ *out = 2 * in;
+ LOG(INFO) << "doubleNumber (" << in << ") => " << *out;
+ return STATUS_OK;
+ }
+ binder_status_t die() override {
+ ADD_FAILURE() << "die called on local instance";
+ return STATUS_OK;
+ }
+};
+
class MyBinderNdkUnitTest : public aidl::BnBinderNdkUnitTest {
ndk::ScopedAStatus repeatInt(int32_t in, int32_t* out) {
*out = in;
@@ -296,11 +308,10 @@
}
TEST(NdkBinder, UnimplementedDump) {
- sp<IFoo> foo = IFoo::getService(IFoo::kSomeInstanceName);
+ ndk::SpAIBinder binder;
+ sp<IFoo> foo = IFoo::getService(IFoo::kSomeInstanceName, binder.getR());
ASSERT_NE(foo, nullptr);
- AIBinder* binder = foo->getBinder();
- EXPECT_EQ(OK, AIBinder_dump(binder, STDOUT_FILENO, nullptr, 0));
- AIBinder_decStrong(binder);
+ EXPECT_EQ(OK, AIBinder_dump(binder.get(), STDOUT_FILENO, nullptr, 0));
}
TEST(NdkBinder, UnimplementedShell) {
@@ -324,6 +335,24 @@
EXPECT_EQ(2, out);
}
+TEST(NdkBinder, ReassociateBpBinderWithSameDescriptor) {
+ ndk::SpAIBinder binder;
+ sp<IFoo> foo = IFoo::getService(IFoo::kSomeInstanceName, binder.getR());
+
+ EXPECT_TRUE(AIBinder_isRemote(binder.get()));
+
+ EXPECT_TRUE(AIBinder_associateClass(binder.get(), IFoo::kClassDupe));
+}
+
+TEST(NdkBinder, CantHaveTwoLocalBinderClassesWithSameDescriptor) {
+ sp<IFoo> foo = sp<MyTestFoo>::make();
+ ndk::SpAIBinder binder(foo->getBinder());
+
+ EXPECT_FALSE(AIBinder_isRemote(binder.get()));
+
+ EXPECT_FALSE(AIBinder_associateClass(binder.get(), IFoo::kClassDupe));
+}
+
TEST(NdkBinder, GetTestServiceStressTest) {
// libbinder has some complicated logic to make sure only one instance of
// ABpBinder is associated with each binder.
@@ -545,18 +574,6 @@
AIBinder_decStrong(binder);
}
-class MyTestFoo : public IFoo {
- binder_status_t doubleNumber(int32_t in, int32_t* out) override {
- *out = 2 * in;
- LOG(INFO) << "doubleNumber (" << in << ") => " << *out;
- return STATUS_OK;
- }
- binder_status_t die() override {
- ADD_FAILURE() << "die called on local instance";
- return STATUS_OK;
- }
-};
-
TEST(NdkBinder, SetInheritRt) {
// functional test in binderLibTest
sp<IFoo> foo = sp<MyTestFoo>::make();
@@ -597,7 +614,8 @@
sp<IFoo> foo = new MyTestFoo;
EXPECT_EQ(EX_NONE, foo->addService(kInstanceName));
- sp<IFoo> getFoo = IFoo::getService(kInstanceName);
+ ndk::SpAIBinder binder;
+ sp<IFoo> getFoo = IFoo::getService(kInstanceName, binder.getR());
EXPECT_EQ(foo.get(), getFoo.get());
int32_t out;