Address review comments

weiznich · weiznich · commit 9ccb197c323b · 2025-12-06T12:20:26.000+01:00
This moves the creation of the closure to a more meaningful location.
Additionally it adds a test + aborts the execution as soon as the
closure is called as we currently cannot return a reasonable value
there.
diff --git a/src/alloc_addresses/mod.rs b/src/alloc_addresses/mod.rs
@@ -8,7 +8,7 @@ use std::cell::RefCell;
 
 use rustc_abi::{Align, Size};
 use rustc_data_structures::fx::{FxHashMap, FxHashSet};
-use rustc_middle::ty::TyCtxt;
+use rustc_middle::ty::{InstanceKind, TyCtxt};
 
 pub use self::address_generator::AddressGenerator;
 use self::reuse_pool::ReusePool;
@@ -169,9 +169,39 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
                         Align::from_bytes(1).unwrap(),
                         params,
                     );
-                    let ptr = alloc_bytes.as_ptr();
+                    let mut ptr = alloc_bytes.as_ptr();
                     // Leak the underlying memory to ensure it remains unique.
                     std::mem::forget(alloc_bytes);
+                    if let Some(GlobalAlloc::Function { instance, .. }) =
+                        this.tcx.try_get_global_alloc(alloc_id)
+                    {
+                        #[cfg(all(unix, feature = "native-lib"))]
+                        if let InstanceKind::Item(def_id) = instance.def {
+                            let sig = this.tcx.fn_sig(def_id).skip_binder().skip_binder();
+                            let closure =
+                                crate::shims::native_lib::build_libffi_closure(this, sig)?;
+                            if let Some(closure) = closure {
+                                let closure = Box::leak(Box::new(closure));
+                                // Libffi returns a **reference** to a function ptr here
+                                // (The actual argument type doesn't matter)
+                                let fn_ptr = unsafe {
+                                    closure.instantiate_code_ptr::<unsafe extern "C" fn(*const std::ffi::c_void)>()
+                                };
+                                // Therefore we need to dereference the reference to get the actual function pointer
+                                let fn_ptr = *fn_ptr;
+                                #[expect(
+                                    clippy::as_conversions,
+                                    reason = "No better way to cast a function ptr to a ptr"
+                                )]
+                                {
+                                    // After that we need to cast the function pointer to the
+                                    // expected pointer type. At this point we don't actually care about the
+                                    // type of this pointer
+                                    ptr = (fn_ptr as *const std::ffi::c_void).cast();
+                                }
+                            }
+                        }
+                    }
                     ptr
                 }
                 AllocKind::TypeId | AllocKind::Dead => unreachable!(),
diff --git a/src/shims/mod.rs b/src/shims/mod.rs
@@ -6,7 +6,7 @@ mod backtrace;
 mod files;
 mod math;
 #[cfg(all(unix, feature = "native-lib"))]
-mod native_lib;
+pub mod native_lib;
 mod unix;
 mod windows;
 mod x86;
diff --git a/src/shims/native_lib/mod.rs b/src/shims/native_lib/mod.rs
@@ -1,7 +1,5 @@
 //! Implements calling functions from a native library.
 
-use std::borrow::Cow;
-use std::cell::RefCell;
 use std::ops::Deref;
 use std::os::raw::c_void;
 use std::sync::atomic::AtomicBool;
@@ -19,14 +17,6 @@ use self::helpers::ToSoft;
 
 mod ffi;
 
-struct CallbackError {
-    message: Cow<'static, str>,
-}
-
-thread_local! {
-    pub static CALLBACK_MESSAGES: RefCell<Vec<CallbackError>> = RefCell::new(Vec::new());
-}
-
 #[cfg_attr(
     not(all(
         target_os = "linux",
@@ -103,8 +93,6 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
         let alloc = ();
 
         trace::Supervisor::do_ffi(alloc, || {
-            // clear the callback error buffer
-            CALLBACK_MESSAGES.with_borrow_mut(|c| c.clear());
             // Call the function (`ptr`) with arguments `libffi_args`, and obtain the return value
             // as the specified primitive integer type
             let scalar = match dest.layout.ty.kind() {
@@ -179,11 +167,6 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
                     ))
                     .into(),
             };
-            let callback_error_messages = CALLBACK_MESSAGES.take();
-            if !callback_error_messages.is_empty() {
-                let first = callback_error_messages.first().unwrap();
-                return Err(err_unsup_format!("{}", first.message)).into();
-            }
             interp_ok(ImmTy::from_scalar(scalar, dest.layout))
         })
     }
@@ -303,12 +286,7 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
 
     /// Extract the value from the result of reading an operand from the machine
     /// and convert it to a `OwnedArg`.
-    fn op_to_ffi_arg(
-        &self,
-        v: &OpTy<'tcx>,
-        tracing: bool,
-        link_name: &Symbol,
-    ) -> InterpResult<'tcx, OwnedArg> {
+    fn op_to_ffi_arg(&self, v: &OpTy<'tcx>, tracing: bool) -> InterpResult<'tcx, OwnedArg> {
         let this = self.eval_context_ref();
 
         // This should go first so that we emit unsupported before doing a bunch
@@ -333,44 +311,6 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
         // casting the integer in `byte` to a pointer and using that.
         let bytes = match v.as_mplace_or_imm() {
             either::Either::Left(mplace) => {
-                let ptr_overwrite = match v.layout.ty.kind() {
-                    ty::Adt(_adt_def, args) =>
-                        if let ty::FnPtr(fn_ptr, _header) = args.type_at(0).kind() {
-                            let args = fn_ptr
-                                .skip_binder()
-                                .inputs()
-                                .into_iter()
-                                .map(|i| {
-                                    let layout = this.layout_of(i.clone())?;
-                                    this.ty_to_ffitype(layout)
-                                })
-                                .collect::<InterpResult<'_, Vec<_>>>()?;
-                            let res_type = fn_ptr.skip_binder().output();
-                            let res_type = {
-                                let layout = this.layout_of(res_type)?;
-                                this.ty_to_ffitype(layout)?
-                            };
-                            let closure_builder = libffi::middle::Builder::new()
-                                .args(args)
-                                .res(res_type)
-                                .abi(libffi::raw::ffi_abi_FFI_UNIX64);
-                            let data = CallbackData {
-                                args: fn_ptr.skip_binder().inputs().to_vec(),
-                                result: fn_ptr.skip_binder().output(),
-                                this,
-                                link_name: link_name.clone(),
-                                ty: v.layout.ty,
-                            };
-                            // todo: leaking is likely not optimal here
-                            let data = Box::leak(Box::new(data));
-
-                            let closure = closure_builder.into_closure(callback_callback, data);
-                            Some(closure)
-                        } else {
-                            None
-                        },
-                    _ => None,
-                };
                 // 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).
@@ -393,33 +333,7 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
                 // is kind of outside the interpreter, after all...
                 let ret: Box<[u8]> =
                     Box::from(alloc.inspect_with_uninit_and_ptr_outside_interpreter(range));
-                if ret.iter().any(|b| *b != 0)
-                    && let Some(ptr_overwrite) = ptr_overwrite
-                {
-                    // we need to leak the closure here as we don't know when it's actually called
-                    // I'm not sure if it's possible to have a better solution for that
-                    let ptr_overwrite = Box::leak(Box::new(ptr_overwrite));
-
-                    // we get a **reference** to a function ptr here
-                    // (The actual argument type doesn't matter)
-                    let ptr = unsafe {
-                        ptr_overwrite.instantiate_code_ptr::<unsafe extern "C" fn(*const c_void)>()
-                    };
-                    // so deref away the reference
-                    let ptr = *ptr;
-                    // cast it to void as the actual function type doesn't matter
-                    let ptr = ptr as *const c_void;
-                    // get a the address as usize to write it into the
-                    // right memory location
-                    let bytes = ptr.addr();
-                    // bytes are in native endian, as that's literally
-                    // the definition of native endian
-                    let bytes = usize::to_ne_bytes(bytes);
-                    // return the bytes of the ptr
-                    Box::from(bytes)
-                } else {
-                    ret
-                }
+                ret
             }
             either::Either::Right(imm) => {
                 let mut bytes: Box<[u8]> = vec![0; imm.layout.size.bytes_usize()].into();
@@ -538,6 +452,57 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
     }
 }
 
+pub fn build_libffi_closure<'tcx, 'this>(
+    this: &'this MiriInterpCx<'tcx>,
+    fn_ptr: rustc_middle::ty::FnSig<'tcx>,
+) -> InterpResult<'tcx, Option<libffi::middle::Closure<'this>>> {
+    let mut args = Vec::new();
+    for input in fn_ptr.inputs().iter() {
+        let layout = match this.layout_of(*input) {
+            Ok(layout) => layout,
+            Err(e) => {
+                tracing::info!(?e, "Skip closure");
+                return interp_ok(None);
+            }
+        };
+        let ty = match this.ty_to_ffitype(layout).report_err() {
+            Ok(ty) => ty,
+            Err(e) => {
+                tracing::info!(?e, "Skip closure");
+                return interp_ok(None);
+            }
+        };
+        args.push(ty);
+    }
+    let res_type = fn_ptr.output();
+    let res_type = {
+        let layout = match this.layout_of(res_type) {
+            Ok(layout) => layout,
+            Err(e) => {
+                tracing::info!(?e, "Skip closure");
+                return interp_ok(None);
+            }
+        };
+        match this.ty_to_ffitype(layout).report_err() {
+            Ok(ty) => ty,
+            Err(e) => {
+                tracing::info!(?e, "Skip closure");
+                return interp_ok(None);
+            }
+        }
+    };
+    let closure_builder = libffi::middle::Builder::new().args(args).res(res_type);
+    let data = CallbackData {
+        args: fn_ptr.inputs().to_vec(),
+        result: fn_ptr.output(),
+        this,
+        //link_name: *link_name,
+    };
+    let data = Box::leak(Box::new(data));
+    let closure = closure_builder.into_closure(callback_callback, data);
+    interp_ok(Some(closure))
+}
+
 impl<'tcx> EvalContextExt<'tcx> for crate::MiriInterpCx<'tcx> {}
 pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
     /// Call the native host function, with supplied arguments.
@@ -567,7 +532,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         // Get the function arguments, copy them, and prepare the type descriptions.
         let mut libffi_args = Vec::<OwnedArg>::with_capacity(args.len());
         for arg in args.iter() {
-            libffi_args.push(this.op_to_ffi_arg(arg, tracing, &link_name)?);
+            libffi_args.push(this.op_to_ffi_arg(arg, tracing)?);
         }
 
         // Prepare all exposed memory (both previously exposed, and just newly exposed since a
@@ -633,26 +598,26 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
 
 struct CallbackData<'a, 'tcx> {
     args: Vec<Ty<'tcx>>,
+    #[expect(dead_code, reason = "It's there for later")]
     result: Ty<'tcx>,
     this: &'a MiriInterpCx<'tcx>,
-    link_name: Symbol,
-    ty: Ty<'tcx>,
 }
 
 unsafe extern "C" fn callback_callback(
     cif: &libffi::low::ffi_cif,
-    result: &mut c_void,
+    _result: &mut c_void,
     args: *const *const c_void,
     infos: &CallbackData<'_, '_>,
 ) {
-    debug_assert_eq!(cif.nargs as usize, infos.args.len());
+    debug_assert_eq!(cif.nargs.try_into(), Ok(infos.args.len()));
     let mut rust_args = Vec::with_capacity(infos.args.len());
-    // cast away the pointer to pointer
-    let mut args = args as *const c_void;
+    // We cast away the pointer to pointer to get a pointer to the actual argument
+    let mut args = args.cast::<c_void>();
     for arg in &infos.args {
         let scalar = match arg.kind() {
             ty::RawPtr(..) => {
                 let ptr = StrictPointer::new(Provenance::Wildcard, Size::from_bytes(args.addr()));
+                // This offset moves the pointer to the next argument
                 args = unsafe { args.offset(1) };
                 Scalar::from_pointer(ptr, infos.this)
             }
@@ -662,60 +627,13 @@ unsafe extern "C" fn callback_callback(
         rust_args.push(scalar);
     }
 
-    CALLBACK_MESSAGES.with_borrow_mut(|msgs| {
-        msgs.push(CallbackError {
-            message: format!("Tried to call a function pointer via FFI boundary. \
-                      That's not supported yet by miri\n This function pointer was registered by a call to `{}` \
-                      using an argument of the type `{}`", infos.link_name, infos.ty)
-                .into(),
-        });
-    });
-
-    // write here the output
-    // For now we just try to write some dummy output
-    // by using some "reasonable" default values
-    // to prevent crashing
-    match infos.result.kind() {
-        ty::RawPtr(..) => {
-            write_helper::<*mut c_void>(result, std::ptr::null_mut());
-        }
-        ty::Int(IntTy::I8) => {
-            write_helper::<i8>(result, 0);
-        }
-        ty::Int(IntTy::I16) => {
-            write_helper::<i32>(result, 0);
-        }
-        ty::Int(IntTy::I32) => {
-            write_helper::<i32>(result, 0);
-        }
-        ty::Int(IntTy::I64) => {
-            write_helper::<i64>(result, 0);
-        }
-        ty::Int(IntTy::Isize) => {
-            write_helper::<isize>(result, 0);
-        }
-        ty::Uint(UintTy::U8) => {
-            write_helper::<u8>(result, 0);
-        }
-        ty::Uint(UintTy::U16) => {
-            write_helper::<u16>(result, 0);
-        }
-        ty::Uint(UintTy::U32) => {
-            write_helper::<u32>(result, 0);
-        }
-        ty::Uint(UintTy::U64) => {
-            write_helper::<u64>(result, 0);
-        }
-        ty::Uint(UintTy::Usize) => {
-            write_helper::<usize>(result, 0);
-        }
-        // unsure how to handle that at allow
-        // Just do nothing for now?
-        _ => {}
-    };
-}
-
-fn write_helper<T>(ptr: &mut c_void, value: T) {
-    let ptr = (ptr as *mut c_void) as *mut T;
-    unsafe { std::ptr::write(ptr, value) };
+    // We abort the execution at this point as we cannot return the
+    // expected value here.
+    eprintln!(
+        "Tried to call a function pointer via FFI boundary. \
+         That's not supported yet by miri\nThis function pointer was registered by a call to `{}` \
+         using an argument of the type `{}`",
+        "todo: fill in name", "todo: fill in type"
+    );
+    std::process::exit(1);
 }
diff --git a/tests/native-lib/ptr_read_access.c b/tests/native-lib/ptr_read_access.c
@@ -68,3 +68,10 @@ EXPORT uintptr_t do_one_deref(const int32_t ***ptr) {
 EXPORT void pass_fn_ptr(void f(void)) {
   (void)f; // suppress unused warning
 }
+
+/* Test: function_ptrs */
+EXPORT void call_fn_ptr(void f(void)) {
+    if (f != NULL) {
+        f();
+    }
+}
