Merge changes Ibdbe8161,Id2b0fc02
* changes:
libbinder: ensure entire Parcel is the same format
libbinder: RPC avoid copy of transaction data
diff --git a/libs/binder/Parcel.cpp b/libs/binder/Parcel.cpp
index 34a474b..3f0b0df 100644
--- a/libs/binder/Parcel.cpp
+++ b/libs/binder/Parcel.cpp
@@ -555,12 +555,17 @@
}
void Parcel::markForBinder(const sp<IBinder>& 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<RpcConnection>& 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;
}
@@ -2100,6 +2105,9 @@
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<uint8_t*>(data);
diff --git a/libs/binder/RpcState.cpp b/libs/binder/RpcState.cpp
index 3b3adca..755ff35 100644
--- a/libs/binder/RpcState.cpp
+++ b/libs/binder/RpcState.cpp
@@ -312,8 +312,8 @@
return waitForReply(fd, connection, reply);
}
-static void cleanup_data(Parcel* p, const uint8_t* data, size_t dataSize,
- const binder_size_t* objects, size_t objectsCount) {
+static void cleanup_reply_data(Parcel* p, const uint8_t* data, size_t dataSize,
+ const binder_size_t* objects, size_t objectsCount) {
(void)p;
delete[] const_cast<uint8_t*>(data - offsetof(RpcWireReply, data));
(void)dataSize;
@@ -351,7 +351,7 @@
if (rpcReply->status != OK) return rpcReply->status;
reply->ipcSetDataReference(rpcReply->data, command.bodySize - offsetof(RpcWireReply, data),
- nullptr, 0, cleanup_data);
+ nullptr, 0, cleanup_reply_data);
reply->markForRpc(connection);
@@ -427,6 +427,15 @@
return processTransactInternal(fd, connection, std::move(transactionData));
}
+static void do_nothing_to_transact_data(Parcel* p, const uint8_t* data, size_t dataSize,
+ const binder_size_t* objects, size_t objectsCount) {
+ (void)p;
+ (void)data;
+ (void)dataSize;
+ (void)objects;
+ (void)objectsCount;
+}
+
status_t RpcState::processTransactInternal(const base::unique_fd& fd,
const sp<RpcConnection>& connection,
std::vector<uint8_t>&& transactionData) {
@@ -490,7 +499,12 @@
}
Parcel data;
- data.setData(transaction->data, transactionData.size() - offsetof(RpcWireTransaction, data));
+ // transaction->data is owned by this function. Parcel borrows this data and
+ // only holds onto it for the duration of this function call. Parcel will be
+ // deleted before the 'transactionData' object.
+ data.ipcSetDataReference(transaction->data,
+ transactionData.size() - offsetof(RpcWireTransaction, data),
+ nullptr /*object*/, 0 /*objectCount*/, do_nothing_to_transact_data);
data.markForRpc(connection);
Parcel reply;
diff --git a/libs/binder/include/binder/Parcel.h b/libs/binder/include/binder/Parcel.h
index 9578372..211790d 100644
--- a/libs/binder/include/binder/Parcel.h
+++ b/libs/binder/include/binder/Parcel.h
@@ -101,10 +101,6 @@
// is for an RPC transaction).
void markForBinder(const sp<IBinder>& binder);
- // Whenever possible, markForBinder should be preferred. This method is
- // called automatically on reply Parcels for RPC transactions.
- void markForRpc(const sp<RpcConnection>& connection);
-
// Whether this Parcel is written for RPC transactions (after calls to
// markForBinder or markForRpc).
bool isForRpc() const;
@@ -540,6 +536,10 @@
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<RpcConnection>& 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 a300575..efa922d 100644
--- a/libs/binder/include/binder/RpcConnection.h
+++ b/libs/binder/include/binder/RpcConnection.h
@@ -106,6 +106,7 @@
};
private:
+ friend sp<RpcConnection>;
RpcConnection();
bool addServer(const SocketAddress& address);
diff --git a/libs/binder/ndk/ibinder.cpp b/libs/binder/ndk/ibinder.cpp
index 0f59de4..76afd4c 100644
--- a/libs/binder/ndk/ibinder.cpp
+++ b/libs/binder/ndk/ibinder.cpp
@@ -598,6 +598,8 @@
}
*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 aebc7f4..b4f6358 100644
--- a/libs/binder/ndk/parcel_internal.h
+++ b/libs/binder/ndk/parcel_internal.h
@@ -29,11 +29,7 @@
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 5f68a25..985d086 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<BBinder>::make()), "");
+}
+
using android::binder::Status;
#define EXPECT_OK(status) \