address review, refactor ffi logic

Author: nia-e

src/shims/native_lib/ffi.rs

@@ -3,72 +3,65 @@ use libffi::middle::{Arg as ArgPtr, Cif, Type as FfiType};
 
 /// Perform the actual FFI call.
 ///
-/// SAFETY: The `FfiArg`s passed must have been correctly instantiated (i.e. their
-/// type layout must match the data they point to), and the safety invariants of
-/// the foreign function being called must be upheld (if any).
-pub unsafe fn call<'a, R: libffi::high::CType>(fun: CodePtr, args: Vec<FfiArg<'a>>) -> R {
+/// SAFETY: The safety invariants of the foreign function being called must be
+/// upheld (if any).
+pub unsafe fn call<R: libffi::high::CType>(fun: CodePtr, args: &[OwnedArg]) -> R {
     let mut arg_tys = vec![];
     let mut arg_ptrs = vec![];
     for arg in args {
-        arg_tys.push(arg.ty);
-        arg_ptrs.push(arg.ptr)
+        arg_tys.push(arg.ty());
+        arg_ptrs.push(arg.ptr())
     }
     let cif = Cif::new(arg_tys, R::reify().into_middle());
+    // SAFETY: Caller upholds that the function is safe to call, and since we
+    // were passed a slice reference we know the `OwnedArg`s won't have been
+    // by this point.
     unsafe { cif.call(fun, &arg_ptrs) }
 }
 
-/// A wrapper type for `libffi::middle::Type` which also holds a pointer to the data.
-pub struct FfiArg<'a> {
-    /// The type layout information for the pointed-to data.
-    ty: FfiType,
-    /// A pointer to the data described in `ty`.
-    ptr: ArgPtr,
-    /// Lifetime of the actual pointed-to data.
-    _p: std::marker::PhantomData<&'a [u8]>,
-}
-
-impl<'a> FfiArg<'a> {
-    fn new(ty: FfiType, ptr: ArgPtr) -> Self {
-        Self { ty, ptr, _p: std::marker::PhantomData }
-    }
-}
-
-/// An owning form of `FfiArg`.
-/// We introduce this enum instead of just calling `Arg::new` and storing a list of
-/// `libffi::middle::Arg` directly, because the `libffi::middle::Arg` just wraps a reference to
-/// the value it represents and we need to store a copy of the value, and pass a reference to
-/// this copy to C instead.
+/// An argument for an FFI call.
 #[derive(Debug, Clone)]
-pub enum CArg {
+pub enum OwnedArg {
     /// Primitive type.
-    Primitive(CPrimitive),
-    /// Struct with its computed type layout and bytes.
-    Struct(FfiType, Box<[u8]>),
+    Primitive(ScalarArg),
+    /// ADT with its computed type layout and bytes.
+    Adt(FfiType, Box<[u8]>),
 }
 
-impl CArg {
-    /// Convert a `CArg` to the required FFI argument type.
-    pub fn arg_downcast<'a>(&'a self) -> FfiArg<'a> {
+impl OwnedArg {
+    /// Gets the libffi type descriptor for this argument.
+    fn ty(&self) -> FfiType {
         match self {
-            CArg::Primitive(cprim) => cprim.arg_downcast(),
+            OwnedArg::Primitive(scalar_arg) => scalar_arg.ty(),
+            OwnedArg::Adt(ty, _) => ty.clone(),
+        }
+    }
+
+    /// Instantiates a libffi argument pointer pointing to this argument's bytes.
+    /// NB: Since `libffi::middle::Arg` ignores the lifetime of the reference
+    /// it's derived from, it is up to the caller to ensure the `OwnedArg` is
+    /// not dropped before unsafely calling `libffi::middle::Cif::call()`!
+    fn ptr(&self) -> ArgPtr {
+        match self {
+            OwnedArg::Primitive(scalar_arg) => scalar_arg.ptr(),
             // FIXME: Using `&items[0]` to reference the whole array is definitely
             // unsound under SB, but we're waiting on
             // https://github.com/libffi-rs/libffi-rs/commit/112a37b3b6ffb35bd75241fbcc580de40ba74a73
             // to land in a release so that we don't need to do this.
-            CArg::Struct(cstruct, items) => FfiArg::new(cstruct.clone(), ArgPtr::new(&items[0])),
+            OwnedArg::Adt(_, items) => ArgPtr::new(&items[0]),
         }
     }
 }
 
-impl From<CPrimitive> for CArg {
-    fn from(prim: CPrimitive) -> Self {
+impl From<ScalarArg> for OwnedArg {
+    fn from(prim: ScalarArg) -> Self {
         Self::Primitive(prim)
     }
 }
 
 #[derive(Debug, Clone)]
-/// Enum of supported primitive arguments to external C functions.
-pub enum CPrimitive {
+/// Enum of supported scalar arguments to external C functions.
+pub enum ScalarArg {
     /// 8-bit signed integer.
     Int8(i8),
     /// 16-bit signed integer.
@@ -93,21 +86,38 @@ pub enum CPrimitive {
     RawPtr(*mut std::ffi::c_void),
 }
 
-impl CPrimitive {
-    /// Convert a primitive to the required FFI argument type.
-    fn arg_downcast<'a>(&'a self) -> FfiArg<'a> {
+impl ScalarArg {
+    /// See `OwnedArg::ty()`.
+    fn ty(&self) -> FfiType {
+        match self {
+            ScalarArg::Int8(_) => FfiType::i8(),
+            ScalarArg::Int16(_) => FfiType::i16(),
+            ScalarArg::Int32(_) => FfiType::i32(),
+            ScalarArg::Int64(_) => FfiType::i64(),
+            ScalarArg::ISize(_) => FfiType::isize(),
+            ScalarArg::UInt8(_) => FfiType::u8(),
+            ScalarArg::UInt16(_) => FfiType::u16(),
+            ScalarArg::UInt32(_) => FfiType::u32(),
+            ScalarArg::UInt64(_) => FfiType::u64(),
+            ScalarArg::USize(_) => FfiType::usize(),
+            ScalarArg::RawPtr(_) => FfiType::pointer(),
+        }
+    }
+
+    /// See `OwnedArg::ptr()`.
+    fn ptr(&self) -> ArgPtr {
         match self {
-            CPrimitive::Int8(i) => FfiArg::new(FfiType::i8(), ArgPtr::new(i)),
-            CPrimitive::Int16(i) => FfiArg::new(FfiType::i16(), ArgPtr::new(i)),
-            CPrimitive::Int32(i) => FfiArg::new(FfiType::i32(), ArgPtr::new(i)),
-            CPrimitive::Int64(i) => FfiArg::new(FfiType::i64(), ArgPtr::new(i)),
-            CPrimitive::ISize(i) => FfiArg::new(FfiType::isize(), ArgPtr::new(i)),
-            CPrimitive::UInt8(i) => FfiArg::new(FfiType::u8(), ArgPtr::new(i)),
-            CPrimitive::UInt16(i) => FfiArg::new(FfiType::u16(), ArgPtr::new(i)),
-            CPrimitive::UInt32(i) => FfiArg::new(FfiType::u32(), ArgPtr::new(i)),
-            CPrimitive::UInt64(i) => FfiArg::new(FfiType::u64(), ArgPtr::new(i)),
-            CPrimitive::USize(i) => FfiArg::new(FfiType::usize(), ArgPtr::new(i)),
-            CPrimitive::RawPtr(i) => FfiArg::new(FfiType::pointer(), ArgPtr::new(i)),
+            ScalarArg::Int8(i) => ArgPtr::new(i),
+            ScalarArg::Int16(i) => ArgPtr::new(i),
+            ScalarArg::Int32(i) => ArgPtr::new(i),
+            ScalarArg::Int64(i) => ArgPtr::new(i),
+            ScalarArg::ISize(i) => ArgPtr::new(i),
+            ScalarArg::UInt8(i) => ArgPtr::new(i),
+            ScalarArg::UInt16(i) => ArgPtr::new(i),
+            ScalarArg::UInt32(i) => ArgPtr::new(i),
+            ScalarArg::UInt64(i) => ArgPtr::new(i),
+            ScalarArg::USize(i) => ArgPtr::new(i),
+            ScalarArg::RawPtr(i) => ArgPtr::new(i),
         }
     }
 }

src/shims/native_lib/mod.rs

@@ -21,7 +21,7 @@ mod ffi;
 )]
 pub mod trace;
 
-use self::ffi::{CArg, CPrimitive, FfiArg};
+use self::ffi::{OwnedArg, ScalarArg};
 use crate::*;
 
 /// The final results of an FFI trace, containing every relevant event detected
@@ -72,12 +72,12 @@ impl AccessRange {
 impl<'tcx> EvalContextExtPriv<'tcx> for crate::MiriInterpCx<'tcx> {}
 trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
     /// Call native host function and return the output as an immediate.
-    fn call_native_with_args<'a>(
+    fn call_native_with_args(
         &mut self,
         link_name: Symbol,
         dest: &MPlaceTy<'tcx>,
         ptr: CodePtr,
-        libffi_args: Vec<FfiArg<'a>>,
+        libffi_args: &[OwnedArg],
     ) -> InterpResult<'tcx, (crate::ImmTy<'tcx>, Option<MemEvents>)> {
         let this = self.eval_context_mut();
         #[cfg(target_os = "linux")]
@@ -275,95 +275,90 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
     }
 
     /// Extract the value from the result of reading an operand from the machine
-    /// and convert it to a `CArg`.
-    fn op_to_ffi_arg(&self, v: &OpTy<'tcx>, tracing: bool) -> InterpResult<'tcx, CArg> {
+    /// and convert it to a `OwnedArg`.
+    fn op_to_ffi_arg(&self, v: &OpTy<'tcx>, tracing: bool) -> InterpResult<'tcx, OwnedArg> {
         let this = self.eval_context_ref();
         let scalar = |v| interp_ok(this.read_immediate(v)?.to_scalar());
         interp_ok(match v.layout.ty.kind() {
             // If the primitive provided can be converted to a type matching the type pattern
-            // then create a `CArg` of this primitive value with the corresponding `CArg` constructor.
+            // then create a `OwnedArg` of this primitive value with the corresponding `OwnedArg` constructor.
             // the ints
-            ty::Int(IntTy::I8) => CPrimitive::Int8(scalar(v)?.to_i8()?).into(),
-            ty::Int(IntTy::I16) => CPrimitive::Int16(scalar(v)?.to_i16()?).into(),
-            ty::Int(IntTy::I32) => CPrimitive::Int32(scalar(v)?.to_i32()?).into(),
-            ty::Int(IntTy::I64) => CPrimitive::Int64(scalar(v)?.to_i64()?).into(),
+            ty::Int(IntTy::I8) => ScalarArg::Int8(scalar(v)?.to_i8()?).into(),
+            ty::Int(IntTy::I16) => ScalarArg::Int16(scalar(v)?.to_i16()?).into(),
+            ty::Int(IntTy::I32) => ScalarArg::Int32(scalar(v)?.to_i32()?).into(),
+            ty::Int(IntTy::I64) => ScalarArg::Int64(scalar(v)?.to_i64()?).into(),
             ty::Int(IntTy::Isize) =>
-                CPrimitive::ISize(scalar(v)?.to_target_isize(this)?.try_into().unwrap()).into(),
+                ScalarArg::ISize(scalar(v)?.to_target_isize(this)?.try_into().unwrap()).into(),
             // the uints
-            ty::Uint(UintTy::U8) => CPrimitive::UInt8(scalar(v)?.to_u8()?).into(),
-            ty::Uint(UintTy::U16) => CPrimitive::UInt16(scalar(v)?.to_u16()?).into(),
-            ty::Uint(UintTy::U32) => CPrimitive::UInt32(scalar(v)?.to_u32()?).into(),
-            ty::Uint(UintTy::U64) => CPrimitive::UInt64(scalar(v)?.to_u64()?).into(),
+            ty::Uint(UintTy::U8) => ScalarArg::UInt8(scalar(v)?.to_u8()?).into(),
+            ty::Uint(UintTy::U16) => ScalarArg::UInt16(scalar(v)?.to_u16()?).into(),
+            ty::Uint(UintTy::U32) => ScalarArg::UInt32(scalar(v)?.to_u32()?).into(),
+            ty::Uint(UintTy::U64) => ScalarArg::UInt64(scalar(v)?.to_u64()?).into(),
             ty::Uint(UintTy::Usize) =>
-                CPrimitive::USize(scalar(v)?.to_target_usize(this)?.try_into().unwrap()).into(),
+                ScalarArg::USize(scalar(v)?.to_target_usize(this)?.try_into().unwrap()).into(),
             ty::RawPtr(..) => {
                 let ptr = scalar(v)?.to_pointer(this)?;
-                // Pointer without provenance may not access any memory anyway, skip.
-                if let Some(prov) = ptr.provenance {
-                    // The first time this happens, print a warning.
-                    if !this.machine.native_call_mem_warned.replace(true) {
-                        // Newly set, so first time we get here.
-                        this.emit_diagnostic(NonHaltingDiagnostic::NativeCallSharedMem { tracing });
-                    }
-
-                    this.expose_provenance(prov)?;
-                };
+                this.expose_and_warn(ptr.provenance, tracing)?;
 
                 // This relies on the `expose_provenance` in the `visit_reachable_allocs` callback
                 // below to expose the actual interpreter-level allocation.
-                CPrimitive::RawPtr(std::ptr::with_exposed_provenance_mut(ptr.addr().bytes_usize()))
+                ScalarArg::RawPtr(std::ptr::with_exposed_provenance_mut(ptr.addr().bytes_usize()))
                     .into()
             }
             // For ADTs, create an FfiType from their fields.
             ty::Adt(adt_def, args) => {
-                let strukt = this.adt_to_ffitype(v.layout.ty, *adt_def, args)?;
+                let adt = this.adt_to_ffitype(v.layout.ty, *adt_def, args)?;
 
                 // Copy the raw bytes backing this arg.
                 let bytes = match v.as_mplace_or_imm() {
                     either::Either::Left(mplace) => {
-                        // We do all of this to grab the bytes without actually
-                        // stripping provenance from them, since it'll later be
-                        // exposed recursively.
-                        let ptr = mplace.ptr();
-                        // Make sure the provenance of this allocation is exposed;
-                        // there must be one for this mplace to be valid at all.
-                        // The interpreter-level allocation itself is exposed in
-                        // visit_reachable_allocs.
-                        this.expose_provenance(ptr.provenance.unwrap())?;
-                        // Then get the actual bytes.
+                        // Get the alloc id corresponding to this mplace, alongside
+                        // a pointer that's offset to point to this particular
+                        // mplace (not one at the base addr of the allocation).
+                        let mplace_ptr = mplace.ptr();
+                        let sz = mplace.layout.size.bytes_usize();
                         let id = this
                             .alloc_id_from_addr(
-                                ptr.addr().bytes(),
-                                mplace.layout.size.bytes_usize().try_into().unwrap(),
-                                /* only_exposed_allocations */ true,
+                                mplace_ptr.addr().bytes(),
+                                sz.try_into().unwrap(),
+                                /* only_exposed_allocations */ false,
                             )
                             .unwrap();
-                        let ptr_raw = this.get_alloc_bytes_unchecked_raw(id)?;
-                        // SAFETY: We know for sure that at ptr_raw the next layout.size bytes
-                        // are part of this allocation and initialised. They might be marked as
-                        // uninit in Miri, but all bytes returned by `MiriAllocBytes` are
+                        // Expose all provenances in the allocation within the byte
+                        // range of the struct, if any.
+                        let alloc = this.get_alloc_raw(id)?;
+                        let alloc_ptr = this.get_alloc_bytes_unchecked_raw(id)?;
+                        let start_addr =
+                            mplace_ptr.addr().bytes_usize().strict_sub(alloc_ptr.addr());
+                        for byte in start_addr..start_addr.strict_add(sz) {
+                            if let Some(prov) = alloc.provenance().get(Size::from_bytes(byte), this)
+                            {
+                                this.expose_provenance(prov)?;
+                            }
+                        }
+                        // SAFETY: We know for sure that at mplace_ptr.addr() the next layout.size
+                        // bytes are part of this allocation and initialised. They might be marked
+                        // as uninit in Miri, but all bytes returned by `MiriAllocBytes` are
                         // initialised.
                         unsafe {
-                            std::slice::from_raw_parts(ptr_raw, mplace.layout.size.bytes_usize())
-                                .to_vec()
-                                .into_boxed_slice()
+                            std::slice::from_raw_parts(
+                                alloc_ptr.with_addr(mplace_ptr.addr().bytes_usize()),
+                                mplace.layout.size.bytes_usize(),
+                            )
+                            .to_vec()
+                            .into_boxed_slice()
                         }
                     }
-                    either::Either::Right(imm) => {
-                        // For immediates, we know the backing scalar is going to be 128 bits,
-                        // so we can just copy that.
-                        // TODO: is it possible for this to be a scalar pair?
-                        let scalar = imm.to_scalar();
-                        if scalar.size().bytes() > 0 {
-                            let bits = scalar.to_bits(scalar.size())?;
-                            bits.to_ne_bytes().to_vec().into_boxed_slice()
-                        } else {
-                            throw_ub_format!("attempting to pass a ZST over FFI: {}", imm.layout.ty)
-                        }
+                    either::Either::Right(_) => {
+                        // TODO: support this!
+                        throw_unsup_format!(
+                            "Immediate structs can't be passed over FFI: {}",
+                            v.layout.ty
+                        )
                     }
                 };
 
-                ffi::CArg::Struct(strukt, bytes)
+                ffi::OwnedArg::Adt(adt, bytes)
             }
             _ => throw_unsup_format!("unsupported argument type for native call: {}", v.layout.ty),
         })
@@ -376,14 +371,15 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
         adt_def: ty::AdtDef<'tcx>,
         args: &'tcx ty::List<ty::GenericArg<'tcx>>,
     ) -> InterpResult<'tcx, FfiType> {
-        // TODO: is this correct? Maybe `repr(transparent)` when the inner field
-        // is itself `repr(c)` is ok?
+        // TODO: Certain non-C reprs should be okay also.
         if !adt_def.repr().c() {
-            throw_ub_format!("passing a non-#[repr(C)] struct over FFI: {orig_ty}")
+            throw_unsup_format!("passing a non-#[repr(C)] struct over FFI: {orig_ty}")
         }
         // TODO: unions, etc.
         if !adt_def.is_struct() {
-            throw_unsup_format!("unsupported argument type for native call: {orig_ty} is an enum or union");
+            throw_unsup_format!(
+                "unsupported argument type for native call: {orig_ty} is an enum or union"
+            );
         }
 
         let this = self.eval_context_ref();
@@ -414,6 +410,20 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
             _ => throw_unsup_format!("unsupported argument type for native call: {}", ty),
         })
     }
+
+    fn expose_and_warn(&self, prov: Option<Provenance>, tracing: bool) -> InterpResult<'tcx> {
+        let this = self.eval_context_ref();
+        if let Some(prov) = prov {
+            // The first time this happens, print a warning.
+            if !this.machine.native_call_mem_warned.replace(true) {
+                // Newly set, so first time we get here.
+                this.emit_diagnostic(NonHaltingDiagnostic::NativeCallSharedMem { tracing });
+            }
+
+            this.expose_provenance(prov)?;
+        };
+        interp_ok(())
+    }
 }
 
 impl<'tcx> EvalContextExt<'tcx> for crate::MiriInterpCx<'tcx> {}
@@ -443,12 +453,10 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         let tracing = trace::Supervisor::is_enabled();
 
         // Get the function arguments, copy them, and prepare the type descriptions.
-        let mut libffi_args = Vec::<CArg>::with_capacity(args.len());
+        let mut libffi_args = Vec::<OwnedArg>::with_capacity(args.len());
         for arg in args.iter() {
-            libffi_args.push(this.op_to_carg(arg, tracing)?);
+            libffi_args.push(this.op_to_ffi_arg(arg, tracing)?);
         }
-        // Convert arguments to a libffi-compatible type.
-        let libffi_args = libffi_args.iter().map(|arg| arg.arg_downcast()).collect::<Vec<_>>();
 
         // Prepare all exposed memory (both previously exposed, and just newly exposed since a
         // pointer was passed as argument). Uninitialised memory is left as-is, but any data
@@ -491,7 +499,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
 
         // Call the function and store output, depending on return type in the function signature.
         let (ret, maybe_memevents) =
-            this.call_native_with_args(link_name, dest, code_ptr, libffi_args)?;
+            this.call_native_with_args(link_name, dest, code_ptr, &libffi_args)?;
 
         if tracing {
             this.tracing_apply_accesses(maybe_memevents.unwrap())?;