Merge pull request rust-lang#4122 from tiif/fnabi

Check fixed args number for variadic function

Author: RalfJung

src/tools/miri/src/helpers.rs

@@ -1002,6 +1002,22 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         check_arg_count(args)
     }
 
+    /// Check shim for variadic function.
+    /// Returns a tuple that consisting of an array of fixed args, and a slice of varargs.
+    fn check_shim_variadic<'a, const N: usize>(
+        &mut self,
+        abi: &FnAbi<'tcx, Ty<'tcx>>,
+        exp_abi: Conv,
+        link_name: Symbol,
+        args: &'a [OpTy<'tcx>],
+    ) -> InterpResult<'tcx, (&'a [OpTy<'tcx>; N], &'a [OpTy<'tcx>])>
+    where
+        &'a [OpTy<'tcx>; N]: TryFrom<&'a [OpTy<'tcx>]>,
+    {
+        self.check_abi_and_shim_symbol_clash(abi, exp_abi, link_name)?;
+        check_vargarg_fixed_arg_count(link_name, abi, args)
+    }
+
     /// Mark a machine allocation that was just created as immutable.
     fn mark_immutable(&mut self, mplace: &MPlaceTy<'tcx>) {
         let this = self.eval_context_mut();
@@ -1195,7 +1211,8 @@ 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.
+/// FIXME: Remove this function, use varargs and `check_min_vararg_count` instead.
 pub fn check_min_arg_count<'a, 'tcx, const N: usize>(
     name: &'a str,
     args: &'a [OpTy<'tcx>],
@@ -1210,6 +1227,51 @@ pub fn check_min_arg_count<'a, 'tcx, const N: usize>(
     )
 }
 
+/// Check that the number of varargs is at least the minimum what we expect.
+/// Fixed args should not be included.
+/// Use `check_vararg_fixed_arg_count` to extract the varargs slice from full function arguments.
+pub fn check_min_vararg_count<'a, 'tcx, const N: usize>(
+    name: &'a str,
+    args: &'a [OpTy<'tcx>],
+) -> InterpResult<'tcx, &'a [OpTy<'tcx>; N]> {
+    if let Some((ops, _)) = args.split_first_chunk() {
+        return interp_ok(ops);
+    }
+    throw_ub_format!(
+        "not enough variadic arguments for `{name}`: got {}, expected at least {}",
+        args.len(),
+        N
+    )
+}
+
+/// Check the number of fixed args of a vararg function.
+/// Returns a tuple that consisting of an array of fixed args, and a slice of varargs.
+fn check_vargarg_fixed_arg_count<'a, 'tcx, const N: usize>(
+    link_name: Symbol,
+    abi: &FnAbi<'tcx, Ty<'tcx>>,
+    args: &'a [OpTy<'tcx>],
+) -> InterpResult<'tcx, (&'a [OpTy<'tcx>; N], &'a [OpTy<'tcx>])> {
+    if !abi.c_variadic {
+        throw_ub_format!("calling a variadic function with a non-variadic caller-side signature");
+    }
+    if abi.fixed_count != u32::try_from(N).unwrap() {
+        throw_ub_format!(
+            "incorrect number of fixed arguments for variadic function `{}`: got {}, expected {N}",
+            link_name.as_str(),
+            abi.fixed_count
+        )
+    }
+    if let Some(args) = args.split_first_chunk() {
+        return interp_ok(args);
+    }
+    throw_ub_format!(
+        "incorrect number of arguments for `{}`: got {}, expected at least {}",
+        link_name.as_str(),
+        args.len(),
+        N
+    )
+}
+
 pub fn isolation_abort_error<'tcx>(name: &str) -> InterpResult<'tcx> {
     throw_machine_stop!(TerminationInfo::UnsupportedInIsolation(format!(
         "{name} not available when isolation is enabled",

src/tools/miri/src/shims/unix/android/thread.rs

@@ -3,7 +3,7 @@ use rustc_middle::ty::Ty;
 use rustc_span::Symbol;
 use rustc_target::callconv::{Conv, FnAbi};
 
-use crate::helpers::check_min_arg_count;
+use crate::helpers::check_min_vararg_count;
 use crate::shims::unix::thread::{EvalContextExt as _, ThreadNameResult};
 use crate::*;
 
@@ -16,18 +16,15 @@ pub fn prctl<'tcx>(
     args: &[OpTy<'tcx>],
     dest: &MPlaceTy<'tcx>,
 ) -> InterpResult<'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)?;
+    let ([op], varargs) = ecx.check_shim_variadic(abi, Conv::C, link_name, args)?;
 
     // FIXME: Use constants once https://github.com/rust-lang/libc/pull/3941 backported to the 0.2 branch.
     let pr_set_name = 15;
     let pr_get_name = 16;
 
-    let [op] = check_min_arg_count("prctl", args)?;
     let res = match ecx.read_scalar(op)?.to_i32()? {
         op if op == pr_set_name => {
-            let [_, name] = check_min_arg_count("prctl(PR_SET_NAME, ...)", args)?;
+            let [name] = check_min_vararg_count("prctl(PR_SET_NAME, ...)", varargs)?;
             let name = ecx.read_scalar(name)?;
             let thread = ecx.pthread_self()?;
             // The Linux kernel silently truncates long names.
@@ -38,7 +35,7 @@ pub fn prctl<'tcx>(
             Scalar::from_u32(0)
         }
         op if op == pr_get_name => {
-            let [_, name] = check_min_arg_count("prctl(PR_GET_NAME, ...)", args)?;
+            let [name] = check_min_vararg_count("prctl(PR_GET_NAME, ...)", varargs)?;
             let name = ecx.read_scalar(name)?;
             let thread = ecx.pthread_self()?;
             let len = Scalar::from_target_usize(TASK_COMM_LEN as u64, ecx);

src/tools/miri/src/shims/unix/fd.rs

@@ -6,7 +6,7 @@ use std::io::ErrorKind;
 
 use rustc_abi::Size;
 
-use crate::helpers::check_min_arg_count;
+use crate::helpers::check_min_vararg_count;
 use crate::shims::files::FileDescription;
 use crate::shims::unix::linux_like::epoll::EpollReadyEvents;
 use crate::shims::unix::*;
@@ -127,11 +127,14 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         interp_ok(Scalar::from_i32(this.try_unwrap_io_result(result)?))
     }
 
-    fn fcntl(&mut self, args: &[OpTy<'tcx>]) -> InterpResult<'tcx, Scalar> {
+    fn fcntl(
+        &mut self,
+        fd_num: &OpTy<'tcx>,
+        cmd: &OpTy<'tcx>,
+        varargs: &[OpTy<'tcx>],
+    ) -> InterpResult<'tcx, Scalar> {
         let this = self.eval_context_mut();
 
-        let [fd_num, cmd] = check_min_arg_count("fcntl", args)?;
-
         let fd_num = this.read_scalar(fd_num)?.to_i32()?;
         let cmd = this.read_scalar(cmd)?.to_i32()?;
 
@@ -163,7 +166,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                     "fcntl(fd, F_DUPFD_CLOEXEC, ...)"
                 };
 
-                let [_, _, start] = check_min_arg_count(cmd_name, args)?;
+                let [start] = check_min_vararg_count(cmd_name, varargs)?;
                 let start = this.read_scalar(start)?.to_i32()?;
 
                 if let Some(fd) = this.machine.fds.get(fd_num) {

src/tools/miri/src/shims/unix/foreign_items.rs

@@ -205,10 +205,9 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                 this.write_scalar(result, dest)?;
             }
             "fcntl" => {
-                // `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)?;
-                let result = this.fcntl(args)?;
+                let ([fd_num, cmd], varargs) =
+                    this.check_shim_variadic(abi, Conv::C, link_name, args)?;
+                let result = this.fcntl(fd_num, cmd, varargs)?;
                 this.write_scalar(result, dest)?;
             }
             "dup" => {
@@ -236,8 +235,9 @@ 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)?;
-                let result = this.open(args)?;
+                let ([path_raw, flag], varargs) =
+                    this.check_shim_variadic(abi, Conv::C, link_name, args)?;
+                let result = this.open(path_raw, flag, varargs)?;
                 this.write_scalar(result, dest)?;
             }
             "unlink" => {

src/tools/miri/src/shims/unix/fs.rs

@@ -13,7 +13,7 @@ use rustc_abi::Size;
 use rustc_data_structures::fx::FxHashMap;
 
 use self::shims::time::system_time_to_duration;
-use crate::helpers::check_min_arg_count;
+use crate::helpers::check_min_vararg_count;
 use crate::shims::files::{EvalContextExt as _, FileDescription, FileDescriptionRef};
 use crate::shims::os_str::bytes_to_os_str;
 use crate::shims::unix::fd::{FlockOp, UnixFileDescription};
@@ -452,9 +452,12 @@ fn maybe_sync_file(
 
 impl<'tcx> EvalContextExt<'tcx> for crate::MiriInterpCx<'tcx> {}
 pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
-    fn open(&mut self, args: &[OpTy<'tcx>]) -> InterpResult<'tcx, Scalar> {
-        let [path_raw, flag] = check_min_arg_count("open", args)?;
-
+    fn open(
+        &mut self,
+        path_raw: &OpTy<'tcx>,
+        flag: &OpTy<'tcx>,
+        varargs: &[OpTy<'tcx>],
+    ) -> InterpResult<'tcx, Scalar> {
         let this = self.eval_context_mut();
 
         let path_raw = this.read_pointer(path_raw)?;
@@ -507,7 +510,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
             // Get the mode.  On macOS, the argument type `mode_t` is actually `u16`, but
             // C integer promotion rules mean that on the ABI level, it gets passed as `u32`
             // (see https://github.com/rust-lang/rust/issues/71915).
-            let [_, _, mode] = check_min_arg_count("open(pathname, O_CREAT, ...)", args)?;
+            let [mode] = check_min_vararg_count("open(pathname, O_CREAT, ...)", varargs)?;
             let mode = this.read_scalar(mode)?.to_u32()?;
 
             #[cfg(unix)]

src/tools/miri/src/shims/unix/linux_like/sync.rs

@@ -1,5 +1,5 @@
 use crate::concurrency::sync::FutexRef;
-use crate::helpers::check_min_arg_count;
+use crate::helpers::check_min_vararg_count;
 use crate::*;
 
 struct LinuxFutex {
@@ -10,7 +10,7 @@ struct LinuxFutex {
 /// `args` is the arguments *including* the syscall number.
 pub fn futex<'tcx>(
     ecx: &mut MiriInterpCx<'tcx>,
-    args: &[OpTy<'tcx>],
+    varargs: &[OpTy<'tcx>],
     dest: &MPlaceTy<'tcx>,
 ) -> InterpResult<'tcx> {
     // The amount of arguments used depends on the type of futex operation.
@@ -21,7 +21,7 @@ pub fn futex<'tcx>(
     // may or may not be left out from the `syscall()` call.
     // Therefore we don't use `check_arg_count` here, but only check for the
     // number of arguments to fall within a range.
-    let [_, addr, op, val] = check_min_arg_count("`syscall(SYS_futex, ...)`", args)?;
+    let [addr, op, val] = check_min_vararg_count("`syscall(SYS_futex, ...)`", varargs)?;
 
     // The first three arguments (after the syscall number itself) are the same to all futex operations:
     //     (int *addr, int op, int val).
@@ -55,14 +55,16 @@ pub fn futex<'tcx>(
             let wait_bitset = op & !futex_realtime == futex_wait_bitset;
 
             let (timeout, bitset) = if wait_bitset {
-                let [_, _, _, _, timeout, uaddr2, bitset] =
-                    check_min_arg_count("`syscall(SYS_futex, FUTEX_WAIT_BITSET, ...)`", args)?;
+                let [_, _, _, timeout, uaddr2, bitset] = check_min_vararg_count(
+                    "`syscall(SYS_futex, FUTEX_WAIT_BITSET, ...)`",
+                    varargs,
+                )?;
                 let _timeout = ecx.read_pointer(timeout)?;
                 let _uaddr2 = ecx.read_pointer(uaddr2)?;
                 (timeout, ecx.read_scalar(bitset)?.to_u32()?)
             } else {
-                let [_, _, _, _, timeout] =
-                    check_min_arg_count("`syscall(SYS_futex, FUTEX_WAIT, ...)`", args)?;
+                let [_, _, _, timeout] =
+                    check_min_vararg_count("`syscall(SYS_futex, FUTEX_WAIT, ...)`", varargs)?;
                 (timeout, u32::MAX)
             };
 
@@ -190,8 +192,10 @@ pub fn futex<'tcx>(
             let futex_ref = futex_ref.futex.clone();
 
             let bitset = if op == futex_wake_bitset {
-                let [_, _, _, _, timeout, uaddr2, bitset] =
-                    check_min_arg_count("`syscall(SYS_futex, FUTEX_WAKE_BITSET, ...)`", args)?;
+                let [_, _, _, timeout, uaddr2, bitset] = check_min_vararg_count(
+                    "`syscall(SYS_futex, FUTEX_WAKE_BITSET, ...)`",
+                    varargs,
+                )?;
                 let _timeout = ecx.read_pointer(timeout)?;
                 let _uaddr2 = ecx.read_pointer(uaddr2)?;
                 ecx.read_scalar(bitset)?.to_u32()?

src/tools/miri/src/shims/unix/linux_like/syscall.rs

@@ -2,7 +2,7 @@ use rustc_middle::ty::Ty;
 use rustc_span::Symbol;
 use rustc_target::callconv::{Conv, FnAbi};
 
-use crate::helpers::check_min_arg_count;
+use crate::helpers::check_min_vararg_count;
 use crate::shims::unix::linux_like::eventfd::EvalContextExt as _;
 use crate::shims::unix::linux_like::sync::futex;
 use crate::*;
@@ -14,9 +14,7 @@ pub fn syscall<'tcx>(
     args: &[OpTy<'tcx>],
     dest: &MPlaceTy<'tcx>,
 ) -> InterpResult<'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)?;
+    let ([op], varargs) = ecx.check_shim_variadic(abi, Conv::C, link_name, args)?;
     // 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
@@ -26,14 +24,13 @@ pub fn syscall<'tcx>(
     let sys_futex = ecx.eval_libc("SYS_futex").to_target_usize(ecx)?;
     let sys_eventfd2 = ecx.eval_libc("SYS_eventfd2").to_target_usize(ecx)?;
 
-    let [op] = check_min_arg_count("syscall", args)?;
     match ecx.read_target_usize(op)? {
         // `libc::syscall(NR_GETRANDOM, buf.as_mut_ptr(), buf.len(), GRND_NONBLOCK)`
         // is called if a `HashMap` is created the regular way (e.g. HashMap<K, V>).
         num if num == sys_getrandom => {
             // Used by getrandom 0.1
             // The first argument is the syscall id, so skip over it.
-            let [_, ptr, len, flags] = check_min_arg_count("syscall(SYS_getrandom, ...)", args)?;
+            let [ptr, len, flags] = check_min_vararg_count("syscall(SYS_getrandom, ...)", varargs)?;
 
             let ptr = ecx.read_pointer(ptr)?;
             let len = ecx.read_target_usize(len)?;
@@ -47,10 +44,10 @@ pub fn syscall<'tcx>(
         }
         // `futex` is used by some synchronization primitives.
         num if num == sys_futex => {
-            futex(ecx, args, dest)?;
+            futex(ecx, varargs, dest)?;
         }
         num if num == sys_eventfd2 => {
-            let [_, initval, flags] = check_min_arg_count("syscall(SYS_evetfd2, ...)", args)?;
+            let [initval, flags] = check_min_vararg_count("syscall(SYS_evetfd2, ...)", varargs)?;
 
             let result = ecx.eventfd(initval, flags)?;
             ecx.write_int(result.to_i32()?, dest)?;

src/tools/miri/src/shims/unix/macos/foreign_items.rs

@@ -3,7 +3,6 @@ use rustc_span::Symbol;
 use rustc_target::callconv::{Conv, FnAbi};
 
 use super::sync::EvalContextExt as _;
-use crate::helpers::check_min_arg_count;
 use crate::shims::unix::*;
 use crate::*;
 
@@ -69,10 +68,9 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                 this.write_scalar(result, dest)?;
             }
             "ioctl" => {
-                // `ioctl` is variadic. The argument count is checked based on the first argument
-                // in `this.ioctl()`, so we do not use `check_shim` here.
-                this.check_abi_and_shim_symbol_clash(abi, Conv::C, link_name)?;
-                let result = this.ioctl(args)?;
+                let ([fd_num, cmd], varargs) =
+                    this.check_shim_variadic(abi, Conv::C, link_name, args)?;
+                let result = this.ioctl(fd_num, cmd, varargs)?;
                 this.write_scalar(result, dest)?;
             }
 
@@ -243,12 +241,16 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         interp_ok(EmulateItemResult::NeedsReturn)
     }
 
-    fn ioctl(&mut self, args: &[OpTy<'tcx>]) -> InterpResult<'tcx, Scalar> {
+    fn ioctl(
+        &mut self,
+        fd_num: &OpTy<'tcx>,
+        cmd: &OpTy<'tcx>,
+        _varargs: &[OpTy<'tcx>],
+    ) -> InterpResult<'tcx, Scalar> {
         let this = self.eval_context_mut();
 
         let fioclex = this.eval_libc_u64("FIOCLEX");
 
-        let [fd_num, cmd] = check_min_arg_count("ioctl", args)?;
         let fd_num = this.read_scalar(fd_num)?.to_i32()?;
         let cmd = this.read_scalar(cmd)?.to_u64()?;
 

src/tools/miri/test_dependencies/Cargo.toml

@@ -8,7 +8,7 @@ version = "0.1.0"
 edition = "2021"
 
 [dependencies]
-# all dependencies (and their transitive ones) listed here can be used in `tests/`.
+# all dependencies (and their transitive ones) listed here can be used in `tests/*-dep`.
 libc = "0.2"
 num_cpus = "1.10.1"
 cfg-if = "1"

src/tools/miri/tests/fail-dep/libc/fs/unix_open_missing_required_mode.rs

@@ -8,5 +8,5 @@ fn main() {
 fn test_file_open_missing_needed_mode() {
     let name = b"missing_arg.txt\0";
     let name_ptr = name.as_ptr().cast::<libc::c_char>();
-    let _fd = unsafe { libc::open(name_ptr, libc::O_CREAT) }; //~ ERROR: Undefined Behavior: incorrect number of arguments for `open(pathname, O_CREAT, ...)`: got 2, expected at least 3
+    let _fd = unsafe { libc::open(name_ptr, libc::O_CREAT) }; //~ ERROR: Undefined Behavior: not enough variadic arguments
 }

src/tools/miri/tests/fail-dep/libc/fs/unix_open_missing_required_mode.stderr

@@ -1,8 +1,8 @@
-error: Undefined Behavior: incorrect number of arguments for `open(pathname, O_CREAT, ...)`: got 2, expected at least 3
+error: Undefined Behavior: not enough variadic arguments for `open(pathname, O_CREAT, ...)`: got 0, expected at least 1
   --> tests/fail-dep/libc/fs/unix_open_missing_required_mode.rs:LL:CC
    |
-LL | ... { libc::open(name_ptr, libc::O_CREAT) };
-   |       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ incorrect number of arguments for `open(pathname, O_CREAT, ...)`: got 2, expected at least 3
+LL |     let _fd = unsafe { libc::open(name_ptr, libc::O_CREAT) };
+   |                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ not enough variadic arguments for `open(pathname, O_CREAT, ...)`: got 0, expected at least 1
    |
    = 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