Merge "rust: split transact into prepare and submit" am: 78793f22b8 am: 853e51c9fa
Original change: https://android-review.googlesource.com/c/platform/frameworks/native/+/1802250
Change-Id: Ib3c3ea4203158a69fc92f9ca72e5a83574ce01c8
diff --git a/libs/binder/rust/src/binder.rs b/libs/binder/rust/src/binder.rs
index dd0c7b8..41ceee5 100644
--- a/libs/binder/rust/src/binder.rs
+++ b/libs/binder/rust/src/binder.rs
@@ -152,20 +152,46 @@
/// available.
fn get_extension(&mut self) -> Result<Option<SpIBinder>>;
+ /// Create a Parcel that can be used with `submit_transact`.
+ fn prepare_transact(&self) -> Result<Parcel>;
+
/// Perform a generic operation with the object.
///
+ /// The provided [`Parcel`] must have been created by a call to
+ /// `prepare_transact` on the same binder.
+ ///
+ /// # Arguments
+ ///
+ /// * `code` - Transaction code for the operation.
+ /// * `data` - [`Parcel`] with input data.
+ /// * `flags` - Transaction flags, e.g. marking the transaction as
+ /// asynchronous ([`FLAG_ONEWAY`](FLAG_ONEWAY)).
+ fn submit_transact(
+ &self,
+ code: TransactionCode,
+ data: Parcel,
+ flags: TransactionFlags,
+ ) -> Result<Parcel>;
+
+ /// Perform a generic operation with the object. This is a convenience
+ /// method that internally calls `prepare_transact` followed by
+ /// `submit_transact.
+ ///
/// # Arguments
/// * `code` - Transaction code for the operation
- /// * `data` - [`Parcel`] with input data
- /// * `reply` - Optional [`Parcel`] for reply data
/// * `flags` - Transaction flags, e.g. marking the transaction as
/// asynchronous ([`FLAG_ONEWAY`](FLAG_ONEWAY))
+ /// * `input_callback` A callback for building the `Parcel`.
fn transact<F: FnOnce(&mut Parcel) -> Result<()>>(
&self,
code: TransactionCode,
flags: TransactionFlags,
input_callback: F,
- ) -> Result<Parcel>;
+ ) -> Result<Parcel> {
+ let mut parcel = self.prepare_transact()?;
+ input_callback(&mut parcel)?;
+ self.submit_transact(code, parcel, flags)
+ }
}
/// Interface of binder local or remote objects.
diff --git a/libs/binder/rust/src/parcel.rs b/libs/binder/rust/src/parcel.rs
index a0e991c..dad89ec 100644
--- a/libs/binder/rust/src/parcel.rs
+++ b/libs/binder/rust/src/parcel.rs
@@ -25,6 +25,7 @@
use std::convert::TryInto;
use std::mem::ManuallyDrop;
use std::ptr;
+use std::fmt;
mod file_descriptor;
mod parcelable;
@@ -96,6 +97,13 @@
let _ = ManuallyDrop::new(self);
ptr
}
+
+ pub(crate) fn is_owned(&self) -> bool {
+ match *self {
+ Self::Owned(_) => true,
+ Self::Borrowed(_) => false,
+ }
+ }
}
// Data serialization methods
@@ -412,6 +420,13 @@
}
}
+impl fmt::Debug for Parcel {
+ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+ f.debug_struct("Parcel")
+ .finish()
+ }
+}
+
#[cfg(test)]
impl Parcel {
/// Create a new parcel tied to a bogus binder. TESTING ONLY!
diff --git a/libs/binder/rust/src/proxy.rs b/libs/binder/rust/src/proxy.rs
index e1d01ef..68fa34b 100644
--- a/libs/binder/rust/src/proxy.rs
+++ b/libs/binder/rust/src/proxy.rs
@@ -235,13 +235,7 @@
}
impl<T: AsNative<sys::AIBinder>> IBinderInternal for T {
- /// Perform a binder transaction
- fn transact<F: FnOnce(&mut Parcel) -> Result<()>>(
- &self,
- code: TransactionCode,
- flags: TransactionFlags,
- input_callback: F,
- ) -> Result<Parcel> {
+ fn prepare_transact(&self) -> Result<Parcel> {
let mut input = ptr::null_mut();
let status = unsafe {
// Safety: `SpIBinder` guarantees that `self` always contains a
@@ -254,15 +248,25 @@
// pointer, or null.
sys::AIBinder_prepareTransaction(self.as_native() as *mut sys::AIBinder, &mut input)
};
+
status_result(status)?;
- let mut input = unsafe {
+
+ unsafe {
// Safety: At this point, `input` is either a valid, owned `AParcel`
// pointer, or null. `Parcel::owned` safely handles both cases,
// taking ownership of the parcel.
- Parcel::owned(input).ok_or(StatusCode::UNEXPECTED_NULL)?
- };
- input_callback(&mut input)?;
+ Parcel::owned(input).ok_or(StatusCode::UNEXPECTED_NULL)
+ }
+ }
+
+ fn submit_transact(
+ &self,
+ code: TransactionCode,
+ data: Parcel,
+ flags: TransactionFlags,
+ ) -> Result<Parcel> {
let mut reply = ptr::null_mut();
+ assert!(data.is_owned());
let status = unsafe {
// Safety: `SpIBinder` guarantees that `self` always contains a
// valid pointer to an `AIBinder`. Although `IBinder::transact` is
@@ -277,13 +281,13 @@
// only providing `on_transact` with an immutable reference to
// `self`.
//
- // This call takes ownership of the `input` parcel pointer, and
+ // This call takes ownership of the `data` parcel pointer, and
// passes ownership of the `reply` out parameter to its caller. It
// does not affect ownership of the `binder` parameter.
sys::AIBinder_transact(
self.as_native() as *mut sys::AIBinder,
code,
- &mut input.into_raw(),
+ &mut data.into_raw(),
&mut reply,
flags,
)
diff --git a/libs/binder/rust/tests/integration.rs b/libs/binder/rust/tests/integration.rs
index ac9eed2..335e8d8 100644
--- a/libs/binder/rust/tests/integration.rs
+++ b/libs/binder/rust/tests/integration.rs
@@ -697,4 +697,27 @@
assert!(!(service1 > service1));
assert_eq!(service1 < service2, !(service2 < service1));
}
+
+ #[test]
+ fn binder_parcel_mixup() {
+ let service1 = BnTest::new_binder(
+ TestService::new("testing_service1"),
+ BinderFeatures::default(),
+ );
+ let service2 = BnTest::new_binder(
+ TestService::new("testing_service2"),
+ BinderFeatures::default(),
+ );
+
+ let service1 = service1.as_binder();
+ let service2 = service2.as_binder();
+
+ let parcel = service1.prepare_transact().unwrap();
+ let res = service2.submit_transact(super::TestTransactionCode::Test as binder::TransactionCode, parcel, 0);
+
+ match res {
+ Ok(_) => panic!("submit_transact should fail"),
+ Err(err) => assert_eq!(err, binder::StatusCode::BAD_VALUE),
+ }
+ }
}