rust-lang / miri

An interpreter for Rust's mid-level intermediate representation
Apache License 2.0
4.65k stars 348 forks source link

Shims for vararg functions: check that we get the right number of "fixed" arguments #4013

Open RalfJung opened 3 weeks ago

RalfJung commented 3 weeks ago

Most Miri shims use check_shim to ensure they are called with the right ABI and right number of arguments. However, some shims emulate vararg functions. There, we currently separately call check_abi_and_shim_symbol_clash and then check_min_arg_count,however, that misses potential UB: when a function, like open, is declared with 2 fixed args followed by varargs, then it is crucial that the caller uses a signature that actually involves 2 fixed args followed by varargs. If someone were to, say, declare this function as

pub fn open(path: *const c_char, ...) -> ::c_int;

and then call it as open(path, flags), that is Undefined Behavior!

Similarly, non-vararg shims can actually currently be invoked with a vararg import, which should also be detected as UB.

Unfortunately, emulate_foreign_item is not even given enough information to detect this -- we are given a slice of args, but we don't learn how many of those were passed as fixed args vs varargs. So this requires changing the rustc side of this to pass more information to find_mir_or_eval_fn -- basically, we should pass down the full FnAbi.

tiif commented 3 weeks ago

This sounds interesting :)

@rustbot claim

RalfJung commented 3 weeks ago

basically, we should pass down the full FnAbi.

In fact we probably want to pass that down instead of the ExternAbi we pass currently. That will require changing our shim ABI compat checks a bit, but that's fine.

This sounds interesting :)

Great. :D The first part of this, about passing more things down to find_mir_or_eval_fn, will have to be a rustc PR.

tiif commented 1 week ago

In fact we probably want to pass that down instead of the ExternAbi we pass currently. That will require changing our shim ABI compat checks a bit, but that's fine.

Do we really want to remove passing ExternAbi in find_mir_or_eval_fn? It seems to be needed for check_abi_and_shim_symbol_clash. I can add a new function parameter passing down passing down FnAbi first while keeping ExternAbi .

RalfJung commented 1 week ago

It seems to be needed for check_abi_and_shim_symbol_clash

That should be changed to check the calling convention stored in FnAbi, instead of checking ExternAbi.