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

Check fixed args number for variadic function #4122

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion src/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -920,6 +920,22 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
interp_ok(())
}

/// Check the number of fixed args.
fn check_fixed_args_count<'a>(
&self,
name: &'a str,
abi: &FnAbi<'tcx, Ty<'tcx>>,
expected_count: u32,
) -> InterpResult<'tcx> {
if abi.fixed_count != expected_count {
throw_ub_format!(
"incorrect number of fixed args for `{name}`: got {}, expected {expected_count}",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"incorrect number of fixed args for `{name}`: got {}, expected {expected_count}",
"incorrect number of fixed arguments for variadic function `{name}`: got {}, expected {expected_count}",

abi.fixed_count
)
}
interp_ok(())
}

fn frame_in_std(&self) -> bool {
let this = self.eval_context_ref();
let frame = this.frame();
Expand Down Expand Up @@ -1175,7 +1191,7 @@ where
throw_ub_format!("incorrect number of arguments: got {}, expected {}", args.len(), N)
}

/// Check that the number of args is at least the minumim what we expect.
/// Check that the number of args is at least the minimum what we expect.
pub fn check_min_arg_count<'a, 'tcx, const N: usize>(
name: &'a str,
args: &'a [OpTy<'tcx>],
Expand Down
1 change: 1 addition & 0 deletions src/shims/unix/android/thread.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ pub fn prctl<'tcx>(
// We do not use `check_shim` here because `prctl` is variadic. The argument
// count is checked bellow.
ecx.check_abi_and_shim_symbol_clash(abi, Conv::C, link_name)?;
ecx.check_fixed_args_count("prctl", abi, 1)?;

// FIXME: Use constants once https://github.com/rust-lang/libc/pull/3941 backported to the 0.2 branch.
let pr_set_name = 15;
Expand Down
2 changes: 2 additions & 0 deletions src/shims/unix/foreign_items.rs
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
// `fcntl` is variadic. The argument count is checked based on the first argument
// in `this.fcntl()`, so we do not use `check_shim` here.
this.check_abi_and_shim_symbol_clash(abi, Conv::C , link_name)?;
this.check_fixed_args_count("fcntl", abi, 2)?;
let result = this.fcntl(args)?;
this.write_scalar(result, dest)?;
}
Expand Down Expand Up @@ -236,6 +237,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
"open" | "open64" => {
// `open` is variadic, the third argument is only present when the second argument has O_CREAT (or on linux O_TMPFILE, but miri doesn't support that) set
this.check_abi_and_shim_symbol_clash(abi, Conv::C , link_name)?;
this.check_fixed_args_count("open/open64", abi, 2)?;
Copy link
Member

@RalfJung RalfJung Jan 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now it's still fairly easy to forget to call check_fixed_args_count.

So instead, please add a new this.check_shim_variadic that is a lot like check_shim but returns something like (&'a [OpTy<'tcx>; N], &'a [OpTy<'tcx>]) where the "fixed" arguments are mapped to the array and the variadic ones are returned in the slice.

When this is all done, check_abi_and_shim_symbol_clash should never be called in a shim; each shim should call check_shim or check_shim_variadic.

let result = this.open(args)?;
this.write_scalar(result, dest)?;
}
Expand Down
1 change: 1 addition & 0 deletions src/shims/unix/linux_like/syscall.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ pub fn syscall<'tcx>(
// We do not use `check_shim` here because `syscall` is variadic. The argument
// count is checked bellow.
ecx.check_abi_and_shim_symbol_clash(abi, Conv::C, link_name)?;
ecx.check_fixed_args_count("syscall", abi, 1)?;
// The syscall variadic function is legal to call with more arguments than needed,
// extra arguments are simply ignored. The important check is that when we use an
// argument, we have to also check all arguments *before* it to ensure that they
Expand Down
14 changes: 14 additions & 0 deletions tests/fail-dep/libc/wrong_fixed_arg_count.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
//@ignore-target: windows # File handling is not implemented yet
//@compile-flags: -Zmiri-disable-isolation
use std::ffi::{CString, OsStr};
use std::os::unix::ffi::OsStrExt;

extern "C" {
fn open(path: *const libc::c_char, ...) -> libc::c_int;
}

fn main() {
let c_path = CString::new(OsStr::new("./text").as_bytes()).expect("CString::new failed");
let _fd = unsafe { open(c_path.as_ptr(), libc::O_RDWR) };
//~^ ERROR: incorrect number of fixed args
}
15 changes: 15 additions & 0 deletions tests/fail-dep/libc/wrong_fixed_arg_count.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
error: Undefined Behavior: incorrect number of fixed args for `open/open64`: got 1, expected 2
--> tests/fail-dep/libc/wrong_fixed_arg_count.rs:LL:CC
|
LL | let _fd = unsafe { open(c_path.as_ptr(), libc::O_RDWR) };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ incorrect number of fixed args for `open/open64`: got 1, expected 2
|
= help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
= help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
= note: BACKTRACE:
= note: inside `main` at tests/fail-dep/libc/wrong_fixed_arg_count.rs:LL:CC

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

error: aborting due to 1 previous error

Loading