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/tests/binderAllocationLimits.cpp b/libs/binder/tests/binderAllocationLimits.cpp
index 55a3916..6a6e008 100644
--- a/libs/binder/tests/binderAllocationLimits.cpp
+++ b/libs/binder/tests/binderAllocationLimits.cpp
@@ -172,6 +172,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();