for variadic functions, accept arbitrary trailing arguments but make sure we check all leading arguments

RalfJung · RalfJung · commit 59ee672fef55 · 2022-04-07T16:19:00.000-04:00
diff --git a/src/shims/posix/fs.rs b/src/shims/posix/fs.rs
@@ -15,7 +15,6 @@ use rustc_middle::ty::{self, layout::LayoutOf};
 use rustc_target::abi::{Align, Size};
 
 use crate::*;
-use helpers::check_arg_count;
 use shims::os_str::os_str_to_bytes;
 use shims::time::system_time_to_duration;
 
@@ -479,16 +478,16 @@ fn maybe_sync_file(
 impl<'mir, 'tcx: 'mir> EvalContextExt<'mir, 'tcx> for crate::MiriEvalContext<'mir, 'tcx> {}
 pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx> {
     fn open(&mut self, args: &[OpTy<'tcx, Tag>]) -> InterpResult<'tcx, i32> {
-        if args.len() < 2 || args.len() > 3 {
+        if args.len() < 2 {
             throw_ub_format!(
-                "incorrect number of arguments for `open`: got {}, expected 2 or 3",
+                "incorrect number of arguments for `open`: got {}, expected at least 2",
                 args.len()
             );
         }
 
         let this = self.eval_context_mut();
 
-        let path_op = &args[0];
+        let path = this.read_pointer(&args[0])?;
         let flag = this.read_scalar(&args[1])?.to_i32()?;
 
         let mut options = OpenOptions::new();
@@ -541,7 +540,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
                 this.read_scalar(arg)?.to_u32()?
             } else {
                 throw_ub_format!(
-                    "incorrect number of arguments for `open` with `O_CREAT`: got {}, expected 3",
+                    "incorrect number of arguments for `open` with `O_CREAT`: got {}, expected at least 3",
                     args.len()
                 );
             };
@@ -572,7 +571,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
             throw_unsup_format!("unsupported flags {:#x}", flag & !mirror);
         }
 
-        let path = this.read_path_from_c_str(this.read_pointer(path_op)?)?;
+        let path = this.read_path_from_c_str(path)?;
 
         // Reject if isolation is enabled.
         if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op {
@@ -614,7 +613,6 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
             // `FD_CLOEXEC` value without checking if the flag is set for the file because `std`
             // always sets this flag when opening a file. However we still need to check that the
             // file itself is open.
-            let &[_, _] = check_arg_count(args)?;
             if this.machine.file_handler.handles.contains_key(&fd) {
                 Ok(this.eval_libc_i32("FD_CLOEXEC")?)
             } else {
@@ -627,8 +625,13 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
             // because exec() isn't supported. The F_DUPFD and F_DUPFD_CLOEXEC commands only
             // differ in whether the FD_CLOEXEC flag is pre-set on the new file descriptor,
             // thus they can share the same implementation here.
-            let &[_, _, ref start] = check_arg_count(args)?;
-            let start = this.read_scalar(start)?.to_i32()?;
+            if args.len() < 3 {
+                throw_ub_format!(
+                    "incorrect number of arguments for fcntl with cmd=`F_DUPFD`/`F_DUPFD_CLOEXEC`: got {}, expected at least 3",
+                    args.len()
+                );
+            }
+            let start = this.read_scalar(&args[2])?.to_i32()?;
 
             let fh = &mut this.machine.file_handler;
 
@@ -646,7 +649,6 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
                 None => return this.handle_not_found(),
             }
         } else if this.tcx.sess.target.os == "macos" && cmd == this.eval_libc_i32("F_FULLFSYNC")? {
-            let &[_, _] = check_arg_count(args)?;
             if let Some(file_descriptor) = this.machine.file_handler.handles.get(&fd) {
                 // FIXME: Support fullfsync for all FDs
                 let FileHandle { file, writable } = file_descriptor.as_file_handle()?;
@@ -919,15 +921,18 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         dirfd_op: &OpTy<'tcx, Tag>,    // Should be an `int`
         pathname_op: &OpTy<'tcx, Tag>, // Should be a `const char *`
         flags_op: &OpTy<'tcx, Tag>,    // Should be an `int`
-        _mask_op: &OpTy<'tcx, Tag>,    // Should be an `unsigned int`
+        mask_op: &OpTy<'tcx, Tag>,    // Should be an `unsigned int`
         statxbuf_op: &OpTy<'tcx, Tag>, // Should be a `struct statx *`
     ) -> InterpResult<'tcx, i32> {
         let this = self.eval_context_mut();
 
         this.assert_target_os("linux", "statx");
 
-        let statxbuf_ptr = this.read_pointer(statxbuf_op)?;
+        let dirfd = this.read_scalar(dirfd_op)?.to_i32()?;
         let pathname_ptr = this.read_pointer(pathname_op)?;
+        let flags = this.read_scalar(flags_op)?.to_i32()?;
+        let _mask = this.read_scalar(mask_op)?.to_u32()?;
+        let statxbuf_ptr = this.read_pointer(statxbuf_op)?;
 
         // If the statxbuf or pathname pointers are null, the function fails with `EFAULT`.
         if this.ptr_is_null(statxbuf_ptr)? || this.ptr_is_null(pathname_ptr)? {
@@ -953,9 +958,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
 
         let path = this.read_path_from_c_str(pathname_ptr)?.into_owned();
         // See <https://github.com/rust-lang/rust/pull/79196> for a discussion of argument sizes.
-        let flags = this.read_scalar(flags_op)?.to_i32()?;
         let empty_path_flag = flags & this.eval_libc("AT_EMPTY_PATH")?.to_i32()? != 0;
-        let dirfd = this.read_scalar(dirfd_op)?.to_i32()?;
         // We only support:
         // * interpreting `path` as an absolute directory,
         // * interpreting `path` as a path relative to `dirfd` when the latter is `AT_FDCWD`, or
diff --git a/src/shims/posix/linux/foreign_items.rs b/src/shims/posix/linux/foreign_items.rs
@@ -130,16 +130,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
                 // count is checked bellow.
                 this.check_abi_and_shim_symbol_clash(abi, Abi::C { unwind: false }, link_name)?;
                 // The syscall variadic function is legal to call with more arguments than needed,
-                // extra arguments are simply ignored. However, all arguments need to be scalars;
-                // other types might be treated differently by the calling convention.
-                for arg in args {
-                    if !matches!(arg.layout.abi, rustc_target::abi::Abi::Scalar(_)) {
-                        throw_ub_format!(
-                            "`syscall` arguments must all have scalar layout, but {} does not",
-                            arg.layout.ty
-                        );
-                    }
-                }
+                // 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
+                // have the right type.
 
                 let sys_getrandom = this.eval_libc("SYS_getrandom")?.to_machine_usize(this)?;
 
@@ -181,7 +174,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
                     }
                     // `futex` is used by some synchonization primitives.
                     id if id == sys_futex => {
-                        futex(this, args, dest)?;
+                        futex(this, &args[1..], dest)?;
                     }
                     id => {
                         this.handle_unsupported(format!("can't execute syscall with ID {}", id))?;
diff --git a/src/shims/posix/linux/sync.rs b/src/shims/posix/linux/sync.rs
@@ -4,6 +4,7 @@ use rustc_target::abi::{Align, Size};
 use std::time::{Instant, SystemTime};
 
 /// Implementation of the SYS_futex syscall.
+/// `args` is the arguments *after* the syscall number.
 pub fn futex<'tcx>(
     this: &mut MiriEvalContext<'_, 'tcx>,
     args: &[OpTy<'tcx, Tag>],
@@ -17,22 +18,23 @@ 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.
-    if args.len() < 4 {
+    if args.len() < 3 {
         throw_ub_format!(
-            "incorrect number of arguments for `futex` syscall: got {}, expected at least 4",
+            "incorrect number of arguments for `futex` syscall: got {}, expected at least 3",
             args.len()
         );
     }
 
     // The first three arguments (after the syscall number itself) are the same to all futex operations:
     //     (int *addr, int op, int val).
     // We checked above that these definitely exist.
-    let addr = this.read_immediate(&args[1])?;
-    let op = this.read_scalar(&args[2])?.to_i32()?;
-    let val = this.read_scalar(&args[3])?.to_i32()?;
+    let addr = this.read_immediate(&args[0])?;
+    let op = this.read_scalar(&args[1])?.to_i32()?;
+    let val = this.read_scalar(&args[2])?.to_i32()?;
 
     let thread = this.get_active_thread();
     let addr_scalar = addr.to_scalar()?;
+    let addr_usize = addr_scalar.to_machine_usize(this)?;
 
     let futex_private = this.eval_libc_i32("FUTEX_PRIVATE_FLAG")?;
     let futex_wait = this.eval_libc_i32("FUTEX_WAIT")?;
@@ -56,17 +58,19 @@ pub fn futex<'tcx>(
             let wait_bitset = op & !futex_realtime == futex_wait_bitset;
 
             let bitset = if wait_bitset {
-                if args.len() != 7 {
+                if args.len() < 6 {
                     throw_ub_format!(
-                        "incorrect number of arguments for `futex` syscall with `op=FUTEX_WAIT_BITSET`: got {}, expected 7",
+                        "incorrect number of arguments for `futex` syscall with `op=FUTEX_WAIT_BITSET`: got {}, expected at least 6",
                         args.len()
                     );
                 }
-                this.read_scalar(&args[6])?.to_u32()?
+                let _timeout = this.read_pointer(&args[3])?;
+                let _uaddr2 = this.read_pointer(&args[4])?;
+                this.read_scalar(&args[5])?.to_u32()?
             } else {
-                if args.len() < 5 {
+                if args.len() < 4 {
                     throw_ub_format!(
-                        "incorrect number of arguments for `futex` syscall with `op=FUTEX_WAIT`: got {}, expected at least 5",
+                        "incorrect number of arguments for `futex` syscall with `op=FUTEX_WAIT`: got {}, expected at least 4",
                         args.len()
                     );
                 }
@@ -81,7 +85,7 @@ pub fn futex<'tcx>(
             }
 
             // `deref_operand` but not actually dereferencing the ptr yet (it might be NULL!).
-            let timeout = this.ref_to_mplace(&this.read_immediate(&args[4])?)?;
+            let timeout = this.ref_to_mplace(&this.read_immediate(&args[3])?)?;
             let timeout_time = if this.ptr_is_null(timeout.ptr)? {
                 None
             } else {
@@ -145,7 +149,7 @@ pub fn futex<'tcx>(
             if val == futex_val {
                 // The value still matches, so we block the trait make it wait for FUTEX_WAKE.
                 this.block_thread(thread);
-                this.futex_wait(addr_scalar.to_machine_usize(this)?, thread, bitset);
+                this.futex_wait(addr_usize, thread, bitset);
                 // Succesfully waking up from FUTEX_WAIT always returns zero.
                 this.write_scalar(Scalar::from_machine_isize(0, this), dest)?;
                 // Register a timeout callback if a timeout was specified.
@@ -157,7 +161,7 @@ pub fn futex<'tcx>(
                         timeout_time,
                         Box::new(move |this| {
                             this.unblock_thread(thread);
-                            this.futex_remove_waiter(addr_scalar.to_machine_usize(this)?, thread);
+                            this.futex_remove_waiter(addr_usize, thread);
                             let etimedout = this.eval_libc("ETIMEDOUT")?;
                             this.set_last_error(etimedout)?;
                             this.write_scalar(Scalar::from_machine_isize(-1, this), &dest)?;
@@ -181,13 +185,15 @@ pub fn futex<'tcx>(
         // Same as FUTEX_WAKE, but allows you to specify a bitset to select which threads to wake up.
         op if op == futex_wake || op == futex_wake_bitset => {
             let bitset = if op == futex_wake_bitset {
-                if args.len() != 7 {
+                if args.len() < 6 {
                     throw_ub_format!(
-                        "incorrect number of arguments for `futex` syscall with `op=FUTEX_WAKE_BITSET`: got {}, expected 7",
+                        "incorrect number of arguments for `futex` syscall with `op=FUTEX_WAKE_BITSET`: got {}, expected at least 6",
                         args.len()
                     );
                 }
-                this.read_scalar(&args[6])?.to_u32()?
+                let _timeout = this.read_pointer(&args[3])?;
+                let _uaddr2 = this.read_pointer(&args[4])?;
+                this.read_scalar(&args[5])?.to_u32()?
             } else {
                 u32::MAX
             };
@@ -199,7 +205,7 @@ pub fn futex<'tcx>(
             }
             let mut n = 0;
             for _ in 0..val {
-                if let Some(thread) = this.futex_wake(addr_scalar.to_machine_usize(this)?, bitset) {
+                if let Some(thread) = this.futex_wake(addr_usize, bitset) {
                     this.unblock_thread(thread);
                     this.unregister_timeout_callback_if_exists(thread);
                     n += 1;
diff --git a/tests/run-pass/concurrency/linux-futex.rs b/tests/run-pass/concurrency/linux-futex.rs
@@ -32,8 +32,8 @@ fn wake_nobody() {
             &futex as *const i32,
             libc::FUTEX_WAKE,
             1,
-            0,
-            0,
+            ptr::null::<libc::timespec>(),
+            0usize,
             0,
         ), 0);
     }
@@ -121,7 +121,7 @@ fn wait_absolute_timeout() {
             libc::FUTEX_WAIT_BITSET,
             123,
             &timeout,
-            0,
+            0usize,
             u32::MAX,
         ), -1);
         assert_eq!(*libc::__errno_location(), libc::ETIMEDOUT);
@@ -173,8 +173,8 @@ fn wait_wake_bitset() {
                 &FUTEX as *const i32,
                 libc::FUTEX_WAKE_BITSET,
                 10, // Wake up at most 10 threads.
-                0,
-                0,
+                ptr::null::<libc::timespec>(),
+                0usize,
                 0b1001, // bitset
             ), 0); // Didn't match any thread.
         }
@@ -185,8 +185,8 @@ fn wait_wake_bitset() {
                 &FUTEX as *const i32,
                 libc::FUTEX_WAKE_BITSET,
                 10, // Wake up at most 10 threads.
-                0,
-                0,
+                ptr::null::<libc::timespec>(),
+                0usize,
                 0b0110, // bitset
             ), 1); // Woken up one thread.
         }
@@ -199,7 +199,7 @@ fn wait_wake_bitset() {
             libc::FUTEX_WAIT_BITSET,
             0,
             ptr::null::<libc::timespec>(),
-            0,
+            0usize,
             0b0100, // bitset
         ), 0);
     }
