prevent use-after-free in FFI method return

Signed-off-by: Marin Veršić <[email protected]>

data_model/src/predicate.rs

@@ -300,7 +300,7 @@ pub mod string {
                 assert!(Predicate::starts_with("alice@").applies(&id));
                 assert!(Predicate::ends_with("@wonderland").applies(&id));
                 assert!(Predicate::is("alice@wonderland").applies(&id));
-                // Should we also include a check into string
+                // TODO: Should we also include a check into string
                 // predicates? If the internal predicate starts with
                 // whitespace, it can't possibly match any Id, but
                 // there's no way to enforce this at both type leve
@@ -320,10 +320,10 @@ pub mod string {
                     definition_id,
                     account_id,
                 });
-                assert!(Predicate::starts_with("rose#wonderland").applies(&id));
-                assert!(Predicate::ends_with("@alice@wonderland").applies(&id));
-                assert!(Predicate::is("rose#wonderland@alice@wonderland").applies(&id)); // Feels weird
-                assert!(Predicate::contains("wonderland@alice").applies(&id));
+                assert!(Predicate::starts_with("rose#alice").applies(&id));
+                assert!(Predicate::ends_with("#alice@wonderland").applies(&id));
+                assert!(Predicate::is("rose#alice@wonderland").applies(&id));
+                assert!(Predicate::contains("#alice@").applies(&id));
             }
 
             #[test]
@@ -332,7 +332,7 @@ pub mod string {
                 assert!(Predicate::starts_with("rose#").applies(&id));
                 assert!(Predicate::ends_with("#wonderland").applies(&id));
                 assert!(Predicate::is("rose#wonderland").applies(&id));
-                // Should we also include a check into string
+                // TODO: Should we also include a check into string
                 // predicates? If the internal predicate starts with
                 // whitespace, it can't possibly match any Id, but
                 // there's no way to enforce this at both type leve

data_model/src/transaction.rs

@@ -512,6 +512,7 @@ impl IntoIterator for PendingTransactions {
 #[derive(
     Debug, Clone, PartialEq, Eq, Decode, Encode, Deserialize, Serialize, FfiType, IntoSchema,
 )]
+#[local]
 pub enum TransactionValue {
     /// Committed transaction
     Transaction(Box<VersionedSignedTransaction>),

ffi/derive/src/convert.rs

@@ -25,12 +25,17 @@ pub fn derive_ffi_type(input: DeriveInput) -> TokenStream2 {
                 return derive_ffi_type_for_fieldless_enum(&input.ident, &item, &repr);
             }
 
-            derive_ffi_type_for_data_carrying_enum(&input.ident, input.generics, &item)
+            derive_ffi_type_for_data_carrying_enum(
+                &input.ident,
+                &input.attrs,
+                input.generics,
+                &item,
+            )
         }
         syn::Data::Struct(_) => {
             if is_opaque(&input) {
                 return derive_ffi_type_for_opaque_struct(name, input.generics);
-            } else if is_opaque_wrapper(&input) {
+            } else if is_opaque_wrapper(&input.attrs) {
                 return derive_ffi_type_for_extern_struct(name, input.generics);
             }
 
@@ -119,6 +124,9 @@ fn derive_ffi_type_for_opaque_struct(name: &Ident, mut generics: Generics) -> To
         impl<#lifetime, #impl_generics> iroha_ffi::ir::Ir for &#lifetime #name #ty_generics #where_clause {
             type Type = iroha_ffi::ir::Transparent<&#lifetime #name #ty_generics>;
         }
+
+        unsafe impl<#lifetime, #impl_generics> iroha_ffi::repr_c::NonLocal for &#lifetime #name #ty_generics #where_clause {}
+        unsafe impl<#lifetime, #impl_generics> iroha_ffi::repr_c::NonLocal for &#lifetime mut #name #ty_generics #where_clause {}
     }
 }
 
@@ -142,7 +150,6 @@ fn derive_ffi_type_for_extern_struct(name: &Ident, mut generics: Generics) -> To
         impl<#impl_generics> iroha_ffi::ir::Ir for #name #ty_generics #where_clause {
             type Type = iroha_ffi::ir::AsIs<Self>;
         }
-        // TODO: Add lifetime?
         impl<#impl_generics> iroha_ffi::ir::Ir for #ref_name #ty_generics #where_clause {
             type Type = iroha_ffi::ir::AsIs<Self>;
         }
@@ -188,10 +195,12 @@ fn derive_ffi_type_for_fieldless_enum(
 #[allow(clippy::too_many_lines)]
 pub fn derive_ffi_type_for_data_carrying_enum(
     enum_name: &Ident,
+    attrs: &[syn::Attribute],
     mut generics: syn::Generics,
     enum_: &syn::DataEnum,
 ) -> TokenStream2 {
     let (repr_c_enum_name, repr_c_enum) = gen_repr_c_enum(enum_name, &mut generics, enum_);
+    let mut non_local_where_clause = generics.make_where_clause().clone();
     let lifetime: syn::Lifetime = parse_quote!('__iroha_ffi_itm);
 
     let (impl_generics, ty_generics, where_clause) =
@@ -297,13 +306,38 @@ pub fn derive_ffi_type_for_data_carrying_enum(
             )
         };
 
+    let non_locality = if is_non_local(attrs) {
+        enum_
+            .variants
+            .iter()
+            .filter_map(|variant| match &variant.fields {
+                syn::Fields::Unnamed(syn::FieldsUnnamed { unnamed, .. }) if unnamed.len() == 1 => {
+                    Some(&unnamed[0].ty)
+                }
+                syn::Fields::Unnamed(syn::FieldsUnnamed { unnamed, .. }) => {
+                    abort!(unnamed, "Only 1-sized variants are supported")
+                }
+                syn::Fields::Named(syn::FieldsNamed { named, .. }) => {
+                    abort!(named, "Named variants are not supported")
+                }
+                syn::Fields::Unit => None,
+            })
+            .map(|ty| parse_quote! {<#ty as iroha_ffi::ir::Ir>::Type: iroha_ffi::repr_c::NonLocal})
+            .for_each(|predicate| non_local_where_clause.predicates.push(predicate));
+
+        quote! {unsafe impl<#impl_generics> iroha_ffi::repr_c::NonLocal for #enum_name #ty_generics #non_local_where_clause {}}
+    } else {
+        quote! {}
+    };
+
     quote! {
         #repr_c_enum
+        #non_locality
 
         // TODO: Enum can be transmutable if all variants are transmutable and the enum is `repr(C)`
         impl<#impl_generics> iroha_ffi::repr_c::NonTransmute for #enum_name #ty_generics #where_clause where Self: Clone {}
 
-        // NOTE: Data-carrying enum cannot implement `ReprC` unless it is robust
+        // NOTE: Data-carrying enum cannot implement `ReprC` unless it is robust `repr(C)`
         impl<#impl_generics> iroha_ffi::ir::Ir for #enum_name #ty_generics #where_clause {
             type Type = Self;
         }
@@ -492,16 +526,21 @@ fn gen_repr_c_enum_payload_name(enum_name: &Ident) -> Ident {
     )
 }
 
-fn is_opaque_wrapper(input: &DeriveInput) -> bool {
+fn is_opaque_wrapper(attrs: &[syn::Attribute]) -> bool {
     let opaque_attr = parse_quote! {#[opaque_wrapper]};
-    input.attrs.iter().any(|a| *a == opaque_attr)
+    attrs.iter().any(|a| *a == opaque_attr)
 }
 
 fn is_transparent(input: &DeriveInput) -> bool {
     let repr = &find_attr(&input.attrs, "repr");
     is_repr_attr(repr, "transparent")
 }
 
+fn is_non_local(attrs: &[syn::Attribute]) -> bool {
+    let local_attr = parse_quote! {#[local]};
+    attrs.iter().all(|a| *a != local_attr)
+}
+
 fn is_fieldless_enum(item: &syn::DataEnum) -> bool {
     !item
         .variants

ffi/derive/src/lib.rs

@@ -69,7 +69,10 @@ pub fn ffi(input: TokenStream) -> TokenStream {
 }
 
 /// Derive implementations of traits required to convert to and from an FFI-compatible type
-#[proc_macro_derive(FfiType, attributes(opaque_wrapper))]
+// TODO: `local` is a temporary workaround for https://github.com/rust-lang/rust/issues/48214
+// because some derived types cannot derive `NonLocal` othwerise
+// TODO: prefix derive macro helper attributes with `ffi_type(_)`
+#[proc_macro_derive(FfiType, attributes(opaque_wrapper, local))]
 #[proc_macro_error::proc_macro_error]
 pub fn ffi_type_derive(input: TokenStream) -> TokenStream {
     let item = parse_macro_input!(input as syn::DeriveInput);

ffi/derive/tests/ui_fail/nested_owned_structure.rs

@@ -0,0 +1,21 @@
+use std::mem::MaybeUninit;
+
+use iroha_ffi::{ffi_export, slice::OutBoxedSlice, FfiType};
+
+type Nested = Vec<Vec<FfiStruct>>;
+
+/// Ffi structure
+#[derive(Clone, FfiType)]
+pub struct FfiStruct;
+
+/// Return nested structure
+#[ffi_export]
+pub fn return_nested() -> Nested {
+    vec![vec![FfiStruct, FfiStruct], vec![FfiStruct, FfiStruct]]
+}
+
+fn main() {
+    let mut params_len = MaybeUninit::uninit();
+    let nested = OutBoxedSlice::from_uninit_slice(None, &mut params_len);
+    unsafe { __return_nested(nested) };
+}

ffi/derive/tests/ui_fail/nested_owned_structure.stderr

@@ -0,0 +1,18 @@
+error[E0277]: the trait bound `IrVec<FfiStruct, Opaque<FfiStruct>>: NonLocal` is not satisfied
+  --> tests/ui_fail/nested_owned_structure.rs:12:1
+   |
+12 | #[ffi_export]
+   | ^^^^^^^^^^^^^ the trait `NonLocal` is not implemented for `IrVec<FfiStruct, Opaque<FfiStruct>>`
+   |
+   = help: the following other types implement trait `NonLocal`:
+             &'__iroha_ffi_itm FfiStruct
+             &'__iroha_ffi_itm mut FfiStruct
+             &T
+             &mut T
+             (A, B)
+             (A, B, C)
+             (A, B, C, D)
+             (A, B, C, D, E)
+           and 17 others
+   = note: required because of the requirements on the impl of `COutPtr` for `IrVec<Vec<FfiStruct>, IrVec<FfiStruct, Opaque<FfiStruct>>>`
+   = note: this error originates in the attribute macro `ffi_export` (in Nightly builds, run with -Z macro-backtrace for more info)

ffi/src/lib.rs

@@ -9,7 +9,7 @@ extern crate alloc;
 
 use ir::{Ir, IrTypeOf};
 pub use iroha_ffi_derive::*;
-use repr_c::{COutPtr, CType, CTypeConvert, NonTransmute};
+use repr_c::{COutPtr, CType, CTypeConvert, NonLocal, NonTransmute};
 
 pub mod handle;
 pub mod ir;
@@ -218,6 +218,13 @@ impl<T: ReprC, const N: usize> OutPtrOf<[T; N]> for *mut T {
         Ok(())
     }
 }
+
+// SAFETY: &T doesn't use store if T doesn't
+unsafe impl<T: NonLocal> NonLocal for &T {}
+// SAFETY: &T doesn't use store if T doesn't
+// NOTE: `&mut T` should never use store
+unsafe impl<T: NonLocal> NonLocal for &mut T {}
+
 macro_rules! impl_tuple {
     ( ($( $ty:ident ),+ $(,)?) -> $ffi_ty:ident ) => {
         /// FFI-compatible tuple with n elements
@@ -235,7 +242,7 @@ macro_rules! impl_tuple {
 
         // SAFETY: Implementing type is robust with a defined C ABI
         unsafe impl<$($ty: ReprC),+> ReprC for $ffi_ty<$($ty),+> {}
-        impl<$($ty: Clone),+> NonTransmute for ($($ty,)+) {}
+        impl<$($ty: Clone),+> NonTransmute for ($($ty,)+) where Self: CType {}
 
         impl<$($ty),+> $crate::ir::Ir for ($($ty,)+) {
             type Type = Self;
@@ -269,6 +276,9 @@ macro_rules! impl_tuple {
         impl<$($ty),+> COutPtr for ($($ty,)+) where Self: CType {
             type OutPtr = *mut Self::ReprC;
         }
+
+        // SAFETY: Tuple doesn't use store if it's inner types don't use it
+        unsafe impl<$($ty: NonLocal),+> NonLocal for ($($ty,)+) where Self: CType {}
     };
 
     // NOTE: This is a trick to index tuples

ffi/src/option.rs

@@ -2,7 +2,7 @@
 
 use crate::{
     ir::{Ir, IrTypeOf},
-    repr_c::{COutPtr, CType, CTypeConvert},
+    repr_c::{COutPtr, CType, CTypeConvert, NonLocal},
     FfiConvert, FfiOutPtr, FfiType, ReprC, Result,
 };
 
@@ -54,6 +54,7 @@ impl<T: Niche> CType for IrOption<T, T::ReprC> {
     type ReprC = T::ReprC;
 }
 // TODO: Hopefully, compiler will elide checks for Option<&T>, Option<&mut T>, Option<*const T>
+// if not U parameter can be used to distinguish set them apart from other type conversions
 impl<'itm, T: FfiConvert<'itm, U> + Niche<ReprC = U>, U: ReprC> CTypeConvert<'itm, U>
     for IrOption<T, U>
 where
@@ -82,3 +83,6 @@ where
 impl<T: FfiOutPtr + Niche> COutPtr for IrOption<T, T::ReprC> {
     type OutPtr = T::OutPtr;
 }
+
+// SAFETY: Option<T> with a niche doesn't use store if it's inner types don't use it
+unsafe impl<T: Niche + Ir> NonLocal for IrOption<T, T::ReprC> where T::Type: NonLocal {}

ffi/src/primitives.rs

@@ -36,12 +36,6 @@ macro_rules! wasm_repr_impls {
             const NICHE_VALUE: $dst = <$src>::MAX as $dst + 1;
         }
 
-        impl From<$src> for NonWasmIntPrimitive<$src> {
-            fn from(source: $src) -> Self {
-                Self(source)
-            }
-        }
-
         impl IrTypeOf<$src> for NonWasmIntPrimitive<$src> {
             fn into_ir(source: $src) -> Self {
                 Self(source)
@@ -102,19 +96,19 @@ macro_rules! wasm_repr_impls {
             type ReprC = *mut $src;
         }
         impl CTypeConvert<'_, *mut $src> for $crate::ir::IrBox<$src, NonWasmIntPrimitive<$src>> {
-            type RustStore = Box<$src>;
+            type RustStore = $src;
             type FfiStore = ();
 
             fn into_repr_c(self, store: &mut Self::RustStore) -> *mut $src {
-                *store = unsafe {Box::from_raw(Box::into_raw(self.0).cast())};
-                &mut **store
+                *store = *self.0;
+                store
             }
             unsafe fn try_from_repr_c(source: *mut $src, _: &mut ()) -> $crate::Result<Self> {
                 if source.is_null() {
                     return Err(FfiReturn::ArgIsNull);
                 }
 
-                Ok(Self(Box::new(*source.cast())))
+                Ok(Self(Box::new(source.read())))
             }
         }
 
@@ -185,7 +179,7 @@ macro_rules! wasm_repr_impls {
             type RustStore = ();
             type FfiStore = ();
 
-            fn into_repr_c(self, store: &mut Self::RustStore) -> [$src; N] {
+            fn into_repr_c(self, _: &mut ()) -> [$src; N] {
                 self.0
             }
             unsafe fn try_from_repr_c(source: [$src; N], _: &mut ()) -> $crate::Result<Self> {
@@ -223,7 +217,10 @@ macro_rules! wasm_repr_impls {
         }
         impl<const N: usize> COutPtr for $crate::ir::IrArray<$src, NonWasmIntPrimitive<$src>, N> {
             type OutPtr = *mut [$src; N];
-        })+
+        }
+
+        // SAFETY: Conversions of non wasm primitives don't use store
+        unsafe impl $crate::NonLocal for NonWasmIntPrimitive<$src> {})+
     };
 }