From ee78e760a7727947b02972af548c71b2f38019e2 Mon Sep 17 00:00:00 2001 From: Steven Moreland Date: Wed, 5 May 2021 21:12:51 +0000 Subject: libbinder: delete dead server objects We can do more active cleanup, and we can do kinder cleanup, but for now fix leaks (future considerations/TODOs left in code). Bug: 185167543 Test: binderRpcTest Change-Id: Ide06476aefd72cbc46ba5fba095244a5448e493b --- libs/binder/RpcServer.cpp | 12 ++++++++++++ libs/binder/RpcSession.cpp | 20 ++++++++++++++++++++ libs/binder/include/binder/RpcServer.h | 4 ++++ libs/binder/include/binder/RpcSession.h | 2 ++ 4 files changed, 38 insertions(+) diff --git a/libs/binder/RpcServer.cpp b/libs/binder/RpcServer.cpp index 786e2db3b3..aaebb23f6f 100644 --- a/libs/binder/RpcServer.cpp +++ b/libs/binder/RpcServer.cpp @@ -216,4 +216,16 @@ bool RpcServer::setupSocketServer(const RpcSocketAddress& addr) { return true; } +void RpcServer::onSessionTerminating(const sp& session) { + auto id = session->mId; + LOG_ALWAYS_FATAL_IF(id == std::nullopt, "Server sessions must be initialized with ID"); + LOG_RPC_DETAIL("Dropping session %d", *id); + + std::lock_guard _l(mLock); + auto it = mSessions.find(*id); + LOG_ALWAYS_FATAL_IF(it == mSessions.end(), "Bad state, unknown session id %d", *id); + LOG_ALWAYS_FATAL_IF(it->second != session, "Bad state, session has id mismatch %d", *id); + (void)mSessions.erase(it); +} + } // namespace android diff --git a/libs/binder/RpcSession.cpp b/libs/binder/RpcSession.cpp index 09ec20dbf0..bf998c1cd8 100644 --- a/libs/binder/RpcSession.cpp +++ b/libs/binder/RpcSession.cpp @@ -24,6 +24,7 @@ #include #include +#include #include #include @@ -168,6 +169,22 @@ void RpcSession::join(unique_fd client) { "bad state: connection object guaranteed to be in list"); } +void RpcSession::terminateLocked() { + // TODO(b/185167543): + // - kindly notify other side of the connection of termination (can't be + // locked) + // - prevent new client/servers from being added + // - stop all threads which are currently reading/writing + // - terminate RpcState? + + if (mTerminated) return; + + sp server = mForServer.promote(); + if (server) { + server->onSessionTerminating(sp::fromExisting(this)); + } +} + wp RpcSession::server() { return mForServer; } @@ -264,6 +281,9 @@ bool RpcSession::removeServerConnection(const sp& connection) { std::lock_guard _l(mMutex); if (auto it = std::find(mServers.begin(), mServers.end(), connection); it != mServers.end()) { mServers.erase(it); + if (mServers.size() == 0) { + terminateLocked(); + } return true; } return false; diff --git a/libs/binder/include/binder/RpcServer.h b/libs/binder/include/binder/RpcServer.h index c98151d293..8c9ba00f06 100644 --- a/libs/binder/include/binder/RpcServer.h +++ b/libs/binder/include/binder/RpcServer.h @@ -109,6 +109,10 @@ public: ~RpcServer(); + // internal use only + + void onSessionTerminating(const sp& session); + private: friend sp; RpcServer(); diff --git a/libs/binder/include/binder/RpcSession.h b/libs/binder/include/binder/RpcSession.h index 3f58b2ce8e..d49d48d40e 100644 --- a/libs/binder/include/binder/RpcSession.h +++ b/libs/binder/include/binder/RpcSession.h @@ -118,6 +118,7 @@ private: void startThread(base::unique_fd client); void join(base::unique_fd client); + void terminateLocked(); struct RpcConnection : public RefBase { base::unique_fd fd; @@ -196,6 +197,7 @@ private: // TODO(b/185167543): allow sharing between different sessions in a // process? (or combine with mServers) std::map mThreads; + bool mTerminated = false; }; } // namespace android -- cgit v1.2.3-59-g8ed1b From 2ff0d47cf68ae3e6a0593220205ba06b23db73fc Mon Sep 17 00:00:00 2001 From: Steven Moreland Date: Wed, 5 May 2021 22:20:40 +0000 Subject: libbinder: RPC avoid server shutdown crash Server thread needs to detach itself before removing its entry in RpcSession. This was causing the (upcoming) RPC server fuzzer to fail very frequently. Bug: 182938024 Test: w/ fuzzer, binderRpcTest Change-Id: I004747971997ed2ae90613757836eb6f68473abd --- libs/binder/RpcSession.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/libs/binder/RpcSession.cpp b/libs/binder/RpcSession.cpp index bf998c1cd8..f4a3cffa24 100644 --- a/libs/binder/RpcSession.cpp +++ b/libs/binder/RpcSession.cpp @@ -143,8 +143,10 @@ void RpcSession::startThread(unique_fd client) { holdThis->join(unique_fd(fd)); { std::lock_guard _l(holdThis->mMutex); - size_t erased = mThreads.erase(std::this_thread::get_id()); - LOG_ALWAYS_FATAL_IF(erased != 0, "Could not erase thread."); + auto it = mThreads.find(std::this_thread::get_id()); + LOG_ALWAYS_FATAL_IF(it == mThreads.end()); + it->second.detach(); + mThreads.erase(it); } }); mThreads[thread.get_id()] = std::move(thread); -- cgit v1.2.3-59-g8ed1b From e8393349fe438e65babd434f0a37c976ef0ff3ed Mon Sep 17 00:00:00 2001 From: Steven Moreland Date: Wed, 5 May 2021 23:27:53 +0000 Subject: libbinder: RPC avoid abort on too big allocations We may want some more heuristic checks here, since allocations that are just barely too big may interrupt the operations of other threads that can't handle failing allocations, but this is a start so that too-big allocations won't cause an abort. Bug: 182938024 Test: binderRpcTest + running WIP fuzzer Change-Id: Id9f1b63962cd22b7c799b14e595c47ea8628354f --- libs/binder/RpcState.cpp | 35 ++++++++++++++++++++++++++--------- libs/binder/RpcState.h | 19 +++++++++++++++++-- 2 files changed, 43 insertions(+), 11 deletions(-) diff --git a/libs/binder/RpcState.cpp b/libs/binder/RpcState.cpp index 96190dc03c..976351c6c4 100644 --- a/libs/binder/RpcState.cpp +++ b/libs/binder/RpcState.cpp @@ -326,7 +326,11 @@ status_t RpcState::transact(const base::unique_fd& fd, const RpcAddress& address .asyncNumber = asyncNumber, }; - std::vector transactionData(sizeof(RpcWireTransaction) + data.dataSize()); + ByteVec transactionData(sizeof(RpcWireTransaction) + data.dataSize()); + if (!transactionData.valid()) { + return NO_MEMORY; + } + memcpy(transactionData.data() + 0, &transaction, sizeof(RpcWireTransaction)); memcpy(transactionData.data() + sizeof(RpcWireTransaction), data.data(), data.dataSize()); @@ -379,9 +383,12 @@ status_t RpcState::waitForReply(const base::unique_fd& fd, const sp& if (status != OK) return status; } - uint8_t* data = new uint8_t[command.bodySize]; + ByteVec data(command.bodySize); + if (!data.valid()) { + return NO_MEMORY; + } - if (!rpcRec(fd, "reply body", data, command.bodySize)) { + if (!rpcRec(fd, "reply body", data.data(), command.bodySize)) { return DEAD_OBJECT; } @@ -391,9 +398,10 @@ status_t RpcState::waitForReply(const base::unique_fd& fd, const sp& terminate(); return BAD_VALUE; } - RpcWireReply* rpcReply = reinterpret_cast(data); + RpcWireReply* rpcReply = reinterpret_cast(data.data()); if (rpcReply->status != OK) return rpcReply->status; + data.release(); reply->ipcSetDataReference(rpcReply->data, command.bodySize - offsetof(RpcWireReply, data), nullptr, 0, cleanup_reply_data); @@ -461,7 +469,10 @@ status_t RpcState::processTransact(const base::unique_fd& fd, const sp transactionData(command.bodySize); + ByteVec transactionData(command.bodySize); + if (!transactionData.valid()) { + return NO_MEMORY; + } if (!rpcRec(fd, "transaction body", transactionData.data(), transactionData.size())) { return DEAD_OBJECT; } @@ -479,7 +490,7 @@ static void do_nothing_to_transact_data(Parcel* p, const uint8_t* data, size_t d } status_t RpcState::processTransactInternal(const base::unique_fd& fd, const sp& session, - std::vector&& transactionData) { + ByteVec transactionData) { if (transactionData.size() < sizeof(RpcWireTransaction)) { ALOGE("Expecting %zu but got %zu bytes for RpcWireTransaction. Terminating!", sizeof(RpcWireTransaction), transactionData.size()); @@ -630,7 +641,7 @@ status_t RpcState::processTransactInternal(const base::unique_fd& fd, const sp data = std::move( + ByteVec data = std::move( const_cast(it->second.asyncTodo.top()).data); it->second.asyncTodo.pop(); _l.unlock(); @@ -644,7 +655,10 @@ status_t RpcState::processTransactInternal(const base::unique_fd& fd, const sp replyData(sizeof(RpcWireReply) + reply.dataSize()); + ByteVec replyData(sizeof(RpcWireReply) + reply.dataSize()); + if (!replyData.valid()) { + return NO_MEMORY; + } memcpy(replyData.data() + 0, &rpcReply, sizeof(RpcWireReply)); memcpy(replyData.data() + sizeof(RpcWireReply), reply.data(), reply.dataSize()); @@ -671,7 +685,10 @@ status_t RpcState::processTransactInternal(const base::unique_fd& fd, const sp commandData(command.bodySize); + ByteVec commandData(command.bodySize); + if (!commandData.valid()) { + return NO_MEMORY; + } if (!rpcRec(fd, "dec ref body", commandData.data(), commandData.size())) { return DEAD_OBJECT; } diff --git a/libs/binder/RpcState.h b/libs/binder/RpcState.h index 3f3eb1c2ed..83d034497f 100644 --- a/libs/binder/RpcState.h +++ b/libs/binder/RpcState.h @@ -21,6 +21,7 @@ #include #include +#include #include namespace android { @@ -100,6 +101,20 @@ private: */ void terminate(); + // alternative to std::vector that doesn't abort on too big of allocations + struct ByteVec { + explicit ByteVec(size_t size) + : mData(size > 0 ? new (std::nothrow) uint8_t[size] : nullptr), mSize(size) {} + bool valid() { return mSize == 0 || mData != nullptr; } + size_t size() { return mSize; } + uint8_t* data() { return mData.get(); } + uint8_t* release() { return mData.release(); } + + private: + std::unique_ptr mData; + size_t mSize; + }; + [[nodiscard]] bool rpcSend(const base::unique_fd& fd, const char* what, const void* data, size_t size); [[nodiscard]] bool rpcRec(const base::unique_fd& fd, const char* what, void* data, size_t size); @@ -113,7 +128,7 @@ private: const RpcWireHeader& command); [[nodiscard]] status_t processTransactInternal(const base::unique_fd& fd, const sp& session, - std::vector&& transactionData); + ByteVec transactionData); [[nodiscard]] status_t processDecStrong(const base::unique_fd& fd, const RpcWireHeader& command); @@ -148,7 +163,7 @@ private: // async transaction queue, _only_ for local binder struct AsyncTodo { - std::vector data; // most convenient format, to move it here + ByteVec data; uint64_t asyncNumber = 0; bool operator<(const AsyncTodo& o) const { -- cgit v1.2.3-59-g8ed1b From b6a1e7eb34980400bdbabcab756c5a244b1f525b Mon Sep 17 00:00:00 2001 From: Steven Moreland Date: Wed, 5 May 2021 23:36:46 +0000 Subject: libbinder: remove RpcState deadlock debug logs 'dump' internally takes a lock, and there were two calls to it here which would actually deadlock. Simply removing them to move fuzzer forward. Bug: 182938024 Test: fuzzer Change-Id: Ic07193955f0794fd1fc11d7e89c65b469a43323c --- libs/binder/RpcState.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/libs/binder/RpcState.cpp b/libs/binder/RpcState.cpp index 976351c6c4..20fdbfe954 100644 --- a/libs/binder/RpcState.cpp +++ b/libs/binder/RpcState.cpp @@ -511,7 +511,6 @@ status_t RpcState::processTransactInternal(const base::unique_fd& fd, const spsecond.binder.promote(); @@ -707,7 +706,6 @@ status_t RpcState::processDecStrong(const base::unique_fd& fd, const RpcWireHead auto it = mNodeForAddress.find(addr); if (it == mNodeForAddress.end()) { ALOGE("Unknown binder address %s for dec strong.", addr.toString().c_str()); - dump(); return OK; } -- cgit v1.2.3-59-g8ed1b