Skip to content

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

@RalfJung

Description

@RalfJung
Member

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.

Activity

added
C-bugCategory: This is a bug.
A-shimsArea: This affects the external function shims
E-good-second-issueA good issue to pick up if you've already seen some parts of Miri, mentoring is available
on Nov 5, 2024
tiif

tiif commented on Nov 5, 2024

@tiif
Member

This sounds interesting :)

@rustbot claim

RalfJung

RalfJung commented on Nov 5, 2024

@RalfJung
MemberAuthor

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

tiif commented on Nov 16, 2024

@tiif
Member

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

RalfJung commented on Nov 16, 2024

@RalfJung
MemberAuthor

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.

added 2 commits that reference this issue on Dec 17, 2024
b9e001f
d7f80f6
added a commit that references this issue on Dec 20, 2024
a53204f
added a commit that references this issue on Dec 20, 2024
added a commit that references this issue on Dec 20, 2024
tiif

tiif commented on Dec 27, 2024

@tiif
Member

This is currently the next thing on my queue, but I am on a break for a few days, so this probably won't be resumed until 2025 ^^.

added a commit that references this issue on Jan 7, 2025
tiif

tiif commented on Feb 1, 2025

@tiif
Member

Progress on this issue (mostly a reminder for myself):

Currently we already have detection for wrong fixed arg number and calling vararg shim with non-vararg import. The final step for this is detecting if non-vararg shim is invoked with vararg import. We probably can just add a check in check_shim.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

A-shimsArea: This affects the external function shimsC-bugCategory: This is a bug.E-good-second-issueA good issue to pick up if you've already seen some parts of Miri, mentoring is available

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

    Development

    Participants

    @RalfJung@tiif

    Issue actions

      Shims for vararg functions: check that we get the right number of "fixed" arguments · Issue #4013 · rust-lang/miri