Skip to content

Commit

Permalink
for variadic functions, accept arbitrary trailing arguments but make …
Browse files Browse the repository at this point in the history 
…sure we check all leading arguments
  • Loading branch information
@RalfJung
RalfJung committed Apr 7, 2022
1 parent 8acc9b2 commit 59ee672
Show file tree
Hide file tree
Showing 4 changed files with 52 additions and 50 deletions.
31 changes: 17 additions & 14 deletions src/shims/posix/fs.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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()
);
};
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand All @@ -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;

Expand All @@ -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()?;
Expand Down Expand Up @@ -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)? {
Expand All @@ -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
Expand Down
15 changes: 4 additions & 11 deletions src/shims/posix/linux/foreign_items.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)?;

Expand Down Expand Up @@ -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))?;
Expand Down
40 changes: 23 additions & 17 deletions src/shims/posix/linux/sync.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>],
Expand All @@ -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")?;
Expand All @@ -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()
);
}
Expand All @@ -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 {
Expand Down Expand Up @@ -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.
Expand All @@ -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)?;
Expand All @@ -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
};
Expand All @@ -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;
Expand Down
16 changes: 8 additions & 8 deletions tests/run-pass/concurrency/linux-futex.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@ fn wake_nobody() {
&futex as *const i32,
libc::FUTEX_WAKE,
1,
0,
0,
ptr::null::<libc::timespec>(),
0usize,
0,
), 0);
}
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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.
}
Expand All @@ -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.
}
Expand All @@ -199,7 +199,7 @@ fn wait_wake_bitset() {
libc::FUTEX_WAIT_BITSET,
0,
ptr::null::<libc::timespec>(),
0,
0usize,
0b0100, // bitset
), 0);
}
Expand Down

0 comments on commit 59ee672

Please sign in to comment.