review 4: a new hope

Author: nia-e

src/bin/miri.rs

@@ -335,6 +335,7 @@ impl rustc_driver::Callbacks for MiriBeRustCompilerCalls {
 fn exit(exit_code: i32) -> ! {
     // Drop the tracing guard before exiting, so tracing calls are flushed correctly.
     deinit_loggers();
+    #[cfg(target_os = "linux")]
     miri::native_lib::register_retcode_sv(exit_code);
     std::process::exit(exit_code);
 }
@@ -752,6 +753,7 @@ fn main() {
     if !miri_config.native_lib.is_empty() && miri_config.native_lib_enable_tracing {
         // FIXME(ptrace): This should display a diagnostic / warning on error
         // SAFETY: No other threads are running
+        #[cfg(target_os = "linux")]
         let _ = unsafe { miri::native_lib::init_sv() };
     }
     run_compiler_and_exit(

src/lib.rs

@@ -97,6 +97,7 @@ pub use rustc_const_eval::interpret::{self, AllocMap, Provenance as _};
 use rustc_middle::{bug, span_bug};
 use tracing::{info, trace};
 
+#[cfg(target_os = "linux")]
 pub mod native_lib {
     pub use crate::shims::{init_sv, register_retcode_sv};
 }

src/shims/native_lib/mod.rs

@@ -21,6 +21,46 @@ pub mod trace;
 
 use crate::*;
 
+/// The final results of an FFI trace, containing every relevant event detected
+/// by the tracer. Sent by the supervisor after receiving a `SIGUSR1` signal.
+///
+/// The sender for this channel should live on the parent process.
+#[allow(dead_code)]
+#[cfg_attr(target_os = "linux", derive(serde::Serialize, serde::Deserialize))]
+#[derive(Debug)]
+pub struct MemEvents {
+    /// An ordered list of memory accesses that occurred. These should be assumed
+    /// to be overcautious; that is, if the size of an access is uncertain it is
+    /// pessimistically rounded up, and if the type (read/write/both) is uncertain
+    /// it is reported as whatever would be safest to assume; i.e. a read + maybe-write
+    /// becomes a read + write, etc.
+    pub acc_events: Vec<AccessEvent>,
+}
+
+/// A single memory access.
+#[allow(dead_code)]
+#[cfg_attr(target_os = "linux", derive(serde::Serialize, serde::Deserialize))]
+#[derive(Debug)]
+pub enum AccessEvent {
+    /// A read may have occurred on no more than the specified address range.
+    Read(AccessRange),
+    /// A write may have occurred on no more than the specified address range.
+    Write(AccessRange),
+}
+
+/// The actual size of a performed access, bounded by size.
+#[allow(dead_code)]
+#[cfg_attr(target_os = "linux", derive(serde::Serialize, serde::Deserialize))]
+#[derive(Clone, Debug)]
+pub struct AccessRange {
+    /// The base address in memory where an access occurred.
+    pub addr: usize,
+    /// The smallest size this access could have been.
+    pub min_size: usize,
+    /// The largest size this access could have been.
+    pub max_size: usize,
+}
+
 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.
@@ -30,7 +70,7 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
         dest: &MPlaceTy<'tcx>,
         ptr: CodePtr,
         libffi_args: Vec<libffi::high::Arg<'a>>,
-    ) -> trace::CallResult<'tcx> {
+    ) -> InterpResult<'tcx, (crate::ImmTy<'tcx>, Option<MemEvents>)> {
         let this = self.eval_context_mut();
         #[cfg(target_os = "linux")]
         let alloc = this.machine.allocator.as_ref().unwrap();
@@ -108,7 +148,7 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
             interp_ok(ImmTy::from_scalar(scalar, dest.layout))
         };
 
-        trace::do_ffi(alloc, ffi_fn)
+        trace::Supervisor::do_ffi(alloc, ffi_fn)
     }
 
     /// Get the pointer to the function of the specified name in the shared object file,

src/shims/native_lib/trace/child.rs

@@ -4,11 +4,13 @@ use std::rc::Rc;
 use ipc_channel::ipc;
 use nix::sys::{ptrace, signal};
 use nix::unistd;
+use rustc_const_eval::interpret::InterpResult;
 
 use super::CALLBACK_STACK_SIZE;
-use super::messages::{Confirmation, MemEvents, StartFfiInfo, TraceRequest};
+use super::messages::{Confirmation, StartFfiInfo, TraceRequest};
 use super::parent::{ChildListener, sv_loop};
 use crate::alloc::isolated_alloc::IsolatedAlloc;
+use crate::shims::native_lib::MemEvents;
 
 /// A handle to the single, shared supervisor process across all `MiriMachine`s.
 /// Since it would be very difficult to trace multiple FFI calls in parallel, we
@@ -32,12 +34,6 @@ pub struct Supervisor {
     event_rx: ipc::IpcReceiver<MemEvents>,
 }
 
-pub struct SvFfiGuard<'a> {
-    alloc: &'a Rc<RefCell<IsolatedAlloc>>,
-    sv_guard: std::sync::MutexGuard<'static, Option<Supervisor>>,
-    cb_stack: Option<*mut [u8; CALLBACK_STACK_SIZE]>,
-}
-
 /// Marker representing that an error occurred during creation of the supervisor.
 #[derive(Debug)]
 pub struct SvInitError;
@@ -48,24 +44,20 @@ impl Supervisor {
         SUPERVISOR.lock().unwrap().is_some()
     }
 
-    /// Begins preparations for doing an FFI call. This should be called at
-    /// the last possible moment before entering said call. `alloc` points to
-    /// the allocator which handed out the memory used for this machine.
-    ///
+    /// Performs an arbitrary FFI call, enabling tracing from the supervisor.
     /// As this locks the supervisor via a mutex, no other threads may enter FFI
-    /// until this one returns and its guard is dropped via `end_ffi`. The
-    /// pointer returned should be passed to `end_ffi` to avoid a memory leak.
-    ///
-    /// SAFETY: The resulting guard must be dropped *via `end_ffi`* immediately
-    /// after the desired call has concluded.
-    pub unsafe fn start_ffi(alloc: &Rc<RefCell<IsolatedAlloc>>) -> SvFfiGuard<'_> {
+    /// until this function returns.
+    pub fn do_ffi<'tcx>(
+        alloc: &Rc<RefCell<IsolatedAlloc>>,
+        f: impl FnOnce() -> InterpResult<'tcx, crate::ImmTy<'tcx>>,
+    ) -> InterpResult<'tcx, (crate::ImmTy<'tcx>, Option<MemEvents>)> {
         let mut sv_guard = SUPERVISOR.lock().unwrap();
         // If the supervisor is not initialised for whatever reason, fast-return.
         // This might be desired behaviour, as even on platforms where ptracing
         // is not implemented it enables us to enforce that only one FFI call
         // happens at a time.
         let Some(sv) = sv_guard.as_mut() else {
-            return SvFfiGuard { alloc, sv_guard, cb_stack: None };
+            return f().map(|v| (v, None))
         };
 
         // Get pointers to all the pages the supervisor must allow accesses in
@@ -97,21 +89,8 @@ impl Supervisor {
         // modifications to our memory - simply waiting on the recv() doesn't
         // count.
         signal::raise(signal::SIGSTOP).unwrap();
-        SvFfiGuard { alloc, sv_guard, cb_stack: Some(raw_stack_ptr) }
-    }
 
-    /// Undoes FFI-related preparations, allowing Miri to continue as normal, then
-    /// gets the memory accesses and changes performed during the FFI call. Note
-    /// that this may include some spurious accesses done by `libffi` itself in
-    /// the process of executing the function call.
-    ///
-    /// SAFETY: The `sv_guard` and `raw_stack_ptr` passed must be the same ones
-    /// received by a prior call to `start_ffi`, and the allocator must be the
-    /// one passed to it also.
-    pub unsafe fn end_ffi(guard: SvFfiGuard<'_>) -> Option<MemEvents> {
-        let alloc = guard.alloc;
-        let mut sv_guard = guard.sv_guard;
-        let cb_stack = guard.cb_stack;
+        let res = f();
 
         // We can't use IPC channels here to signal that FFI mode has ended,
         // since they might allocate memory which could get us stuck in a SIGTRAP
@@ -125,17 +104,13 @@ impl Supervisor {
         // This is safe! It just sets memory to normal expected permissions.
         alloc.borrow_mut().end_ffi();
 
-        // If this is `None`, then `raw_stack_ptr` is None and does not need to
-        // be deallocated (and there's no need to worry about the guard, since
-        // it contains nothing).
-        let sv = sv_guard.as_mut()?;
         // SAFETY: Caller upholds that this pointer was allocated as a box with
         // this type.
         unsafe {
-            drop(Box::from_raw(cb_stack.unwrap()));
+            drop(Box::from_raw(raw_stack_ptr));
         }
         // On the off-chance something really weird happens, don't block forever.
-        sv.event_rx
+        let events = sv.event_rx
             .try_recv_timeout(std::time::Duration::from_secs(5))
             .map_err(|e| {
                 match e {
@@ -144,14 +119,16 @@ impl Supervisor {
                         panic!("Waiting for accesses from supervisor timed out!"),
                 }
             })
-            .ok()
+            .ok();
+
+        res.map(|v| (v, events))
     }
 }
 
 /// Initialises the supervisor process. If this function errors, then the
 /// supervisor process could not be created successfully; else, the caller
-/// is now the child process and can communicate via `start_ffi`/`end_ffi`,
-/// receiving back events through `get_events`.
+/// is now the child process and can communicate via `do_ffi`, receiving back
+/// events at the end.
 ///
 /// # Safety
 /// The invariants for `fork()` must be upheld by the caller, namely either:

src/shims/native_lib/trace/messages.rs

@@ -19,11 +19,9 @@
 //!
 //! NB: sending these events out of order, skipping steps, etc. will result in
 //! unspecified behaviour from the supervisor process, so use the abstractions
-//! in `super::child` (namely `start_ffi()` and `end_ffi()`) to handle this. It is
+//! in `super::child` (namely `do_ffi()`) to handle this. It is
 //! trivially easy to cause a deadlock or crash by messing this up!
 
-use std::ops::Range;
-
 /// An IPC request sent by the child process to the parent.
 ///
 /// The sender for this channel should live on the child process.
@@ -58,27 +56,3 @@ pub struct StartFfiInfo {
 /// The sender for this channel should live on the parent process.
 #[derive(serde::Serialize, serde::Deserialize, Debug)]
 pub struct Confirmation;
-
-/// The final results of an FFI trace, containing every relevant event detected
-/// by the tracer. Sent by the supervisor after receiving a `SIGUSR1` signal.
-///
-/// The sender for this channel should live on the parent process.
-#[derive(serde::Serialize, serde::Deserialize, Debug)]
-pub struct MemEvents {
-    /// An ordered list of memory accesses that occurred. These should be assumed
-    /// to be overcautious; that is, if the size of an access is uncertain it is
-    /// pessimistically rounded up, and if the type (read/write/both) is uncertain
-    /// it is reported as whatever would be safest to assume; i.e. a read + maybe-write
-    /// becomes a read + write, etc.
-    pub acc_events: Vec<AccessEvent>,
-}
-
-/// A single memory access, conservatively overestimated
-/// in case of ambiguity.
-#[derive(serde::Serialize, serde::Deserialize, Debug)]
-pub enum AccessEvent {
-    /// A read may have occurred on no more than the specified address range.
-    Read(Range<usize>),
-    /// A write may have occurred on no more than the specified address range.
-    Write(Range<usize>),
-}

src/shims/native_lib/trace/mod.rs

@@ -2,34 +2,9 @@ mod child;
 pub mod messages;
 mod parent;
 
-use std::cell::RefCell;
-use std::rc::Rc;
-
-use rustc_const_eval::interpret::InterpResult;
-
 pub use self::child::{Supervisor, init_sv, register_retcode_sv};
-use crate::alloc::isolated_alloc::IsolatedAlloc;
 
 /// The size of the temporary stack we use for callbacks that the server executes in the client.
 /// This should be big enough that `mempr_on` and `mempr_off` can safely be jumped into with the
 /// stack pointer pointing to a "stack" of this size without overflowing it.
 const CALLBACK_STACK_SIZE: usize = 1024;
-
-/// Wrapper type for what we get back from an FFI call; the former is its actual
-/// return value, and the latter is the list of memory accesses that occurred during
-/// this call.
-pub type CallResult<'tcx> = InterpResult<'tcx, (crate::ImmTy<'tcx>, Option<messages::MemEvents>)>;
-
-/// Performs an arbitrary FFI call, enabling tracing from the supervisor.
-pub fn do_ffi<'tcx>(
-    alloc: &Rc<RefCell<IsolatedAlloc>>,
-    f: impl FnOnce() -> InterpResult<'tcx, crate::ImmTy<'tcx>>,
-) -> CallResult<'tcx> {
-    // SAFETY: We don't touch the machine memory past this point.
-    let guard = unsafe { Supervisor::start_ffi(alloc) };
-
-    f().map(|v| {
-        let memevents = unsafe { Supervisor::end_ffi(guard) };
-        (v, memevents)
-    })
-}