From 438cce8a7489d4da97ed6fa58e3e8f3d71e5021e Mon Sep 17 00:00:00 2001 From: Steven Moreland Date: Fri, 2 Apr 2021 18:04:08 +0000 Subject: libbinder: RPC avoid copy of transaction data The socket code here needs a lot more optimization/analysis, but using setIpcDataReference here is important so that we know internally whether a Parcel is owned or not. This will allow for stronger checks in markFor* functions which control the Parcel format. Bug: 167966510 Test: binderRpcTest Change-Id: Id2b0fc02972936f3e3b7382e7b56f7631a621e97 --- libs/binder/Parcel.cpp | 3 +++ 1 file changed, 3 insertions(+) (limited to 'libs/binder/Parcel.cpp') diff --git a/libs/binder/Parcel.cpp b/libs/binder/Parcel.cpp index 34a474b252..96d12cae14 100644 --- a/libs/binder/Parcel.cpp +++ b/libs/binder/Parcel.cpp @@ -2100,6 +2100,9 @@ size_t Parcel::ipcObjectsCount() const void Parcel::ipcSetDataReference(const uint8_t* data, size_t dataSize, const binder_size_t* objects, size_t objectsCount, release_func relFunc) { + // this code uses 'mOwner == nullptr' to understand whether it owns memory + LOG_ALWAYS_FATAL_IF(relFunc == nullptr, "must provide cleanup function"); + freeData(); mData = const_cast(data); -- cgit v1.2.3-59-g8ed1b From 1fda67b02a7d3b78c1c3ee92734e096715190a14 Mon Sep 17 00:00:00 2001 From: Steven Moreland Date: Fri, 2 Apr 2021 18:35:50 +0000 Subject: libbinder: ensure entire Parcel is the same format Extra check in preparation for versioning work/as a guard against misuse of this API. Note - these functions are very much like constructors, but they can't be because Parcel is that errorprone type of object which resets its internal state (for better or worse). Bug: 182938972 Test: binderRpcTest Change-Id: Ibdbe8161db9d0c8fba86f1c691c73aab49b21bc0 --- libs/binder/Parcel.cpp | 5 +++++ libs/binder/include/binder/Parcel.h | 8 ++++---- libs/binder/include/binder/RpcConnection.h | 1 + libs/binder/ndk/ibinder.cpp | 2 ++ libs/binder/ndk/parcel_internal.h | 6 +----- libs/binder/tests/binderRpcTest.cpp | 7 +++++++ 6 files changed, 20 insertions(+), 9 deletions(-) (limited to 'libs/binder/Parcel.cpp') diff --git a/libs/binder/Parcel.cpp b/libs/binder/Parcel.cpp index 96d12cae14..3f0b0df4f6 100644 --- a/libs/binder/Parcel.cpp +++ b/libs/binder/Parcel.cpp @@ -555,12 +555,17 @@ void Parcel::markSensitive() const } void Parcel::markForBinder(const sp& binder) { + LOG_ALWAYS_FATAL_IF(mData != nullptr, "format must be set before data is written"); + if (binder && binder->remoteBinder() && binder->remoteBinder()->isRpcBinder()) { markForRpc(binder->remoteBinder()->getPrivateAccessorForId().rpcConnection()); } } void Parcel::markForRpc(const sp& connection) { + LOG_ALWAYS_FATAL_IF(mData != nullptr && mOwner == nullptr, + "format must be set before data is written OR on IPC data"); + LOG_ALWAYS_FATAL_IF(connection == nullptr, "markForRpc requires connection"); mConnection = connection; } diff --git a/libs/binder/include/binder/Parcel.h b/libs/binder/include/binder/Parcel.h index 957837233b..211790d14c 100644 --- a/libs/binder/include/binder/Parcel.h +++ b/libs/binder/include/binder/Parcel.h @@ -101,10 +101,6 @@ public: // is for an RPC transaction). void markForBinder(const sp& binder); - // Whenever possible, markForBinder should be preferred. This method is - // called automatically on reply Parcels for RPC transactions. - void markForRpc(const sp& connection); - // Whether this Parcel is written for RPC transactions (after calls to // markForBinder or markForRpc). bool isForRpc() const; @@ -540,6 +536,10 @@ private: const binder_size_t* objects, size_t objectsCount, release_func relFunc); + // Whenever possible, markForBinder should be preferred. This method is + // called automatically on reply Parcels for RPC transactions. + void markForRpc(const sp& connection); + status_t finishWrite(size_t len); void releaseObjects(); void acquireObjects(); diff --git a/libs/binder/include/binder/RpcConnection.h b/libs/binder/include/binder/RpcConnection.h index a300575454..efa922dbda 100644 --- a/libs/binder/include/binder/RpcConnection.h +++ b/libs/binder/include/binder/RpcConnection.h @@ -106,6 +106,7 @@ public: }; private: + friend sp; RpcConnection(); bool addServer(const SocketAddress& address); diff --git a/libs/binder/ndk/ibinder.cpp b/libs/binder/ndk/ibinder.cpp index 0f59de4309..76afd4cc2c 100644 --- a/libs/binder/ndk/ibinder.cpp +++ b/libs/binder/ndk/ibinder.cpp @@ -598,6 +598,8 @@ binder_status_t AIBinder_prepareTransaction(AIBinder* binder, AParcel** in) { } *in = new AParcel(binder); + (*in)->get()->markForBinder(binder->getBinder()); + status_t status = (*in)->get()->writeInterfaceToken(clazz->getInterfaceDescriptor()); binder_status_t ret = PruneStatusT(status); diff --git a/libs/binder/ndk/parcel_internal.h b/libs/binder/ndk/parcel_internal.h index aebc7f4539..b4f6358841 100644 --- a/libs/binder/ndk/parcel_internal.h +++ b/libs/binder/ndk/parcel_internal.h @@ -29,11 +29,7 @@ struct AParcel { explicit AParcel(AIBinder* binder) : AParcel(binder, new ::android::Parcel, true /*owns*/) {} AParcel(AIBinder* binder, ::android::Parcel* parcel, bool owns) - : mBinder(binder), mParcel(parcel), mOwns(owns) { - if (binder != nullptr) { - parcel->markForBinder(binder->getBinder()); - } - } + : mBinder(binder), mParcel(parcel), mOwns(owns) {} ~AParcel() { if (mOwns) { diff --git a/libs/binder/tests/binderRpcTest.cpp b/libs/binder/tests/binderRpcTest.cpp index 5f68a25f74..985d086d73 100644 --- a/libs/binder/tests/binderRpcTest.cpp +++ b/libs/binder/tests/binderRpcTest.cpp @@ -44,6 +44,13 @@ namespace android { +TEST(BinderRpcParcel, EntireParcelFormatted) { + Parcel p; + p.writeInt32(3); + + EXPECT_DEATH(p.markForBinder(sp::make()), ""); +} + using android::binder::Status; #define EXPECT_OK(status) \ -- cgit v1.2.3-59-g8ed1b