libbinder_ndk: finalizeTransaction->AParcel_delete
AIBinder_finalizeTransaction has been removed and replaced with
AParcel_delete. This makes error handling easier and makes it possible
to clean up the in parcel without executing a transaction.
This comes with the loss of the check that the parcel is read in its
entirety as this check may be moved into libbinder or into an API above
this in order to simplify error handling.
Bug: 111445392
Test: runtests.sh
Change-Id: I4e0d0324f0333b02e6a47c7190819822850bee93
diff --git a/libs/binder/ndk/AIBinder.cpp b/libs/binder/ndk/AIBinder.cpp
index 58cbf56..40d3a00 100644
--- a/libs/binder/ndk/AIBinder.cpp
+++ b/libs/binder/ndk/AIBinder.cpp
@@ -279,12 +279,6 @@
return status;
}
-using AutoParcelDestroyer = std::unique_ptr<AParcel*, void (*)(AParcel**)>;
-static void destroy_parcel(AParcel** parcel) {
- delete *parcel;
- *parcel = nullptr;
-}
-
binder_status_t AIBinder_transact(AIBinder* binder, transaction_code_t code, AParcel** in,
AParcel** out, binder_flags_t flags) {
if (in == nullptr) {
@@ -292,9 +286,10 @@
return EX_NULL_POINTER;
}
+ using AutoParcelDestroyer = std::unique_ptr<AParcel*, void (*)(AParcel**)>;
// This object is the input to the transaction. This function takes ownership of it and deletes
// it.
- AutoParcelDestroyer forIn(in, destroy_parcel);
+ AutoParcelDestroyer forIn(in, AParcel_delete);
if (!isUserCommand(code)) {
LOG(ERROR) << __func__ << ": Only user-defined transactions can be made from the NDK.";
@@ -329,33 +324,3 @@
return parcelStatus;
}
-
-binder_status_t AIBinder_finalizeTransaction(AIBinder* binder, AParcel** out) {
- if (out == nullptr) {
- LOG(ERROR) << __func__ << ": requires non-null out parameter";
- return EX_NULL_POINTER;
- }
-
- // This object is the input to the transaction. This function takes ownership of it and deletes
- // it.
- AutoParcelDestroyer forOut(out, destroy_parcel);
-
- if (binder == nullptr || *out == nullptr) {
- LOG(ERROR) << __func__ << ": requires non-null parameters.";
- return EX_NULL_POINTER;
- }
-
- if ((*out)->getBinder() != binder) {
- LOG(ERROR) << __func__ << ": parcel is associated with binder object " << binder
- << " but called with " << (*out)->getBinder();
- return EX_ILLEGAL_STATE;
- }
-
- if ((**out)->dataAvail() != 0) {
- LOG(ERROR) << __func__
- << ": Only part of this transaction was read. There is remaining data left.";
- return EX_ILLEGAL_STATE;
- }
-
- return EX_NONE;
-}
diff --git a/libs/binder/ndk/AParcel.cpp b/libs/binder/ndk/AParcel.cpp
index f39e732..93384d6 100644
--- a/libs/binder/ndk/AParcel.cpp
+++ b/libs/binder/ndk/AParcel.cpp
@@ -25,6 +25,15 @@
using ::android::Parcel;
using ::android::sp;
+void AParcel_delete(AParcel** parcel) {
+ if (parcel == nullptr) {
+ return;
+ }
+
+ delete *parcel;
+ *parcel = nullptr;
+}
+
binder_status_t AParcel_writeStrongBinder(AParcel* parcel, AIBinder* binder) {
return (*parcel)->writeStrongBinder(binder->getBinder());
}
diff --git a/libs/binder/ndk/include_ndk/android/binder_ibinder.h b/libs/binder/ndk/include_ndk/android/binder_ibinder.h
index f752008..91a87fd 100644
--- a/libs/binder/ndk/include_ndk/android/binder_ibinder.h
+++ b/libs/binder/ndk/include_ndk/android/binder_ibinder.h
@@ -186,10 +186,9 @@
/**
* A transaction is a series of calls to these functions which looks this
* - call AIBinder_prepareTransaction
- * - fill out parcel with in parameters (lifetime of the 'in' variable)
+ * - fill out the in parcel with parameters (lifetime of the 'in' variable)
* - call AIBinder_transact
- * - fill out parcel with out parameters (lifetime of the 'out' variable)
- * - call AIBinder_finalizeTransaction
+ * - read results from the out parcel (lifetime of the 'out' variable)
*/
/**
@@ -200,7 +199,10 @@
* can be avoided. This AIBinder must be either built with a class or associated with a class before
* using this API.
*
- * This does not affect the ownership of binder.
+ * This does not affect the ownership of binder. When this function succeeds, the in parcel's
+ * ownership is passed to the caller. At this point, the parcel can be filled out and passed to
+ * AIBinder_transact. Alternatively, if there is an error while filling out the parcel, it can be
+ * deleted with AParcel_delete.
*/
binder_status_t AIBinder_prepareTransaction(AIBinder* binder, AParcel** in);
@@ -213,19 +215,12 @@
* remote process has processed the transaction, and the out parcel will contain the output data
* from transaction.
*
- * This does not affect the ownership of binder.
+ * This does not affect the ownership of binder. The out parcel's ownership is passed to the caller
+ * and must be released with AParcel_delete when finished reading.
*/
binder_status_t AIBinder_transact(AIBinder* binder, transaction_code_t code, AParcel** in,
AParcel** out, binder_flags_t flags);
-/**
- * This takes ownership of the out parcel and automatically deletes it. Additional checks for
- * security or debugging maybe performed internally.
- *
- * This does not affect the ownership of binder.
- */
-binder_status_t AIBinder_finalizeTransaction(AIBinder* binder, AParcel** out);
-
/*
* This does not take any ownership of the input binder, but it can be used to retrieve it if
* something else in some process still holds a reference to it.
diff --git a/libs/binder/ndk/include_ndk/android/binder_parcel.h b/libs/binder/ndk/include_ndk/android/binder_parcel.h
index 091ae8e..19925f7 100644
--- a/libs/binder/ndk/include_ndk/android/binder_parcel.h
+++ b/libs/binder/ndk/include_ndk/android/binder_parcel.h
@@ -35,6 +35,11 @@
typedef struct AParcel AParcel;
/**
+ * Cleans up a parcel and sets it to nullptr.
+ */
+void AParcel_delete(AParcel** parcel);
+
+/**
* Writes an AIBinder to the next location in a non-null parcel. Can be null.
*/
binder_status_t AParcel_writeStrongBinder(AParcel* parcel, AIBinder* binder);
diff --git a/libs/binder/ndk/test/iface.cpp b/libs/binder/ndk/test/iface.cpp
index 27553c8..a1aa0fa 100644
--- a/libs/binder/ndk/test/iface.cpp
+++ b/libs/binder/ndk/test/iface.cpp
@@ -81,7 +81,8 @@
int32_t out;
CHECK(EX_NONE == AParcel_readInt32(parcelOut, &out));
- CHECK(EX_NONE == AIBinder_finalizeTransaction(mBinder, &parcelOut));
+ AParcel_delete(&parcelOut);
+
return out;
}