Auto merge of #1393 - RalfJung:arg-count-cleanup, r=RalfJung

Make sure we check argument count everywhere and argument size for non-stub shims

Author: bors

src/shims/foreign_items/posix.rs

@@ -34,7 +34,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
                 this.write_scalar(Scalar::from_i32(result), dest)?;
             }
             "setenv" => {
-                let &[name, value, _overwrite] = check_arg_count(args)?;
+                let &[name, value, overwrite] = check_arg_count(args)?;
+                this.read_scalar(overwrite)?.to_i32()?;
                 let result = this.setenv(name, value)?;
                 this.write_scalar(Scalar::from_i32(result), dest)?;
             }
@@ -51,8 +52,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
 
             // File related shims
             "open" | "open64" => {
-                let &[path, flag, _mode] = check_arg_count(args)?;
-                let result = this.open(path, flag)?;
+                let &[path, flag, mode] = check_arg_count(args)?;
+                let result = this.open(path, flag, mode)?;
                 this.write_scalar(Scalar::from_i32(result), dest)?;
             }
             "fcntl" => {
@@ -379,19 +380,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
 
             // Incomplete shims that we "stub out" just to get pre-main initialization code to work.
             // These shims are enabled only when the caller is in the standard library.
-            | "pthread_attr_init"
-            | "pthread_attr_destroy"
-            | "pthread_attr_setstacksize"
-            | "pthread_condattr_init"
-            | "pthread_condattr_setclock"
-            | "pthread_cond_init"
-            | "pthread_condattr_destroy"
-            | "pthread_cond_destroy" if this.frame().instance.to_string().starts_with("std::sys::unix::")
-            => {
-                this.write_null(dest)?;
-            }
-            "pthread_attr_getguardsize" if this.frame().instance.to_string().starts_with("std::sys::unix::")
-            => {
+            "pthread_attr_getguardsize"
+            if this.frame().instance.to_string().starts_with("std::sys::unix::") => {
                 let &[_attr, guard_size] = check_arg_count(args)?;
                 let guard_size = this.deref_operand(guard_size)?;
                 let guard_size_layout = this.libc_ty_layout("size_t")?;
@@ -401,11 +391,33 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
                 this.write_null(dest)?;
             }
 
+            | "pthread_attr_init"
+            | "pthread_attr_destroy"
+            | "pthread_condattr_init"
+            | "pthread_condattr_destroy"
+            | "pthread_cond_destroy"
+            if this.frame().instance.to_string().starts_with("std::sys::unix::") => {
+                let &[_] = check_arg_count(args)?;
+                this.write_null(dest)?;
+            }
+            | "pthread_cond_init"
+            | "pthread_attr_setstacksize"
+            | "pthread_condattr_setclock"
+            if this.frame().instance.to_string().starts_with("std::sys::unix::") => {
+                let &[_, _] = check_arg_count(args)?;
+                this.write_null(dest)?;
+            }
+
             | "signal"
-            | "sigaction"
             | "sigaltstack"
-            | "mprotect" if this.frame().instance.to_string().starts_with("std::sys::unix::")
-            => {
+            if this.frame().instance.to_string().starts_with("std::sys::unix::") => {
+                let &[_, _] = check_arg_count(args)?;
+                this.write_null(dest)?;
+            }
+            | "sigaction"
+            | "mprotect"
+            if this.frame().instance.to_string().starts_with("std::sys::unix::") => {
+                let &[_, _, _] = check_arg_count(args)?;
                 this.write_null(dest)?;
             }
 

src/shims/foreign_items/windows.rs

@@ -63,7 +63,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
                 this.write_scalar(Scalar::from_machine_isize(which.into(), this), dest)?;
             }
             "WriteFile" => {
-                let &[handle, buf, n, written_ptr, _overlapped] = check_arg_count(args)?;
+                let &[handle, buf, n, written_ptr, overlapped] = check_arg_count(args)?;
+                this.read_scalar(overlapped)?.to_machine_usize(this)?; // this is a poiner, that we ignore
                 let handle = this.read_scalar(handle)?.to_machine_isize(this)?;
                 let buf = this.read_scalar(buf)?.not_undef()?;
                 let n = this.read_scalar(n)?.to_u32()?;
@@ -251,22 +252,19 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
                 // Just fake a HANDLE
                 this.write_scalar(Scalar::from_machine_isize(1, this), dest)?;
             }
-            "GetModuleHandleW" if this.frame().instance.to_string().starts_with("std::sys::windows::")
-            => {
+            "GetModuleHandleW" if this.frame().instance.to_string().starts_with("std::sys::windows::") => {
                 #[allow(non_snake_case)]
                 let &[_lpModuleName] = check_arg_count(args)?;
                 // Pretend this does not exist / nothing happened, by returning zero.
                 this.write_null(dest)?;
             }
-            "GetProcAddress" if this.frame().instance.to_string().starts_with("std::sys::windows::")
-            => {
+            "GetProcAddress" if this.frame().instance.to_string().starts_with("std::sys::windows::") => {
                 #[allow(non_snake_case)]
                 let &[_hModule, _lpProcName] = check_arg_count(args)?;
                 // Pretend this does not exist / nothing happened, by returning zero.
                 this.write_null(dest)?;
             }
-            "SetConsoleTextAttribute" if this.frame().instance.to_string().starts_with("std::sys::windows::")
-            => {
+            "SetConsoleTextAttribute" if this.frame().instance.to_string().starts_with("std::sys::windows::") => {
                 #[allow(non_snake_case)]
                 let &[_hConsoleOutput, _wAttribute] = check_arg_count(args)?;
                 // Pretend these does not exist / nothing happened, by returning zero.
@@ -281,17 +279,17 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
             | "InitializeCriticalSection"
             | "EnterCriticalSection"
             | "LeaveCriticalSection"
-            | "DeleteCriticalSection" if this.frame().instance.to_string().starts_with("std::sys::windows::")
-            => {
+            | "DeleteCriticalSection"
+            if this.frame().instance.to_string().starts_with("std::sys::windows::") => {
                 #[allow(non_snake_case)]
                 let &[_lpCriticalSection] = check_arg_count(args)?;
                 assert_eq!(this.get_total_thread_count()?, 1, "concurrency on Windows not supported");
                 // Nothing to do, not even a return value.
                 // (Windows locks are reentrant, and we have only 1 thread,
                 // so not doing any futher checks here is at least not incorrect.)
             }
-            "TryEnterCriticalSection" if this.frame().instance.to_string().starts_with("std::sys::windows::")
-            => {
+            "TryEnterCriticalSection"
+            if this.frame().instance.to_string().starts_with("std::sys::windows::") => {
                 #[allow(non_snake_case)]
                 let &[_lpCriticalSection] = check_arg_count(args)?;
                 assert_eq!(this.get_total_thread_count()?, 1, "concurrency on Windows not supported");

src/shims/fs.rs

@@ -238,13 +238,27 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         &mut self,
         path_op: OpTy<'tcx, Tag>,
         flag_op: OpTy<'tcx, Tag>,
+        mode_op: OpTy<'tcx, Tag>,
     ) -> InterpResult<'tcx, i32> {
         let this = self.eval_context_mut();
 
         this.check_no_isolation("open")?;
 
         let flag = this.read_scalar(flag_op)?.to_i32()?;
 
+        // Check mode (size depends on platform).
+        // FIXME: should we do something with the mode?
+        match this.tcx.sess.target.target.target_os.as_str() {
+            "macos" => {
+                // FIXME: I think `mode` should be `u16` on macOS, but see
+                // <https://github.com/rust-lang/rust/issues/71915>.
+                // For now, just don't check on macos.
+            }
+            _ => {
+                this.read_scalar(mode_op)?.to_u32()?;
+            }
+        };
+
         let mut options = OpenOptions::new();
 
         let o_rdonly = this.eval_libc_i32("O_RDONLY")?;
@@ -331,15 +345,15 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         if args.len() < 2 {
             throw_ub_format!("incorrect number of arguments for fcntl: got {}, expected at least 2", args.len());
         }
+        let fd = this.read_scalar(args[0])?.to_i32()?;
         let cmd = this.read_scalar(args[1])?.to_i32()?;
         // We only support getting the flags for a descriptor.
         if cmd == this.eval_libc_i32("F_GETFD")? {
             // Currently this is the only flag that `F_GETFD` returns. It is OK to just return the
             // `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 &[fd, _] = check_arg_count(args)?;
-            let fd = this.read_scalar(fd)?.to_i32()?;
+            let &[_, _] = check_arg_count(args)?;
             if this.machine.file_handler.handles.contains_key(&fd) {
                 Ok(this.eval_libc_i32("FD_CLOEXEC")?)
             } else {
@@ -352,8 +366,7 @@ 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 &[fd, _, start] = check_arg_count(args)?;
-            let fd = this.read_scalar(fd)?.to_i32()?;
+            let &[_, _, start] = check_arg_count(args)?;
             let start = this.read_scalar(start)?.to_i32()?;
             if fd < MIN_NORMAL_FILE_FD {
                 throw_unsup_format!("duplicating file descriptors for stdin, stdout, or stderr is not supported")

src/shims/intrinsics.rs

@@ -363,7 +363,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
             | "atomic_singlethreadfence_acqrel"
             | "atomic_singlethreadfence"
             => {
-                // we are inherently singlethreaded and singlecored, this is a nop
+                let &[] = check_arg_count(args)?;
+                // FIXME: this will become relevant once we try to detect data races.
             }
 
             _ if intrinsic_name.starts_with("atomic_xchg") => {