Shims should check ABI

[RalfJung]

Our shims for missing extern functions now all check that the right number of arguments is being passed. However, they should also check that the right ABI is used.

To do this, probably first librustc_mir needs to be changed to even propagate this information to Miri's find_mir_or_eval_fn.

The goal is to reject buggy code like this:

fn main() {
    extern "Rust" { // should be "C"!
        fn malloc(size: usize) -> *mut std::ffi::c_void;
    }

    unsafe {
        let _ = malloc(0);
    };
}

[unseddd]

Hi, I started looking into this issue a little, and wanted to ask for some guidance before diving too much deeper in the rabbit hole.

The check in miri/src/shims/mod.rs:44:

if this.tcx.is_foreign_item(instance.def_id())

and call to:

this.emulate_foreign_item(instance.def_id(), args, ret, unwind)

do matches on function name and OS type.

Since some ABI matching is done in latter function (and its inner calls), what other preliminary checks are needed in EvalContextExt::find_mir_or_eval_fn?

Should the arg types be matches against layouts for the target OS?

Should the OS from this.tcx.sess.target.os be checked against something supplied as an argument (or one of their inner field/values)?

Should a lookup of the function being interpreted be done with something like:

// Check foreign allocation
let alloc_id = this.machine.extern_static_alloc_id(&this.memory, instance.def.def_id())?;
match this.memory.get_fn_alloc(alloc_id) {
  Some(fn_val) => ... check fn_val matches instance?,
  None => throw error,
};

Thanks in advance for any help.

[RalfJung]

Since some ABI matching is done in latter function (and its inner calls), what other preliminary checks are needed in EvalContextExt::find_mir_or_eval_fn?

I am not sure if there are any other preliminary checks that can be done. It would have to be something that works uniformly for all functions.

I do not entirely understand the purpose of the checks you are proposing, or which issues they would resolve. To do the ABI check, the first step is to adjust find_mir_or_eval_fn and call_extra_fn in compiler/rustc_mir/src/interpret/machine.rs to also pass the rustc_target::spec::abi::Abi of the call to this function. This is the "ABI" that I am talking about in the issue. Then, in a separate PR on the Miri side, we can check that this ABI matches what we are expecting.

[unseddd]

I do not entirely understand the purpose of the checks you are proposing, or which issues they would resolve.

Yeah, glad I asked. Had the feeling I was going in the wrong direction 😄

To do the ABI check, the first step is to adjust find_mir_or_eval_fn and call_extra_fn in compiler/rustc_mir/src/interpret/machine.rs to also pass the rustc_target::spec::abi::Abi of the call to this function. This is the "ABI" that I am talking about in the issue. Then, in a separate PR on the Miri side, we can check that this ABI matches what we are expecting.

I'll get started on those changes, thanks for the help.