libbinder: cache interface descriptor if empty
This adds a few additional bytes of .ro data to store the warning
message in the String, but worrying about re-fetching the interface
descriptor when it is empty (which happens less often in native
code after BBinder has a default descriptor, but still happens in
Java, or in custom implementations) adds complexity to other code.
Since we guarantee to always cache the descriptor, we don't need
to think about this case as much.
One alternative implementation would be to drop BpBinder mObitsSent
and use both !mAlive and an empty obituary list to represent the
obituaries being sent. However, due to sendObituary using mObitsSent
in order to avoid taking a lock in some cases (something that
should have never been done, because it's optimizing a fast path
and the way it does it means that certain races will take a lock
part of the time - which is flake prone), I couldn't find a way to
remove this variable without introducing the possibility that
we take an extra lock after linkToDeath fires, which could prevent
system recovery and cause a deadlock. Moving this variable would
have to be done more carefully.
For now, we can avoid repeated binder calls for an empty interface
descriptor. This is intended to help justify (perhaps overly so)
other changes being made in the bug, but I'm submitting it for
review entirely independently, because it's not strictly necessary
for correctness assuming that the corresponding Bn implementation
of getInterfaceDescriptor is correct. If the implementation of this
function is adverserial, it could lead to a deadlock in some
situations, but a far easier way to cause this same deadlock would
be to not return from getInterfaceDescriptor at all, which is
well-known.
Bug: 262463798
Test: binderAllocationLimits
Change-Id: I07aee55f6092b52189ad2fadbbcd0880e2e3cbf4
diff --git a/libs/binder/BpBinder.cpp b/libs/binder/BpBinder.cpp
index 921e57c..93e8625 100644
--- a/libs/binder/BpBinder.cpp
+++ b/libs/binder/BpBinder.cpp
@@ -43,6 +43,8 @@
binder_proxy_limit_callback BpBinder::sLimitCallback;
bool BpBinder::sBinderProxyThrottleCreate = false;
+static StaticString16 kDescriptorUninit(u"<uninit descriptor>");
+
// Arbitrarily high value that probably distinguishes a bad behaving app
uint32_t BpBinder::sBinderProxyCountHighWatermark = 2500;
// Another arbitrary value a binder count needs to drop below before another callback will be called
@@ -172,6 +174,7 @@
mAlive(true),
mObitsSent(false),
mObituaries(nullptr),
+ mDescriptorCache(kDescriptorUninit),
mTrackedUid(-1) {
extendObjectLifetime(OBJECT_LIFETIME_WEAK);
}
@@ -214,12 +217,12 @@
bool BpBinder::isDescriptorCached() const {
Mutex::Autolock _l(mLock);
- return mDescriptorCache.size() ? true : false;
+ return mDescriptorCache.string() != kDescriptorUninit.string();
}
const String16& BpBinder::getInterfaceDescriptor() const
{
- if (isDescriptorCached() == false) {
+ if (!isDescriptorCached()) {
sp<BpBinder> thiz = sp<BpBinder>::fromExisting(const_cast<BpBinder*>(this));
Parcel data;
@@ -232,8 +235,7 @@
Mutex::Autolock _l(mLock);
// mDescriptorCache could have been assigned while the lock was
// released.
- if (mDescriptorCache.size() == 0)
- mDescriptorCache = res;
+ if (mDescriptorCache.string() == kDescriptorUninit.string()) mDescriptorCache = res;
}
}
@@ -308,10 +310,7 @@
if (data.dataSize() > LOG_TRANSACTIONS_OVER_SIZE) {
Mutex::Autolock _l(mLock);
ALOGW("Large outgoing transaction of %zu bytes, interface descriptor %s, code %d",
- data.dataSize(),
- mDescriptorCache.size() ? String8(mDescriptorCache).c_str()
- : "<uncached descriptor>",
- code);
+ data.dataSize(), String8(mDescriptorCache).c_str(), code);
}
if (status == DEAD_OBJECT) mAlive = 0;
@@ -531,7 +530,7 @@
if(obits != nullptr) {
if (!obits->isEmpty()) {
ALOGI("onLastStrongRef automatically unlinking death recipients: %s",
- mDescriptorCache.size() ? String8(mDescriptorCache).c_str() : "<uncached descriptor>");
+ String8(mDescriptorCache).c_str());
}
if (ipc) ipc->clearDeathNotification(binderHandle(), this);
diff --git a/libs/binder/tests/binderAllocationLimits.cpp b/libs/binder/tests/binderAllocationLimits.cpp
index e1f5ed5..f5fb4e4 100644
--- a/libs/binder/tests/binderAllocationLimits.cpp
+++ b/libs/binder/tests/binderAllocationLimits.cpp
@@ -160,6 +160,24 @@
a_binder->pingBinder();
}
+TEST(BinderAllocation, InterfaceDescriptorTransaction) {
+ sp<IBinder> a_binder = GetRemoteBinder();
+
+ size_t mallocs = 0;
+ const auto on_malloc = OnMalloc([&](size_t bytes) {
+ mallocs++;
+ // Happens to be SM package length. We could switch to forking
+ // and registering our own service if it became an issue.
+ EXPECT_EQ(bytes, 78);
+ });
+
+ a_binder->getInterfaceDescriptor();
+ a_binder->getInterfaceDescriptor();
+ a_binder->getInterfaceDescriptor();
+
+ EXPECT_EQ(mallocs, 1);
+}
+
TEST(BinderAllocation, SmallTransaction) {
String16 empty_descriptor = String16("");
sp<IServiceManager> manager = defaultServiceManager();