From 7adc456db416c029b09c0e7c5eb6bce46536a192 Mon Sep 17 00:00:00 2001 From: Andrei Homescu Date: Thu, 14 Oct 2021 02:00:30 +0000 Subject: binder_rs: Make ParcelableHolder thread-safe Replace a few non-thread-safe types and traits in ParcelableHolder with their thread-safe equivalents, i.e., RefCell => Mutex, Rc => Arc, Downcast => DowncastSync. Test: atest aidl_integration_test Change-Id: If0ba81232b482e6427ec3e62d2b4474615a69147 --- libs/binder/rust/src/parcel/parcelable_holder.rs | 70 +++++++++++++----------- 1 file changed, 37 insertions(+), 33 deletions(-) diff --git a/libs/binder/rust/src/parcel/parcelable_holder.rs b/libs/binder/rust/src/parcel/parcelable_holder.rs index 3e75d1b11d..bccfd2d25e 100644 --- a/libs/binder/rust/src/parcel/parcelable_holder.rs +++ b/libs/binder/rust/src/parcel/parcelable_holder.rs @@ -16,13 +16,12 @@ use crate::binder::Stability; use crate::error::{Result, StatusCode}; -use crate::parcel::{Parcel, Parcelable}; +use crate::parcel::{OwnedParcel, Parcel, Parcelable}; use crate::{impl_deserialize_for_parcelable, impl_serialize_for_parcelable}; -use downcast_rs::{impl_downcast, Downcast}; +use downcast_rs::{impl_downcast, DowncastSync}; use std::any::Any; -use std::cell::RefCell; -use std::rc::Rc; +use std::sync::{Arc, Mutex}; /// Metadata that `ParcelableHolder` needs for all parcelables. /// @@ -40,18 +39,18 @@ pub trait ParcelableMetadata { } } -trait AnyParcelable: Downcast + Parcelable + std::fmt::Debug {} -impl_downcast!(AnyParcelable); -impl AnyParcelable for T where T: Downcast + Parcelable + std::fmt::Debug {} +trait AnyParcelable: DowncastSync + Parcelable + std::fmt::Debug {} +impl_downcast!(sync AnyParcelable); +impl AnyParcelable for T where T: DowncastSync + Parcelable + std::fmt::Debug {} #[derive(Debug, Clone)] enum ParcelableHolderData { Empty, Parcelable { - parcelable: Rc, + parcelable: Arc, name: String, }, - Parcel(Parcel), + Parcel(OwnedParcel), } impl Default for ParcelableHolderData { @@ -67,15 +66,15 @@ impl Default for ParcelableHolderData { /// `ParcelableHolder` is currently not thread-safe (neither /// `Send` nor `Sync`), mainly because it internally contains /// a `Parcel` which in turn is not thread-safe. -#[derive(Debug, Default, Clone)] +#[derive(Debug, Default)] pub struct ParcelableHolder { - // This is a `RefCell` because of `get_parcelable` + // 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 `RefCell` here for a performance + // 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: RefCell, + data: Mutex, stability: Stability, } @@ -83,7 +82,7 @@ impl ParcelableHolder { /// Construct a new `ParcelableHolder` with the given stability. pub fn new(stability: Stability) -> Self { Self { - data: RefCell::new(ParcelableHolderData::Empty), + data: Mutex::new(ParcelableHolderData::Empty), stability, } } @@ -93,20 +92,20 @@ impl ParcelableHolder { /// Note that this method does not reset the stability, /// only the contents. pub fn reset(&mut self) { - *self.data.get_mut() = ParcelableHolderData::Empty; + *self.data.get_mut().unwrap() = ParcelableHolderData::Empty; // We could also clear stability here, but C++ doesn't } /// Set the parcelable contained in this `ParcelableHolder`. - pub fn set_parcelable(&mut self, p: Rc) -> Result<()> + pub fn set_parcelable(&mut self, p: Arc) -> Result<()> where - T: Any + Parcelable + ParcelableMetadata + std::fmt::Debug, + T: Any + Parcelable + ParcelableMetadata + std::fmt::Debug + Send + Sync, { if self.stability > p.get_stability() { return Err(StatusCode::BAD_VALUE); } - *self.data.get_mut() = ParcelableHolderData::Parcelable { + *self.data.get_mut().unwrap() = ParcelableHolderData::Parcelable { parcelable: p, name: T::get_descriptor().into(), }; @@ -127,12 +126,12 @@ impl ParcelableHolder { /// * `Ok(None)` if the holder is empty or the descriptor does not match /// * `Ok(Some(_))` if the object holds a parcelable of type `T` /// with the correct descriptor - pub fn get_parcelable(&self) -> Result>> + pub fn get_parcelable(&self) -> Result>> where - T: Any + Parcelable + ParcelableMetadata + Default + std::fmt::Debug, + T: Any + Parcelable + ParcelableMetadata + Default + std::fmt::Debug + Send + Sync, { let parcelable_desc = T::get_descriptor(); - let mut data = self.data.borrow_mut(); + let mut data = self.data.lock().unwrap(); match *data { ParcelableHolderData::Empty => Ok(None), ParcelableHolderData::Parcelable { @@ -143,12 +142,13 @@ impl ParcelableHolder { return Err(StatusCode::BAD_VALUE); } - match Rc::clone(parcelable).downcast_rc::() { + match Arc::clone(parcelable).downcast_arc::() { Err(_) => Err(StatusCode::BAD_VALUE), Ok(x) => Ok(Some(x)), } } - ParcelableHolderData::Parcel(ref parcel) => { + ParcelableHolderData::Parcel(ref mut parcel) => { + let parcel = parcel.borrowed(); unsafe { // Safety: 0 should always be a valid position. parcel.set_data_position(0)?; @@ -160,10 +160,10 @@ impl ParcelableHolder { } let mut parcelable = T::default(); - parcelable.read_from_parcel(parcel)?; + parcelable.read_from_parcel(&parcel)?; - let parcelable = Rc::new(parcelable); - let result = Rc::clone(&parcelable); + let parcelable = Arc::new(parcelable); + let result = Arc::clone(&parcelable); *data = ParcelableHolderData::Parcelable { parcelable, name }; Ok(Some(result)) @@ -184,7 +184,8 @@ impl Parcelable for ParcelableHolder { fn write_to_parcel(&self, parcel: &mut Parcel) -> Result<()> { parcel.write(&self.stability)?; - match *self.data.borrow() { + let mut data = self.data.lock().unwrap(); + match *data { ParcelableHolderData::Empty => parcel.write(&0i32), ParcelableHolderData::Parcelable { ref parcelable, @@ -212,9 +213,10 @@ impl Parcelable for ParcelableHolder { Ok(()) } - ParcelableHolderData::Parcel(ref p) => { + ParcelableHolderData::Parcel(ref mut p) => { + let p = p.borrowed(); parcel.write(&p.get_data_size())?; - parcel.append_all_from(p) + parcel.append_all_from(&p) } } } @@ -229,7 +231,7 @@ impl Parcelable for ParcelableHolder { return Err(StatusCode::BAD_VALUE); } if data_size == 0 { - *self.data.get_mut() = ParcelableHolderData::Empty; + *self.data.get_mut().unwrap() = ParcelableHolderData::Empty; return Ok(()); } @@ -240,9 +242,11 @@ impl Parcelable for ParcelableHolder { .checked_add(data_size) .ok_or(StatusCode::BAD_VALUE)?; - let mut new_parcel = Parcel::new(); - new_parcel.append_from(parcel, data_start, data_size)?; - *self.data.get_mut() = ParcelableHolderData::Parcel(new_parcel); + let mut new_parcel = OwnedParcel::new(); + new_parcel + .borrowed() + .append_from(parcel, data_start, data_size)?; + *self.data.get_mut().unwrap() = ParcelableHolderData::Parcel(new_parcel); unsafe { // Safety: `append_from` checks if `data_size` overflows -- cgit v1.2.3