mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Auto merge of #2058 - RalfJung:variadic, r=RalfJung

Browse Source 
For variadic functions, accept arbitrary trailing arguments

However, make sure that if we use argument N we check the size of all arguments before that, because otherwise this might not work properly depending on how varargs are implemented. This caught bugs in our futex tests. ;)

I couldn't find a good way to systematically ensure this, so it is just something we have to be on the look for during review. (This generally applies also for fixed-arg shims: we should check the size of each parameter.)

Also treat prctl like a variadic function, Cc `@saethlin.`
This commit is contained in:
bors 2022-04-08 13:08:08 +00:00
commit f3a98563df
7 changed files with 81 additions and 81 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

31
src/shims/posix/fs.rs
View File

@ -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

21
src/shims/posix/linux/foreign_items.rs
View File

@ -106,9 +106,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
// Threading
"prctl" => {
let &[ref option, ref arg2, ref arg3, ref arg4, ref arg5] =
this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?;
let result = this.prctl(option, arg2, arg3, arg4, arg5)?;
// prctl is variadic. (It is not documented like that in the manpage, but defined like that in the libc crate.)
this.check_abi_and_shim_symbol_clash(abi, Abi::C { unwind: false }, link_name)?;
let result = this.prctl(args)?;
this.write_scalar(Scalar::from_i32(result), dest)?;
}
"pthread_condattr_setclock" => {
@ -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))?;

40
src/shims/posix/linux/sync.rs
View File

@ -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,9 +18,9 @@ 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()
);
}
@ -27,12 +28,13 @@ pub fn futex<'tcx>(
// 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;

36
src/shims/posix/thread.rs
View File

@ -97,20 +97,27 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
this.write_scalar(Scalar::from_uint(thread_id.to_u32(), dest.layout.size), dest)
}
fn prctl(
&mut self,
option: &OpTy<'tcx, Tag>,
arg2: &OpTy<'tcx, Tag>,
_arg3: &OpTy<'tcx, Tag>,
_arg4: &OpTy<'tcx, Tag>,
_arg5: &OpTy<'tcx, Tag>,
) -> InterpResult<'tcx, i32> {
fn prctl(&mut self, args: &[OpTy<'tcx, Tag>]) -> InterpResult<'tcx, i32> {
let this = self.eval_context_mut();
this.assert_target_os("linux", "prctl");
let option = this.read_scalar(option)?.to_i32()?;
if args.len() < 1 {
throw_ub_format!(
"incorrect number of arguments for `prctl`: got {}, expected at least 1",
args.len()
);
}
let option = this.read_scalar(&args[0])?.to_i32()?;
if option == this.eval_libc_i32("PR_SET_NAME")? {
let address = this.read_pointer(arg2)?;
if args.len() < 2 {
throw_ub_format!(
"incorrect number of arguments for `prctl` with `PR_SET_NAME`: got {}, expected at least 2",
args.len()
);
}
let address = this.read_pointer(&args[1])?;
let mut name = this.read_c_str(address)?.to_owned();
// The name should be no more than 16 bytes, including the null
// byte. Since `read_c_str` returns the string without the null
@ -118,7 +125,14 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
name.truncate(15);
this.set_active_thread_name(name);
} else if option == this.eval_libc_i32("PR_GET_NAME")? {
let address = this.read_pointer(arg2)?;
if args.len() < 2 {
throw_ub_format!(
"incorrect number of arguments for `prctl` with `PR_SET_NAME`: got {}, expected at least 2",
args.len()
);
}
let address = this.read_pointer(&args[1])?;
let mut name = this.get_active_thread_name().to_vec();
name.push(0u8);
assert!(name.len() <= 16);

2
tests/compile-fail/fs/unix_open_missing_required_mode.rs
View File

@ -12,5 +12,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` with `O_CREAT`: got 2, expected 3
let _fd = unsafe { libc::open(name_ptr, libc::O_CREAT) }; //~ ERROR Undefined Behavior: incorrect number of arguments for `open` with `O_CREAT`: got 2, expected at least 3
}

16
tests/compile-fail/fs/unix_open_too_many_args.rs
View File

@ -1,16 +0,0 @@
// ignore-windows: No libc on Windows
// compile-flags: -Zmiri-disable-isolation
#![feature(rustc_private)]
extern crate libc;
fn main() {
test_open_too_many_args();
}
fn test_open_too_many_args() {
let name = b"too_many_args.txt\0";
let name_ptr = name.as_ptr().cast::<libc::c_char>();
let _fd = unsafe { libc::open(name_ptr, libc::O_RDONLY, 0, 0) }; //~ ERROR Undefined Behavior: incorrect number of arguments for `open`: got 4, expected 2 or 3
}

16
tests/run-pass/concurrency/linux-futex.rs
View File

@ -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);
}