From 4e3d1fee51a3a326c4146cb0986820ed129fce95 Mon Sep 17 00:00:00 2001 From: Chase Albert Date: Mon, 4 May 2020 14:24:22 -0400 Subject: [PATCH] Address comments. --- src/helpers.rs | 2 +- src/shims/foreign_items.rs | 1 + src/shims/foreign_items/posix/linux.rs | 3 ++- src/shims/foreign_items/posix/macos.rs | 5 +++-- src/shims/foreign_items/windows.rs | 8 ++++---- src/shims/fs.rs | 25 +++++++++---------------- src/shims/thread.rs | 4 ++-- 7 files changed, 22 insertions(+), 26 deletions(-) diff --git a/src/helpers.rs b/src/helpers.rs index 43a20d290c8..510efe46608 100644 --- a/src/helpers.rs +++ b/src/helpers.rs @@ -477,7 +477,7 @@ pub fn check_arg_count<'a, 'tcx, const N: usize>(args: &'a [OpTy<'tcx, Tag>]) -> if let Ok(ops) = args.try_into() { return Ok(ops); } - throw_ub_format!("incorrect number of arguments, got {}, needed {}", args.len(), N) + throw_ub_format!("incorrect number of arguments: got {}, expected {}", args.len(), N) } pub fn immty_from_int_checked<'tcx>( diff --git a/src/shims/foreign_items.rs b/src/shims/foreign_items.rs index 643f76bfe37..8a75fb03a53 100644 --- a/src/shims/foreign_items.rs +++ b/src/shims/foreign_items.rs @@ -454,6 +454,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx // Architecture-specific shims "llvm.x86.sse2.pause" if this.tcx.sess.target.target.arch == "x86" || this.tcx.sess.target.target.arch == "x86_64" => { + let &[] = check_arg_count(args)?; this.sched_yield()?; } diff --git a/src/shims/foreign_items/posix/linux.rs b/src/shims/foreign_items/posix/linux.rs index 3f9d1b259a8..e0f54cac157 100644 --- a/src/shims/foreign_items/posix/linux.rs +++ b/src/shims/foreign_items/posix/linux.rs @@ -102,7 +102,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx .to_machine_usize(this)?; if args.is_empty() { - throw_ub_format!("incorrect number of arguments for syscall, needed at least 1"); + throw_ub_format!("incorrect number of arguments for syscall: got 0, expected at least 1"); } match this.read_scalar(args[0])?.to_machine_usize(this)? { // `libc::syscall(NR_GETRANDOM, buf.as_mut_ptr(), buf.len(), GRND_NONBLOCK)` @@ -143,6 +143,7 @@ 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_getattr_np" if this.frame().instance.to_string().starts_with("std::sys::unix::") => { + let &[_thread, _attr] = check_arg_count(args)?; this.write_null(dest)?; } diff --git a/src/shims/foreign_items/posix/macos.rs b/src/shims/foreign_items/posix/macos.rs index 685f66d443d..1cfecbc9346 100644 --- a/src/shims/foreign_items/posix/macos.rs +++ b/src/shims/foreign_items/posix/macos.rs @@ -118,8 +118,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx // Threading "pthread_setname_np" => { - let ptr = this.read_scalar(args[0])?.not_undef()?; - this.pthread_setname_np(ptr)?; + let &[name] = check_arg_count(args)?; + let name = this.read_scalar(name)?.not_undef()?; + this.pthread_setname_np(name)?; } // Incomplete shims that we "stub out" just to get pre-main initialization code to work. diff --git a/src/shims/foreign_items/windows.rs b/src/shims/foreign_items/windows.rs index 3d7afc616e8..ab912476f6a 100644 --- a/src/shims/foreign_items/windows.rs +++ b/src/shims/foreign_items/windows.rs @@ -106,17 +106,17 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx this.write_scalar(res, dest)?; } "HeapFree" => { - let &[handle, _flags, ptr] = check_arg_count(args)?; + let &[handle, flags, ptr] = check_arg_count(args)?; this.read_scalar(handle)?.to_machine_isize(this)?; - let _flags = this.read_scalar(_flags)?.to_u32()?; + this.read_scalar(flags)?.to_u32()?; let ptr = this.read_scalar(ptr)?.not_undef()?; this.free(ptr, MiriMemoryKind::WinHeap)?; this.write_scalar(Scalar::from_i32(1), dest)?; } "HeapReAlloc" => { - let &[handle, _flags, ptr, size] = check_arg_count(args)?; + let &[handle, flags, ptr, size] = check_arg_count(args)?; this.read_scalar(handle)?.to_machine_isize(this)?; - let _flags = this.read_scalar(_flags)?.to_u32()?; + this.read_scalar(flags)?.to_u32()?; let ptr = this.read_scalar(ptr)?.not_undef()?; let size = this.read_scalar(size)?.to_machine_usize(this)?; let res = this.realloc(ptr, size, MiriMemoryKind::WinHeap)?; diff --git a/src/shims/fs.rs b/src/shims/fs.rs index b9bb0fadf1a..ecb906f158e 100644 --- a/src/shims/fs.rs +++ b/src/shims/fs.rs @@ -328,22 +328,18 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx this.check_no_isolation("fcntl")?; - let (fd, cmd, start) = if args.len() == 2 { - let &[fd, cmd] = check_arg_count(args)?; - (fd, cmd, None) - } else { - // If args.len() isn't 2 or 3 this will error appropriately. - let &[fd, cmd, start] = check_arg_count(args)?; - (fd, cmd, Some(start)) - }; - let fd = this.read_scalar(fd)?.to_i32()?; - let cmd = this.read_scalar(cmd)?.to_i32()?; + if args.len() < 2 { + throw_ub_format!("incorrect number of arguments for fcntl: got {}, expected at least 2", args.len()); + } + 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()?; if this.machine.file_handler.handles.contains_key(&fd) { Ok(this.eval_libc_i32("FD_CLOEXEC")?) } else { @@ -356,15 +352,12 @@ 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 = 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") } - let start = start.ok_or_else(|| { - err_unsup_format!( - "fcntl with command F_DUPFD or F_DUPFD_CLOEXEC requires a third argument" - ) - })?; - let start = this.read_scalar(start)?.to_i32()?; let fh = &mut this.machine.file_handler; let (file_result, writable) = match fh.handles.get(&fd) { Some(FileHandle { file, writable }) => (file.try_clone(), *writable), diff --git a/src/shims/thread.rs b/src/shims/thread.rs index ac1bb39a698..3ea1ee0aa17 100644 --- a/src/shims/thread.rs +++ b/src/shims/thread.rs @@ -121,12 +121,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx fn pthread_setname_np( &mut self, - ptr: Scalar, + name: Scalar, ) -> InterpResult<'tcx> { let this = self.eval_context_mut(); this.assert_target_os("macos", "pthread_setname_np"); - let name = this.memory.read_c_str(ptr)?.to_owned(); + let name = this.memory.read_c_str(name)?.to_owned(); this.set_active_thread_name(name)?; Ok(())