-
Notifications
You must be signed in to change notification settings - Fork 413
Basic support for FFI callbacks (at least stop segfaulting and report a proper error) #4728
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| //! Implements calling functions from a native library. | ||
|
|
||
| use std::ops::Deref; | ||
| use std::os::raw::c_void; | ||
| use std::sync::atomic::AtomicBool; | ||
|
|
||
| use libffi::low::CodePtr; | ||
|
|
@@ -330,7 +331,9 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> { | |
| // Read the bytes that make up this argument. We cannot use the normal getter as | ||
| // those would fail if any part of the argument is uninitialized. Native code | ||
| // is kind of outside the interpreter, after all... | ||
| Box::from(alloc.inspect_with_uninit_and_ptr_outside_interpreter(range)) | ||
| let ret: Box<[u8]> = | ||
| Box::from(alloc.inspect_with_uninit_and_ptr_outside_interpreter(range)); | ||
| ret | ||
| } | ||
| either::Either::Right(imm) => { | ||
| let mut bytes: Box<[u8]> = vec![0; imm.layout.size.bytes_usize()].into(); | ||
|
|
@@ -439,11 +442,67 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> { | |
| interp_ok(match layout.ty.kind() { | ||
| // Scalar types have already been handled above. | ||
| ty::Adt(adt_def, args) => self.adt_to_ffitype(layout.ty, *adt_def, args)?, | ||
| _ => throw_unsup_format!("unsupported argument type for native call: {}", layout.ty), | ||
| // Functions with no declared return type (i.e., the default return) | ||
| // have the output_type `Tuple([])`. | ||
| ty::Tuple(t_list) if (*t_list).deref().is_empty() => FfiType::void(), | ||
|
Comment on lines
+453
to
+455
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should not be needed -- function pointers are scalar types.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The function I happened to use for testing returned That written: You are correct that this is not needed for a minimal support of callback over fro, it just happens to help with my particular test case and was rather straightforward to add.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Ah, that makes sense. Given that I think we should never return from that closure (see my other comments), I think we should not need this either.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is still required as we need the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could handle this separately in another PR and make all tests here have an already implemented return type. But not too important
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can do this, but in that case I likely also need to change existing tests to make it work with the implemented feature as they also use void as return/argument type for the callback. |
||
| _ => { | ||
| throw_unsup_format!("unsupported argument type for native call: {}", layout.ty) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| 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. | ||
|
|
@@ -536,3 +595,45 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { | |
| interp_ok(true) | ||
| } | ||
| } | ||
|
|
||
| struct CallbackData<'a, 'tcx> { | ||
| args: Vec<Ty<'tcx>>, | ||
| #[expect(dead_code, reason = "It's there for later")] | ||
| result: Ty<'tcx>, | ||
| this: &'a MiriInterpCx<'tcx>, | ||
| } | ||
|
|
||
| unsafe extern "C" fn callback_callback( | ||
| cif: &libffi::low::ffi_cif, | ||
| _result: &mut c_void, | ||
| args: *const *const c_void, | ||
| infos: &CallbackData<'_, '_>, | ||
| ) { | ||
| debug_assert_eq!(cif.nargs.try_into(), Ok(infos.args.len())); | ||
| let mut rust_args = Vec::with_capacity(infos.args.len()); | ||
| // 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) | ||
| } | ||
| // the other types | ||
| _ => todo!(), | ||
| }; | ||
| rust_args.push(scalar); | ||
| } | ||
|
||
|
|
||
| // 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); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| warning: sharing memory with a native function called via FFI | ||
| --> tests/native-lib/fail/call_function_ptr.rs:LL:CC | ||
| | | ||
| LL | call_fn_ptr(Some(nop)); // this one is not | ||
| | ^^^^^^^^^^^^^^^^^^^^^^ 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 `pass_fn_ptr` at tests/native-lib/fail/call_function_ptr.rs:LL:CC | ||
| note: inside `main` | ||
| --> tests/native-lib/fail/call_function_ptr.rs:LL:CC | ||
| | | ||
| LL | pass_fn_ptr() | ||
| | ^^^^^^^^^^^^^ | ||
|
|
||
| warning: sharing a function pointer with a native function called via FFI | ||
| --> tests/native-lib/fail/call_function_ptr.rs:LL:CC | ||
| | | ||
| LL | call_fn_ptr(Some(nop)); // this one is not | ||
| | ^^^^^^^^^^^^^^^^^^^^^^ sharing a function pointer with a native function | ||
| | | ||
| = help: calling Rust functions from C is not supported and will, in the best case, crash the program | ||
| = note: BACKTRACE: | ||
| = note: inside `pass_fn_ptr` at tests/native-lib/fail/call_function_ptr.rs:LL:CC | ||
| note: inside `main` | ||
| --> tests/native-lib/fail/call_function_ptr.rs:LL:CC | ||
| | | ||
| LL | pass_fn_ptr() | ||
| | ^^^^^^^^^^^^^ | ||
|
|
||
| Tried to call a function pointer via FFI boundary. That's not supported yet by miri | ||
| This function pointer was registered by a call to `todo: fill in name` using an argument of the type `todo: fill in type` |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| //@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() { | ||
| pass_fn_ptr() | ||
| } | ||
|
|
||
| fn pass_fn_ptr() { | ||
| extern "C" { | ||
| fn call_fn_ptr(s: Option<extern "C" fn()>); | ||
| } | ||
|
|
||
| extern "C" fn nop() {} | ||
|
|
||
| unsafe { | ||
| call_fn_ptr(None); // this one is fine | ||
| call_fn_ptr(Some(nop)); // this one is not | ||
|
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| warning: sharing memory with a native function called via FFI | ||
| --> tests/native-lib/fail/call_function_ptr.rs:LL:CC | ||
| | | ||
| LL | call_fn_ptr(Some(nop)); // this one is not | ||
| | ^^^^^^^^^^^^^^^^^^^^^^ sharing memory with a native function | ||
| | | ||
| = help: when memory is shared with a native function call, Miri can only track initialisation and provenance on a best-effort basis | ||
| = help: in particular, Miri assumes that the native call initializes all memory it has written 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 | ||
| = help: tracing memory accesses in native code is not yet fully implemented, so there can be further imprecisions beyond what is documented here | ||
| = note: BACKTRACE: | ||
| = note: inside `pass_fn_ptr` at tests/native-lib/fail/call_function_ptr.rs:LL:CC | ||
| note: inside `main` | ||
| --> tests/native-lib/fail/call_function_ptr.rs:LL:CC | ||
| | | ||
| LL | pass_fn_ptr() | ||
| | ^^^^^^^^^^^^^ | ||
|
|
||
| warning: sharing a function pointer with a native function called via FFI | ||
| --> tests/native-lib/fail/call_function_ptr.rs:LL:CC | ||
| | | ||
| LL | call_fn_ptr(Some(nop)); // this one is not | ||
| | ^^^^^^^^^^^^^^^^^^^^^^ sharing a function pointer with a native function | ||
| | | ||
| = help: calling Rust functions from C is not supported and will, in the best case, crash the program | ||
| = note: BACKTRACE: | ||
| = note: inside `pass_fn_ptr` at tests/native-lib/fail/call_function_ptr.rs:LL:CC | ||
| note: inside `main` | ||
| --> tests/native-lib/fail/call_function_ptr.rs:LL:CC | ||
| | | ||
| LL | pass_fn_ptr() | ||
| | ^^^^^^^^^^^^^ | ||
|
|
||
| Tried to call a function pointer via FFI boundary. That's not supported yet by miri | ||
| This function pointer was registered by a call to `todo: fill in name` using an argument of the type `todo: fill in type` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above code is irrelevant for this code path. Split the function code path from the vtable code path and unwrap the function global alloc as all AllocKind::Function should be backed by a GlobalAlloc::Function I think