binder: fix decoding of vintf stability ParcelableHolder

Unlike the C++ implementation, `ParcelableHolder`s in rust are decoded
into a new object. The new object was losing the original stability
level, always reverting to local stability, so vintf parcelables would
fail the assertion.

We could replicate the C++ implementation, but it would require a lot of
API refactoring and it is easy to mess up. Instead, we make the
stability part of the type. This somewhat matches AIDL, where the
stability is a static annotation on the type containing the
`ParcelableHolder`.

Bug: 366383257
Test: m
Change-Id: Ic4654830d404fbf18046660eeb0129ab8abf7e61

Author: fkm3

libs/binder/rust/src/binder.rs

@@ -136,6 +136,31 @@ impl TryFrom<i32> for Stability {
     }
 }
 
+/// Same as `Stability`, but in the form of a trait. Used when the stability should be encoded in
+/// the type.
+///
+/// When/if the `adt_const_params` Rust feature is stabilized, this could be replace by using
+/// `Stability` directly with const generics.
+pub trait StabilityType {
+    /// The `Stability` represented by this type.
+    const VALUE: Stability;
+}
+
+/// `Stability::Local`.
+#[derive(Debug)]
+pub enum LocalStabilityType {}
+/// `Stability::Vintf`.
+#[derive(Debug)]
+pub enum VintfStabilityType {}
+
+impl StabilityType for LocalStabilityType {
+    const VALUE: Stability = Stability::Local;
+}
+
+impl StabilityType for VintfStabilityType {
+    const VALUE: Stability = Stability::Vintf;
+}
+
 /// A local service that can be remotable via Binder.
 ///
 /// An object that implement this interface made be made into a Binder service

libs/binder/rust/src/lib.rs

@@ -128,9 +128,10 @@ pub type Result<T> = std::result::Result<T, Status>;
 /// without AIDL.
 pub mod binder_impl {
     pub use crate::binder::{
-        IBinderInternal, InterfaceClass, Remotable, Stability, ToAsyncInterface, ToSyncInterface,
-        TransactionCode, TransactionFlags, FIRST_CALL_TRANSACTION, FLAG_CLEAR_BUF, FLAG_ONEWAY,
-        FLAG_PRIVATE_LOCAL, LAST_CALL_TRANSACTION,
+        IBinderInternal, InterfaceClass, LocalStabilityType, Remotable, Stability, StabilityType,
+        ToAsyncInterface, ToSyncInterface, TransactionCode, TransactionFlags, VintfStabilityType,
+        FIRST_CALL_TRANSACTION, FLAG_CLEAR_BUF, FLAG_ONEWAY, FLAG_PRIVATE_LOCAL,
+        LAST_CALL_TRANSACTION,
     };
     pub use crate::binder_async::BinderAsyncRuntime;
     pub use crate::error::status_t;

libs/binder/rust/src/parcel/parcelable_holder.rs

@@ -15,6 +15,7 @@
  */
 
 use crate::binder::Stability;
+use crate::binder::StabilityType;
 use crate::error::StatusCode;
 use crate::parcel::{
     BorrowedParcel, Deserialize, Parcel, Parcelable, Serialize, NON_NULL_PARCELABLE_FLAG,
@@ -60,21 +61,25 @@ enum ParcelableHolderData {
 /// `Send` nor `Sync`), mainly because it internally contains
 /// a `Parcel` which in turn is not thread-safe.
 #[derive(Debug)]
-pub struct ParcelableHolder {
+pub struct ParcelableHolder<STABILITY: StabilityType> {
     // This is a `Mutex` because of `get_parcelable`
     // which takes `&self` for consistency with C++.
     // We could make `get_parcelable` take a `&mut self`
     // and get rid of the `Mutex` here for a performance
     // improvement, but then callers would require a mutable
     // `ParcelableHolder` even for that getter method.
     data: Mutex<ParcelableHolderData>,
-    stability: Stability,
+
+    _stability_phantom: std::marker::PhantomData<STABILITY>,
 }
 
-impl ParcelableHolder {
+impl<STABILITY: StabilityType> ParcelableHolder<STABILITY> {
     /// Construct a new `ParcelableHolder` with the given stability.
-    pub fn new(stability: Stability) -> Self {
-        Self { data: Mutex::new(ParcelableHolderData::Empty), stability }
+    pub fn new() -> Self {
+        Self {
+            data: Mutex::new(ParcelableHolderData::Empty),
+            _stability_phantom: Default::default(),
+        }
     }
 
     /// Reset the contents of this `ParcelableHolder`.
@@ -91,7 +96,7 @@ impl ParcelableHolder {
     where
         T: Any + Parcelable + ParcelableMetadata + std::fmt::Debug + Send + Sync,
     {
-        if self.stability > p.get_stability() {
+        if STABILITY::VALUE > p.get_stability() {
             return Err(StatusCode::BAD_VALUE);
         }
 
@@ -157,30 +162,36 @@ impl ParcelableHolder {
 
     /// Return the stability value of this object.
     pub fn get_stability(&self) -> Stability {
-        self.stability
+        STABILITY::VALUE
+    }
+}
+
+impl<STABILITY: StabilityType> Default for ParcelableHolder<STABILITY> {
+    fn default() -> Self {
+        Self::new()
     }
 }
 
-impl Clone for ParcelableHolder {
-    fn clone(&self) -> ParcelableHolder {
+impl<STABILITY: StabilityType> Clone for ParcelableHolder<STABILITY> {
+    fn clone(&self) -> Self {
         ParcelableHolder {
             data: Mutex::new(self.data.lock().unwrap().clone()),
-            stability: self.stability,
+            _stability_phantom: Default::default(),
         }
     }
 }
 
-impl Serialize for ParcelableHolder {
+impl<STABILITY: StabilityType> Serialize for ParcelableHolder<STABILITY> {
     fn serialize(&self, parcel: &mut BorrowedParcel<'_>) -> Result<(), StatusCode> {
         parcel.write(&NON_NULL_PARCELABLE_FLAG)?;
         self.write_to_parcel(parcel)
     }
 }
 
-impl Deserialize for ParcelableHolder {
+impl<STABILITY: StabilityType> Deserialize for ParcelableHolder<STABILITY> {
     type UninitType = Self;
     fn uninit() -> Self::UninitType {
-        Self::new(Default::default())
+        Self::new()
     }
     fn from_init(value: Self) -> Self::UninitType {
         value
@@ -191,16 +202,16 @@ impl Deserialize for ParcelableHolder {
         if status == NULL_PARCELABLE_FLAG {
             Err(StatusCode::UNEXPECTED_NULL)
         } else {
-            let mut parcelable = ParcelableHolder::new(Default::default());
+            let mut parcelable = Self::new();
             parcelable.read_from_parcel(parcel)?;
             Ok(parcelable)
         }
     }
 }
 
-impl Parcelable for ParcelableHolder {
+impl<STABILITY: StabilityType> Parcelable for ParcelableHolder<STABILITY> {
     fn write_to_parcel(&self, parcel: &mut BorrowedParcel<'_>) -> Result<(), StatusCode> {
-        parcel.write(&self.stability)?;
+        parcel.write(&STABILITY::VALUE)?;
 
         let mut data = self.data.lock().unwrap();
         match *data {
@@ -236,7 +247,7 @@ impl Parcelable for ParcelableHolder {
     }
 
     fn read_from_parcel(&mut self, parcel: &BorrowedParcel<'_>) -> Result<(), StatusCode> {
-        if self.stability != parcel.read()? {
+        if self.get_stability() != parcel.read()? {
             return Err(StatusCode::BAD_VALUE);
         }
 

libs/binder/rust/tests/parcel_fuzzer/parcel_fuzzer.rs

@@ -21,7 +21,8 @@ mod read_utils;
 
 use crate::read_utils::READ_FUNCS;
 use binder::binder_impl::{
-    Binder, BorrowedParcel, IBinderInternal, Parcel, Stability, TransactionCode,
+    Binder, BorrowedParcel, IBinderInternal, LocalStabilityType, Parcel, TransactionCode,
+    VintfStabilityType,
 };
 use binder::{
     declare_binder_interface, BinderFeatures, Interface, Parcelable, ParcelableHolder, SpIBinder,
@@ -121,13 +122,15 @@ fn do_read_fuzz(read_operations: Vec<ReadOperation>, data: &[u8]) {
             }
 
             ReadOperation::ReadParcelableHolder { is_vintf } => {
-                let stability = if is_vintf { Stability::Vintf } else { Stability::Local };
-                let mut holder: ParcelableHolder = ParcelableHolder::new(stability);
-                match holder.read_from_parcel(parcel.borrowed_ref()) {
-                    Ok(result) => result,
-                    Err(err) => {
-                        println!("error occurred while reading from parcel: {:?}", err)
-                    }
+                let result = if is_vintf {
+                    ParcelableHolder::<VintfStabilityType>::new()
+                        .read_from_parcel(parcel.borrowed_ref())
+                } else {
+                    ParcelableHolder::<LocalStabilityType>::new()
+                        .read_from_parcel(parcel.borrowed_ref())
+                };
+                if let Err(e) = result {
+                    println!("error occurred while reading from parcel: {e:?}")
                 }
             }