Merge changes Ieb537412,I2339a8e0 am: a7e51752c2 am: 5ada8b386e am: fbe52e4cf2 am: fa943a44b7 am: 5d59f5737f
Original change: https://android-review.googlesource.com/c/platform/frameworks/native/+/1833330
Change-Id: I4f50a8732de10afdc2a3f380e0a2ce8045ce197f
diff --git a/libs/binder/Parcel.cpp b/libs/binder/Parcel.cpp
index 4ecf4b3..07555f6 100644
--- a/libs/binder/Parcel.cpp
+++ b/libs/binder/Parcel.cpp
@@ -291,6 +291,9 @@
if (status_t status = mSession->state()->onBinderEntering(mSession, addr, &binder);
status != OK)
return status;
+ if (status_t status = mSession->state()->flushExcessBinderRefs(mSession, addr, binder);
+ status != OK)
+ return status;
}
return finishUnflattenBinder(binder, out);
diff --git a/libs/binder/RpcSession.cpp b/libs/binder/RpcSession.cpp
index 38958c9..dafb339 100644
--- a/libs/binder/RpcSession.cpp
+++ b/libs/binder/RpcSession.cpp
@@ -217,15 +217,17 @@
}
status_t RpcSession::sendDecStrong(const BpBinder* binder) {
- return sendDecStrong(binder->getPrivateAccessor().rpcAddress());
+ // target is 0 because this is used to free BpBinder objects
+ return sendDecStrongToTarget(binder->getPrivateAccessor().rpcAddress(), 0 /*target*/);
}
-status_t RpcSession::sendDecStrong(uint64_t address) {
+status_t RpcSession::sendDecStrongToTarget(uint64_t address, size_t target) {
ExclusiveConnection connection;
status_t status = ExclusiveConnection::find(sp<RpcSession>::fromExisting(this),
ConnectionUse::CLIENT_REFCOUNT, &connection);
if (status != OK) return status;
- return state()->sendDecStrong(connection.get(), sp<RpcSession>::fromExisting(this), address);
+ return state()->sendDecStrongToTarget(connection.get(), sp<RpcSession>::fromExisting(this),
+ address, target);
}
status_t RpcSession::readId() {
diff --git a/libs/binder/RpcState.cpp b/libs/binder/RpcState.cpp
index 1e86104..3ff13bc 100644
--- a/libs/binder/RpcState.cpp
+++ b/libs/binder/RpcState.cpp
@@ -152,7 +152,7 @@
return BAD_VALUE;
}
- std::unique_lock<std::mutex> _l(mNodeMutex);
+ std::lock_guard<std::mutex> _l(mNodeMutex);
if (mTerminated) return DEAD_OBJECT;
if (auto it = mNodeForAddress.find(address); it != mNodeForAddress.end()) {
@@ -160,13 +160,7 @@
// implicitly have strong RPC refcount, since we received this binder
it->second.timesRecd++;
-
- _l.unlock();
-
- // We have timesRecd RPC refcounts, but we only need to hold on to one
- // when we keep the object. All additional dec strongs are sent
- // immediately, we wait to send the last one in BpBinder::onLastDecStrong.
- return session->sendDecStrong(address);
+ return OK;
}
// we don't know about this binder, so the other side of the connection
@@ -187,6 +181,36 @@
return OK;
}
+status_t RpcState::flushExcessBinderRefs(const sp<RpcSession>& session, uint64_t address,
+ const sp<IBinder>& binder) {
+ std::unique_lock<std::mutex> _l(mNodeMutex);
+ if (mTerminated) return DEAD_OBJECT;
+
+ auto it = mNodeForAddress.find(address);
+
+ LOG_ALWAYS_FATAL_IF(it == mNodeForAddress.end(), "Can't be deleted while we hold sp<>");
+ LOG_ALWAYS_FATAL_IF(it->second.binder != binder,
+ "Caller of flushExcessBinderRefs using inconsistent arguments");
+
+ // if this is a local binder, then we want to get rid of all refcounts
+ // (tell the other process it can drop the binder when it wants to - we
+ // have a local sp<>, so we will drop it when we want to as well). if
+ // this is a remote binder, then we need to hold onto one refcount until
+ // it is dropped in BpBinder::onLastStrongRef
+ size_t targetRecd = binder->localBinder() ? 0 : 1;
+
+ // We have timesRecd RPC refcounts, but we only need to hold on to one
+ // when we keep the object. All additional dec strongs are sent
+ // immediately, we wait to send the last one in BpBinder::onLastDecStrong.
+ if (it->second.timesRecd != targetRecd) {
+ _l.unlock();
+
+ return session->sendDecStrongToTarget(address, targetRecd);
+ }
+
+ return OK;
+}
+
size_t RpcState::countBinders() {
std::lock_guard<std::mutex> _l(mNodeMutex);
return mNodeForAddress.size();
@@ -571,32 +595,43 @@
return OK;
}
-status_t RpcState::sendDecStrong(const sp<RpcSession::RpcConnection>& connection,
- const sp<RpcSession>& session, uint64_t addr) {
+status_t RpcState::sendDecStrongToTarget(const sp<RpcSession::RpcConnection>& connection,
+ const sp<RpcSession>& session, uint64_t addr,
+ size_t target) {
+ RpcDecStrong body = {
+ .address = RpcWireAddress::fromRaw(addr),
+ };
+
{
std::lock_guard<std::mutex> _l(mNodeMutex);
if (mTerminated) return DEAD_OBJECT; // avoid fatal only, otherwise races
auto it = mNodeForAddress.find(addr);
LOG_ALWAYS_FATAL_IF(it == mNodeForAddress.end(),
"Sending dec strong on unknown address %" PRIu64, addr);
- LOG_ALWAYS_FATAL_IF(it->second.timesRecd <= 0, "Bad dec strong %" PRIu64, addr);
- it->second.timesRecd--;
+ LOG_ALWAYS_FATAL_IF(it->second.timesRecd < target, "Can't dec count of %zu to %zu.",
+ it->second.timesRecd, target);
+
+ // typically this happens when multiple threads send dec refs at the
+ // same time - the transactions will get combined automatically
+ if (it->second.timesRecd == target) return OK;
+
+ body.amount = it->second.timesRecd - target;
+ it->second.timesRecd = target;
+
LOG_ALWAYS_FATAL_IF(nullptr != tryEraseNode(it),
"Bad state. RpcState shouldn't own received binder");
}
RpcWireHeader cmd = {
.command = RPC_COMMAND_DEC_STRONG,
- .bodySize = sizeof(RpcWireAddress),
+ .bodySize = sizeof(RpcDecStrong),
};
if (status_t status = rpcSend(connection, session, "dec ref header", &cmd, sizeof(cmd));
status != OK)
return status;
- if (status_t status = rpcSend(connection, session, "dec ref body", &addr, sizeof(addr));
- status != OK)
- return status;
- return OK;
+
+ return rpcSend(connection, session, "dec ref body", &body, sizeof(body));
}
status_t RpcState::getAndExecuteCommand(const sp<RpcSession::RpcConnection>& connection,
@@ -827,6 +862,12 @@
}
}
+ // Binder refs are flushed for oneway calls only after all calls which are
+ // built up are executed. Otherwise, they fill up the binder buffer.
+ if (addr != 0 && replyStatus == OK && !oneway) {
+ replyStatus = flushExcessBinderRefs(session, addr, target);
+ }
+
if (oneway) {
if (replyStatus != OK) {
ALOGW("Oneway call failed with error: %d", replyStatus);
@@ -875,6 +916,13 @@
goto processTransactInternalTailCall;
}
}
+
+ // done processing all the async commands on this binder that we can, so
+ // write decstrongs on the binder
+ if (addr != 0 && replyStatus == OK) {
+ return flushExcessBinderRefs(session, addr, target);
+ }
+
return OK;
}
@@ -916,16 +964,15 @@
status != OK)
return status;
- if (command.bodySize != sizeof(RpcWireAddress)) {
- ALOGE("Expecting %zu but got %" PRId32 " bytes for RpcWireAddress. Terminating!",
- sizeof(RpcWireAddress), command.bodySize);
+ if (command.bodySize != sizeof(RpcDecStrong)) {
+ ALOGE("Expecting %zu but got %" PRId32 " bytes for RpcDecStrong. Terminating!",
+ sizeof(RpcDecStrong), command.bodySize);
(void)session->shutdownAndWait(false);
return BAD_VALUE;
}
- RpcWireAddress* address = reinterpret_cast<RpcWireAddress*>(commandData.data());
+ RpcDecStrong* body = reinterpret_cast<RpcDecStrong*>(commandData.data());
- uint64_t addr = RpcWireAddress::toRaw(*address);
-
+ uint64_t addr = RpcWireAddress::toRaw(body->address);
std::unique_lock<std::mutex> _l(mNodeMutex);
auto it = mNodeForAddress.find(addr);
if (it == mNodeForAddress.end()) {
@@ -943,15 +990,19 @@
return BAD_VALUE;
}
- if (it->second.timesSent == 0) {
- ALOGE("No record of sending binder, but requested decStrong: %" PRIu64, addr);
+ if (it->second.timesSent < body->amount) {
+ ALOGE("Record of sending binder %zu times, but requested decStrong for %" PRIu64 " of %u",
+ it->second.timesSent, addr, body->amount);
return OK;
}
LOG_ALWAYS_FATAL_IF(it->second.sentRef == nullptr, "Inconsistent state, lost ref for %" PRIu64,
addr);
- it->second.timesSent--;
+ LOG_RPC_DETAIL("Processing dec strong of %" PRIu64 " by %u from %zu", addr, body->amount,
+ it->second.timesSent);
+
+ it->second.timesSent -= body->amount;
sp<IBinder> tempHold = tryEraseNode(it);
_l.unlock();
tempHold = nullptr; // destructor may make binder calls on this session
diff --git a/libs/binder/RpcState.h b/libs/binder/RpcState.h
index dcfb569..42e95e0 100644
--- a/libs/binder/RpcState.h
+++ b/libs/binder/RpcState.h
@@ -82,8 +82,29 @@
uint64_t address, uint32_t code, const Parcel& data,
const sp<RpcSession>& session, Parcel* reply,
uint32_t flags);
- [[nodiscard]] status_t sendDecStrong(const sp<RpcSession::RpcConnection>& connection,
- const sp<RpcSession>& session, uint64_t address);
+
+ /**
+ * The ownership model here carries an implicit strong refcount whenever a
+ * binder is sent across processes. Since we have a local strong count in
+ * sp<> over these objects, we only ever need to keep one of these. So,
+ * typically we tell the remote process that we drop all the implicit dec
+ * strongs, and we hold onto the last one. 'target' here is the target
+ * timesRecd (the number of remaining reference counts) we wish to keep.
+ * Typically this should be '0' or '1'. The target is used instead of an
+ * explicit decrement count in order to allow multiple threads to lower the
+ * number of counts simultaneously. Since we only lower the count to 0 when
+ * a binder is deleted, targets of '1' should only be sent when the caller
+ * owns a local strong reference to the binder. Larger targets may be used
+ * for testing, and to make the function generic, but generally this should
+ * be avoided because it would be hard to guarantee another thread doesn't
+ * lower the number of held refcounts to '1'. Note also, these refcounts
+ * must be sent actively. If they are sent when binders are deleted, this
+ * can cause leaks, since even remote binders carry an implicit strong ref
+ * when they are sent to another process.
+ */
+ [[nodiscard]] status_t sendDecStrongToTarget(const sp<RpcSession::RpcConnection>& connection,
+ const sp<RpcSession>& session, uint64_t address,
+ size_t target);
enum class CommandType {
ANY,
@@ -108,6 +129,13 @@
*/
[[nodiscard]] status_t onBinderEntering(const sp<RpcSession>& session, uint64_t address,
sp<IBinder>* out);
+ /**
+ * Called on incoming binders to update refcounting information. This should
+ * only be called when it is done as part of making progress on a
+ * transaction.
+ */
+ [[nodiscard]] status_t flushExcessBinderRefs(const sp<RpcSession>& session, uint64_t address,
+ const sp<IBinder>& binder);
size_t countBinders();
void dump();
diff --git a/libs/binder/RpcWireFormat.h b/libs/binder/RpcWireFormat.h
index a87aa07..171550e 100644
--- a/libs/binder/RpcWireFormat.h
+++ b/libs/binder/RpcWireFormat.h
@@ -85,7 +85,7 @@
*/
RPC_COMMAND_REPLY,
/**
- * follows is RpcWireAddress
+ * follows is RpcDecStrong
*
* note - this in the protocol directly instead of as a 'special
* transaction' in order to keep it as lightweight as possible (we don't
@@ -117,6 +117,13 @@
};
static_assert(sizeof(RpcWireHeader) == 16);
+struct RpcDecStrong {
+ RpcWireAddress address;
+ uint32_t amount;
+ uint32_t reserved;
+};
+static_assert(sizeof(RpcDecStrong) == 16);
+
struct RpcWireTransaction {
RpcWireAddress address;
uint32_t code;
diff --git a/libs/binder/include/binder/RpcSession.h b/libs/binder/include/binder/RpcSession.h
index 6a29c05..12d448d 100644
--- a/libs/binder/include/binder/RpcSession.h
+++ b/libs/binder/include/binder/RpcSession.h
@@ -176,7 +176,8 @@
friend RpcState;
explicit RpcSession(std::unique_ptr<RpcTransportCtx> ctx);
- [[nodiscard]] status_t sendDecStrong(uint64_t address);
+ // for 'target', see RpcState::sendDecStrongToTarget
+ [[nodiscard]] status_t sendDecStrongToTarget(uint64_t address, size_t target);
class EventListener : public virtual RefBase {
public: