From 70858197e3ad199b31f9a45ce51750f06575e55d Mon Sep 17 00:00:00 2001 From: tiif Date: Sun, 5 Jan 2025 01:54:49 +0800 Subject: [PATCH 1/4] Add fixed args count check for `open` --- src/helpers.rs | 19 ++++++++++++++++++- src/shims/unix/foreign_items.rs | 1 + tests/fail-dep/libc/wrong_fixed_arg_count.rs | 0 3 files changed, 19 insertions(+), 1 deletion(-) create mode 100644 tests/fail-dep/libc/wrong_fixed_arg_count.rs diff --git a/src/helpers.rs b/src/helpers.rs index adfec33bea..25baa02012 100644 --- a/src/helpers.rs +++ b/src/helpers.rs @@ -920,6 +920,23 @@ 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 {}", + abi.fixed_count, + expected_count + ) + } + interp_ok(()) + } + fn frame_in_std(&self) -> bool { let this = self.eval_context_ref(); let frame = this.frame(); @@ -1175,7 +1192,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>], diff --git a/src/shims/unix/foreign_items.rs b/src/shims/unix/foreign_items.rs index f47a96b10f..0c0ae748d4 100644 --- a/src/shims/unix/foreign_items.rs +++ b/src/shims/unix/foreign_items.rs @@ -236,6 +236,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)?; let result = this.open(args)?; this.write_scalar(result, dest)?; } diff --git a/tests/fail-dep/libc/wrong_fixed_arg_count.rs b/tests/fail-dep/libc/wrong_fixed_arg_count.rs new file mode 100644 index 0000000000..e69de29bb2 From 3d062c1d5507f0e74e2aba466da76f24316aee0b Mon Sep 17 00:00:00 2001 From: tiif Date: Sun, 5 Jan 2025 01:55:06 +0800 Subject: [PATCH 2/4] Add test for fixed args count --- tests/fail-dep/libc/wrong_fixed_arg_count.rs | 14 ++++++++++++++ tests/fail-dep/libc/wrong_fixed_arg_count.stderr | 15 +++++++++++++++ 2 files changed, 29 insertions(+) create mode 100644 tests/fail-dep/libc/wrong_fixed_arg_count.stderr diff --git a/tests/fail-dep/libc/wrong_fixed_arg_count.rs b/tests/fail-dep/libc/wrong_fixed_arg_count.rs index e69de29bb2..fdb2e45a0b 100644 --- a/tests/fail-dep/libc/wrong_fixed_arg_count.rs +++ b/tests/fail-dep/libc/wrong_fixed_arg_count.rs @@ -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 +} diff --git a/tests/fail-dep/libc/wrong_fixed_arg_count.stderr b/tests/fail-dep/libc/wrong_fixed_arg_count.stderr new file mode 100644 index 0000000000..6947ed74c9 --- /dev/null +++ b/tests/fail-dep/libc/wrong_fixed_arg_count.stderr @@ -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 + From d1e50d618c942d299bac724d5c4e28c128ab4661 Mon Sep 17 00:00:00 2001 From: tiif Date: Sun, 5 Jan 2025 02:01:50 +0800 Subject: [PATCH 3/4] Add check for other variadic functions --- src/shims/unix/android/thread.rs | 1 + src/shims/unix/foreign_items.rs | 1 + src/shims/unix/linux_like/syscall.rs | 1 + 3 files changed, 3 insertions(+) diff --git a/src/shims/unix/android/thread.rs b/src/shims/unix/android/thread.rs index 8d5d4a52b6..bdcc97bb85 100644 --- a/src/shims/unix/android/thread.rs +++ b/src/shims/unix/android/thread.rs @@ -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; diff --git a/src/shims/unix/foreign_items.rs b/src/shims/unix/foreign_items.rs index 0c0ae748d4..a868e651e6 100644 --- a/src/shims/unix/foreign_items.rs +++ b/src/shims/unix/foreign_items.rs @@ -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)?; } diff --git a/src/shims/unix/linux_like/syscall.rs b/src/shims/unix/linux_like/syscall.rs index 5fb262e176..d74af2d8bb 100644 --- a/src/shims/unix/linux_like/syscall.rs +++ b/src/shims/unix/linux_like/syscall.rs @@ -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 From 4c643bf04ab2741b63132223bfe38c046d83c9ac Mon Sep 17 00:00:00 2001 From: tiif Date: Sun, 5 Jan 2025 02:43:50 +0800 Subject: [PATCH 4/4] Better format --- src/helpers.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/helpers.rs b/src/helpers.rs index 25baa02012..7080f0e5e2 100644 --- a/src/helpers.rs +++ b/src/helpers.rs @@ -929,9 +929,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { ) -> InterpResult<'tcx> { if abi.fixed_count != expected_count { throw_ub_format!( - "incorrect number of fixed args for `{name}`: got {}, expected {}", - abi.fixed_count, - expected_count + "incorrect number of fixed args for `{name}`: got {}, expected {expected_count}", + abi.fixed_count ) } interp_ok(())