Merge changes Ieb537412,I2339a8e0

* changes:
  libbinder: oneway calls delay refcounting
  libbinder: dec refs include count

6 files changed, 125 insertions, 33 deletions

diff --git a/libs/binder/Parcel.cpp b/libs/binder/Parcel.cpp
index 6644187507..6ce09226d9 100644
--- a/libs/binder/Parcel.cpp
+++ b/libs/binder/Parcel.cpp
@@ -291,6 +291,9 @@ status_t Parcel::unflattenBinder(sp<IBinder>* out) const
             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 38958c93cb..dafb33942a 100644
--- a/libs/binder/RpcSession.cpp
+++ b/libs/binder/RpcSession.cpp
@@ -217,15 +217,17 @@ status_t RpcSession::transact(const sp<IBinder>& binder, uint32_t code, const Pa
 }
 
 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 1e86104392..3ff13bcd9b 100644
--- a/libs/binder/RpcState.cpp
+++ b/libs/binder/RpcState.cpp
@@ -152,7 +152,7 @@ status_t RpcState::onBinderEntering(const sp<RpcSession>& session, uint64_t addr
         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 @@ status_t RpcState::onBinderEntering(const sp<RpcSession>& session, uint64_t addr
 
         // 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 @@ status_t RpcState::onBinderEntering(const sp<RpcSession>& session, uint64_t addr
     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 @@ status_t RpcState::waitForReply(const sp<RpcSession::RpcConnection>& connection,
     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 @@ processTransactInternalTailCall:
         }
     }
 
+    // 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 @@ processTransactInternalTailCall:
                 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_t RpcState::processDecStrong(const sp<RpcSession::RpcConnection>& connect
         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());
-
-    uint64_t addr = RpcWireAddress::toRaw(*address);
+    RpcDecStrong* body = reinterpret_cast<RpcDecStrong*>(commandData.data());
 
+    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 @@ status_t RpcState::processDecStrong(const sp<RpcSession::RpcConnection>& connect
         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 dcfb5699e8..42e95e0c10 100644
--- a/libs/binder/RpcState.h
+++ b/libs/binder/RpcState.h
@@ -82,8 +82,29 @@ public:
                                            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 @@ public:
      */
     [[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 a87aa074a9..171550e620 100644
--- a/libs/binder/RpcWireFormat.h
+++ b/libs/binder/RpcWireFormat.h
@@ -85,7 +85,7 @@ enum : uint32_t {
      */
     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 @@ struct RpcWireHeader {
 };
 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 6a29c05e36..12d448d1e4 100644
--- a/libs/binder/include/binder/RpcSession.h
+++ b/libs/binder/include/binder/RpcSession.h
@@ -176,7 +176,8 @@ private:
     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: