Skip to content
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

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

Open
RalfJung opened this issue Nov 5, 2024 · 4 comments
Assignees
Labels
A-shims Area: This affects the external function shims C-bug Category: This is a bug. E-good-second-issue A good issue to pick up if you've already seen some parts of Miri, mentoring is available

Comments

@RalfJung
Copy link
Member

RalfJung commented Nov 5, 2024

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.

@RalfJung RalfJung added C-bug Category: This is a bug. A-shims Area: This affects the external function shims E-good-second-issue A good issue to pick up if you've already seen some parts of Miri, mentoring is available labels Nov 5, 2024
@tiif
Copy link
Contributor

tiif commented Nov 5, 2024

This sounds interesting :)

@rustbot claim

@RalfJung
Copy link
Member Author

RalfJung commented Nov 5, 2024

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
Copy link
Contributor

tiif commented Nov 16, 2024

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
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-shims Area: This affects the external function shims C-bug Category: This is a bug. E-good-second-issue A good issue to pick up if you've already seen some parts of Miri, mentoring is available
Projects
None yet
Development

No branches or pull requests

2 participants