native-lib: allow passing structs as arguments

Cargo.toml

@@ -39,7 +39,7 @@ features = ['unprefixed_malloc_on_supported_platforms']
 [target.'cfg(unix)'.dependencies]
 libc = "0.2"
 # native-lib dependencies
-libffi = { version = "4.0.0", optional = true }
+libffi = { version = "4.1.1", optional = true }
 libloading = { version = "0.8", optional = true }
 serde = { version = "1.0.219", features = ["derive"], optional = true }
 

src/shims/native_lib/ffi.rs

@@ -0,0 +1,46 @@
+//! Support code for dealing with libffi.
+
+use libffi::low::CodePtr;
+use libffi::middle::{Arg as ArgPtr, Cif, Type as FfiType};
+
+/// Perform the actual FFI call.
+///
+/// # 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: &mut [OwnedArg]) -> R {
+    let arg_ptrs: Vec<_> = args.iter().map(|arg| arg.ptr()).collect();
+    let cif = Cif::new(args.iter_mut().map(|arg| arg.ty.take().unwrap()), 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
+    // dropped by this point.
+    unsafe { cif.call(fun, &arg_ptrs) }
+}
+
+/// An argument for an FFI call.
+#[derive(Debug, Clone)]
+pub struct OwnedArg {
+    /// The type descriptor for this argument.
+    ty: Option<FfiType>,
+    /// Corresponding bytes for the value.
+    bytes: Box<[u8]>,
+}
+
+impl OwnedArg {
+    /// Instantiates an argument from a type descriptor and bytes.
+    pub fn new(ty: FfiType, bytes: Box<[u8]>) -> Self {
+        Self { ty: Some(ty), bytes }
+    }
+
+    /// Creates 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 {
+        // FIXME: Using `&self.bytes[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.
+        ArgPtr::new(&self.bytes[0])
+    }
+}

tests/native-lib/aggregate_arguments.c

@@ -0,0 +1,52 @@
+#include <stdint.h>
+
+// See comments in build_native_lib()
+#define EXPORT __attribute__((visibility("default")))
+
+/* Test: fail/pass_struct_expose_only_range */
+
+typedef struct HasPointer {
+  uint8_t *ptr;
+} HasPointer;
+
+EXPORT uint8_t access_struct_ptr(const HasPointer s) {
+  return *s.ptr;
+}
+
+/* Test: test_pass_struct */
+
+typedef struct PassMe {
+    int32_t value;
+    int64_t other_value;
+} PassMe;
+
+EXPORT int64_t pass_struct(const PassMe pass_me) {
+  return pass_me.value + pass_me.other_value;
+}
+
+/* Test: test_pass_struct_complex */
+
+typedef struct Part1 {
+    uint16_t high;
+    uint16_t low;
+} Part1;
+
+typedef struct Part2 {
+    uint32_t bits;
+} Part2;
+
+typedef struct ComplexStruct {
+    Part1 part_1;
+    Part2 part_2;
+    uint32_t part_3;
+} ComplexStruct;
+
+EXPORT int32_t pass_struct_complex(const ComplexStruct complex, uint16_t high, uint16_t low, uint32_t bits) {
+  if (complex.part_1.high == high && complex.part_1.low == low
+      && complex.part_2.bits == bits
+      && complex.part_3 == bits)
+    return 0;
+  else {
+    return 1;
+  }
+}

tests/native-lib/fail/pass_struct_expose_only_range.rs

@@ -0,0 +1,23 @@
+//@compile-flags: -Zmiri-permissive-provenance
+
+#[repr(C)]
+#[derive(Copy, Clone)]
+struct HasPointer {
+    ptr: *const u8,
+}
+
+extern "C" {
+    fn access_struct_ptr(s: HasPointer) -> u8;
+}
+
+fn main() {
+    let structs = vec![HasPointer { ptr: &0 }, HasPointer { ptr: &1 }];
+    unsafe {
+        let r = access_struct_ptr(structs[1]);
+        assert_eq!(r, 1);
+        // There are two pointers stored in the allocation backing `structs`; ensure
+        // we only exposed the one that was actually passed to C.
+        let _val = *std::ptr::with_exposed_provenance::<u8>(structs[1].ptr.addr()); // fine, ptr got sent to C
+        let _val = *std::ptr::with_exposed_provenance::<u8>(structs[0].ptr.addr()); //~ ERROR: memory access failed
+    };
+}

tests/native-lib/fail/pass_struct_expose_only_range.stderr

@@ -0,0 +1,28 @@
+warning: sharing memory with a native function called via FFI
+  --> tests/native-lib/fail/pass_struct_expose_only_range.rs:LL:CC
+   |
+LL |         let r = access_struct_ptr(structs[1]);
+   |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ sharing memory with a native function
+   |
+   = help: when memory is shared with a native function call, Miri stops tracking initialization and provenance for that memory
+   = help: in particular, Miri assumes that the native call initializes all memory it has access to
+   = help: Miri also assumes that any part of this memory may be a pointer that is permitted to point to arbitrary exposed memory
+   = help: what this means is that Miri will easily miss Undefined Behavior related to incorrect usage of this shared memory, so you should not take a clean Miri run as a signal that your FFI code is UB-free
+   = note: BACKTRACE:
+   = note: inside `main` at tests/native-lib/fail/pass_struct_expose_only_range.rs:LL:CC
+
+error: Undefined Behavior: memory access failed: attempting to access 1 byte, but got $HEX[noalloc] which is a dangling pointer (it has no provenance)
+  --> tests/native-lib/fail/pass_struct_expose_only_range.rs:LL:CC
+   |
+LL |         let _val = *std::ptr::with_exposed_provenance::<u8>(structs[0].ptr.addr());
+   |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Undefined Behavior occurred here
+   |
+   = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
+   = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
+   = note: BACKTRACE:
+   = note: inside `main` at tests/native-lib/fail/pass_struct_expose_only_range.rs:LL:CC
+
+note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
+
+error: aborting due to 1 previous error; 1 warning emitted
+

tests/native-lib/fail/struct_not_extern_c.rs

@@ -0,0 +1,19 @@
+// Only works on Unix targets
+//@ignore-target: windows wasm
+//@only-on-host
+
+#![allow(improper_ctypes)]
+
+pub struct PassMe {
+    pub value: i32,
+    pub other_value: i64,
+}
+
+extern "C" {
+    fn pass_struct(s: PassMe) -> i64;
+}
+
+fn main() {
+    let pass_me = PassMe { value: 42, other_value: 1337 };
+    unsafe { pass_struct(pass_me) }; //~ ERROR: unsupported operation: passing a non-#[repr(C)] struct over FFI
+}

tests/native-lib/fail/struct_not_extern_c.stderr

@@ -0,0 +1,14 @@
+error: unsupported operation: passing a non-#[repr(C)] struct over FFI: PassMe
+  --> tests/native-lib/fail/struct_not_extern_c.rs:LL:CC
+   |
+LL |     unsafe { pass_struct(pass_me) };
+   |              ^^^^^^^^^^^^^^^^^^^^ unsupported operation occurred here
+   |
+   = help: this is likely not a bug in the program; it indicates that the program performed an operation that Miri does not support
+   = note: BACKTRACE:
+   = note: inside `main` at tests/native-lib/fail/struct_not_extern_c.rs:LL:CC
+
+note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
+
+error: aborting due to 1 previous error
+

tests/native-lib/fail/uninit_struct.rs

@@ -0,0 +1,27 @@
+#[repr(C)]
+#[derive(Copy, Clone)]
+struct ComplexStruct {
+    part_1: Part1,
+    part_2: Part2,
+    part_3: u32,
+}
+#[repr(C)]
+#[derive(Copy, Clone)]
+struct Part1 {
+    high: u16,
+    low: u16,
+}
+#[repr(C)]
+#[derive(Copy, Clone)]
+struct Part2 {
+    bits: u32,
+}
+
+extern "C" {
+    fn pass_struct_complex(s: ComplexStruct, high: u16, low: u16, bits: u32) -> i32;
+}
+
+fn main() {
+    let arg = std::mem::MaybeUninit::<ComplexStruct>::uninit();
+    unsafe { pass_struct_complex(*arg.as_ptr(), 0, 0, 0) }; //~ ERROR: Undefined Behavior: constructing invalid value
+}

tests/native-lib/fail/uninit_struct.stderr

@@ -0,0 +1,15 @@
+error: Undefined Behavior: constructing invalid value at .part_1.high: encountered uninitialized memory, but expected an integer
+  --> tests/native-lib/fail/uninit_struct.rs:LL:CC
+   |
+LL |     unsafe { pass_struct_complex(*arg.as_ptr(), 0, 0, 0) };
+   |                                  ^^^^^^^^^^^^^ Undefined Behavior occurred here
+   |
+   = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
+   = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
+   = note: BACKTRACE:
+   = note: inside `main` at tests/native-lib/fail/uninit_struct.rs:LL:CC
+
+note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
+
+error: aborting due to 1 previous error
+

tests/native-lib/pass/aggregate_arguments.rs

@@ -0,0 +1,52 @@
+fn main() {
+    test_pass_struct();
+    test_pass_struct_complex();
+}
+
+/// Test passing a basic struct as an argument.
+fn test_pass_struct() {
+    // Exactly two fields, so that we hit the ScalarPair case.
+    #[repr(C)]
+    struct PassMe {
+        value: i32,
+        other_value: i64,
+    }
+
+    extern "C" {
+        fn pass_struct(s: PassMe) -> i64;
+    }
+
+    let pass_me = PassMe { value: 42, other_value: 1337 };
+    assert_eq!(unsafe { pass_struct(pass_me) }, 42 + 1337);
+}
+
+/// Test passing a more complex struct as an argument.
+fn test_pass_struct_complex() {
+    #[repr(C)]
+    struct ComplexStruct {
+        part_1: Part1,
+        part_2: Part2,
+        part_3: u32,
+    }
+    #[repr(C)]
+    struct Part1 {
+        high: u16,
+        low: u16,
+    }
+    #[repr(C)]
+    struct Part2 {
+        bits: u32,
+    }
+
+    extern "C" {
+        fn pass_struct_complex(s: ComplexStruct, high: u16, low: u16, bits: u32) -> i32;
+    }
+
+    let high = 0xabcd;
+    let low = 0xef01;
+    let bits = 0xabcdef01;
+
+    let complex =
+        ComplexStruct { part_1: Part1 { high, low }, part_2: Part2 { bits }, part_3: bits };
+    assert_eq!(unsafe { pass_struct_complex(complex, high, low, bits) }, 0);
+}

tests/native-lib/pass/ptr_read_access.rs

@@ -1,6 +1,7 @@
 //@revisions: trace notrace
 //@[trace] only-target: x86_64-unknown-linux-gnu i686-unknown-linux-gnu
 //@[trace] compile-flags: -Zmiri-native-lib-enable-tracing
+//@compile-flags: -Zmiri-permissive-provenance
 
 fn main() {
     test_access_pointer();

tests/ui.rs

@@ -60,6 +60,7 @@ fn build_native_lib(target: &str) -> PathBuf {
             native_lib_path.to_str().unwrap(),
             // FIXME: Automate gathering of all relevant C source files in the directory.
             "tests/native-lib/scalar_arguments.c",
+            "tests/native-lib/aggregate_arguments.c",
             "tests/native-lib/ptr_read_access.c",
             "tests/native-lib/ptr_write_access.c",
             // Ensure we notice serious problems in the C code.