From 1ffc5bb5633dd9bf3a38766fbf78ddc76d8b0bc8 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Thu, 1 Oct 2020 20:47:31 +0200 Subject: [PATCH 01/23] Implement futex_wait and futex_wake. This does not support futex_wait with a timeout yet. --- src/shims/posix/linux/foreign_items.rs | 67 ++++++++++++++++++++++++++ src/sync.rs | 27 +++++++++++ 2 files changed, 94 insertions(+) diff --git a/src/shims/posix/linux/foreign_items.rs b/src/shims/posix/linux/foreign_items.rs index 357c55c926..8434d7bfa8 100644 --- a/src/shims/posix/linux/foreign_items.rs +++ b/src/shims/posix/linux/foreign_items.rs @@ -1,4 +1,5 @@ use rustc_middle::mir; +use rustc_target::abi::{Align, Size}; use crate::*; use crate::helpers::check_arg_count; @@ -120,6 +121,10 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx .eval_libc("SYS_statx")? .to_machine_usize(this)?; + let sys_futex = this + .eval_libc("SYS_futex")? + .to_machine_usize(this)?; + if args.is_empty() { throw_ub_format!("incorrect number of arguments for syscall: got 0, expected at least 1"); } @@ -139,6 +144,10 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let result = this.linux_statx(dirfd, pathname, flags, mask, statxbuf)?; this.write_scalar(Scalar::from_machine_isize(result.into(), this), dest)?; } + // `futex` is used by some synchonization primitives. + id if id == sys_futex => { + futex(this, args, dest)?; + } id => throw_unsup_format!("miri does not support syscall ID {}", id), } } @@ -192,3 +201,61 @@ fn getrandom<'tcx>( this.write_scalar(Scalar::from_machine_usize(len, this), dest)?; Ok(()) } + +fn futex<'tcx>( + this: &mut MiriEvalContext<'_, 'tcx>, + args: &[OpTy<'tcx, Tag>], + dest: PlaceTy<'tcx, Tag>, +) -> InterpResult<'tcx> { + if args.len() < 4 { + throw_ub_format!("incorrect number of arguments for futex syscall: got {}, expected at least 4", args.len()); + } + let addr = this.read_scalar(args[1])?.check_init()?; + let op = this.read_scalar(args[2])?.to_i32()?; + let val = this.read_scalar(args[3])?.to_i32()?; + + this.memory.check_ptr_access(addr, Size::from_bytes(4), Align::from_bytes(4).unwrap())?; + + let addr = addr.assert_ptr(); + + let thread = this.get_active_thread(); + + let futex_private = this.eval_libc_i32("FUTEX_PRIVATE_FLAG")?; + let futex_wait = this.eval_libc_i32("FUTEX_WAIT")?; + let futex_wake = this.eval_libc_i32("FUTEX_WAKE")?; + + match op & !futex_private { + op if op == futex_wait => { + if args.len() < 5 { + throw_ub_format!("incorrect number of arguments for FUTEX_WAIT syscall: got {}, expected at least 5", args.len()); + } + let timeout = this.read_scalar(args[4])?.check_init()?; + if !this.is_null(timeout)? { + throw_ub_format!("miri does not support timeouts for futex operations"); + } + let futex_val = this.read_scalar_at_offset(args[1], 0, this.machine.layouts.i32)?.to_i32()?; + if val == futex_val { + this.block_thread(thread); + this.futex_wait(addr, thread); + } else { + let eagain = this.eval_libc("EAGAIN")?; + this.set_last_error(eagain)?; + } + } + op if op == futex_wake => { + let mut n = 0; + for _ in 0..val { + if let Some(thread) = this.futex_wake(addr) { + this.unblock_thread(thread); + n += 1; + } else { + break; + } + } + this.write_scalar(Scalar::from_i32(n), dest)?; + } + op => throw_unsup_format!("miri does not support SYS_futex operation {}", op), + } + + Ok(()) +} diff --git a/src/sync.rs b/src/sync.rs index 7e3c27b386..d5594fb9ec 100644 --- a/src/sync.rs +++ b/src/sync.rs @@ -96,12 +96,26 @@ struct Condvar { waiters: VecDeque, } +/// The futex state. +#[derive(Default, Debug)] +struct Futex { + waiters: VecDeque, +} + +/// A thread waiting on a futex. +#[derive(Debug)] +struct FutexWaiter { + /// The thread that is waiting on this futex. + thread: ThreadId, +} + /// The state of all synchronization variables. #[derive(Default, Debug)] pub(super) struct SynchronizationState { mutexes: IndexVec, rwlocks: IndexVec, condvars: IndexVec, + futexes: HashMap, Futex>, } // Private extension trait for local helper methods @@ -403,4 +417,17 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let this = self.eval_context_mut(); this.machine.threads.sync.condvars[id].waiters.retain(|waiter| waiter.thread != thread); } + + fn futex_wait(&mut self, addr: Pointer, thread: ThreadId) { + let this = self.eval_context_mut(); + let waiters = &mut this.machine.threads.sync.futexes.entry(addr).or_default().waiters; + assert!(waiters.iter().all(|waiter| waiter.thread != thread), "thread is already waiting"); + waiters.push_back(FutexWaiter { thread }); + } + + fn futex_wake(&mut self, addr: Pointer) -> Option { + let this = self.eval_context_mut(); + let waiters = &mut this.machine.threads.sync.futexes.get_mut(&addr)?.waiters; + waiters.pop_front().map(|waiter| waiter.thread) + } } From 281a5382262af7f6d5d9caf677b4c7f2315dd359 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Thu, 1 Oct 2020 21:03:36 +0200 Subject: [PATCH 02/23] Move futex syscall to its own file. --- src/shims/posix/linux/foreign_items.rs | 60 +------------------------ src/shims/posix/linux/mod.rs | 1 + src/shims/posix/linux/sync.rs | 61 ++++++++++++++++++++++++++ 3 files changed, 63 insertions(+), 59 deletions(-) create mode 100644 src/shims/posix/linux/sync.rs diff --git a/src/shims/posix/linux/foreign_items.rs b/src/shims/posix/linux/foreign_items.rs index 8434d7bfa8..2241b8d4b3 100644 --- a/src/shims/posix/linux/foreign_items.rs +++ b/src/shims/posix/linux/foreign_items.rs @@ -1,9 +1,9 @@ use rustc_middle::mir; -use rustc_target::abi::{Align, Size}; use crate::*; use crate::helpers::check_arg_count; use shims::posix::fs::EvalContextExt as _; +use shims::posix::linux::sync::futex; use shims::posix::sync::EvalContextExt as _; use shims::posix::thread::EvalContextExt as _; @@ -201,61 +201,3 @@ fn getrandom<'tcx>( this.write_scalar(Scalar::from_machine_usize(len, this), dest)?; Ok(()) } - -fn futex<'tcx>( - this: &mut MiriEvalContext<'_, 'tcx>, - args: &[OpTy<'tcx, Tag>], - dest: PlaceTy<'tcx, Tag>, -) -> InterpResult<'tcx> { - if args.len() < 4 { - throw_ub_format!("incorrect number of arguments for futex syscall: got {}, expected at least 4", args.len()); - } - let addr = this.read_scalar(args[1])?.check_init()?; - let op = this.read_scalar(args[2])?.to_i32()?; - let val = this.read_scalar(args[3])?.to_i32()?; - - this.memory.check_ptr_access(addr, Size::from_bytes(4), Align::from_bytes(4).unwrap())?; - - let addr = addr.assert_ptr(); - - let thread = this.get_active_thread(); - - let futex_private = this.eval_libc_i32("FUTEX_PRIVATE_FLAG")?; - let futex_wait = this.eval_libc_i32("FUTEX_WAIT")?; - let futex_wake = this.eval_libc_i32("FUTEX_WAKE")?; - - match op & !futex_private { - op if op == futex_wait => { - if args.len() < 5 { - throw_ub_format!("incorrect number of arguments for FUTEX_WAIT syscall: got {}, expected at least 5", args.len()); - } - let timeout = this.read_scalar(args[4])?.check_init()?; - if !this.is_null(timeout)? { - throw_ub_format!("miri does not support timeouts for futex operations"); - } - let futex_val = this.read_scalar_at_offset(args[1], 0, this.machine.layouts.i32)?.to_i32()?; - if val == futex_val { - this.block_thread(thread); - this.futex_wait(addr, thread); - } else { - let eagain = this.eval_libc("EAGAIN")?; - this.set_last_error(eagain)?; - } - } - op if op == futex_wake => { - let mut n = 0; - for _ in 0..val { - if let Some(thread) = this.futex_wake(addr) { - this.unblock_thread(thread); - n += 1; - } else { - break; - } - } - this.write_scalar(Scalar::from_i32(n), dest)?; - } - op => throw_unsup_format!("miri does not support SYS_futex operation {}", op), - } - - Ok(()) -} diff --git a/src/shims/posix/linux/mod.rs b/src/shims/posix/linux/mod.rs index cadd6a8ea3..eba4a517cf 100644 --- a/src/shims/posix/linux/mod.rs +++ b/src/shims/posix/linux/mod.rs @@ -1,2 +1,3 @@ pub mod foreign_items; pub mod dlsym; +pub mod sync; diff --git a/src/shims/posix/linux/sync.rs b/src/shims/posix/linux/sync.rs new file mode 100644 index 0000000000..f9cfb3b8a2 --- /dev/null +++ b/src/shims/posix/linux/sync.rs @@ -0,0 +1,61 @@ +use crate::*; +use rustc_target::abi::{Align, Size}; + +/// Implementation of the SYS_futex syscall. +pub fn futex<'tcx>( + this: &mut MiriEvalContext<'_, 'tcx>, + args: &[OpTy<'tcx, Tag>], + dest: PlaceTy<'tcx, Tag>, +) -> InterpResult<'tcx> { + if args.len() < 4 { + throw_ub_format!("incorrect number of arguments for futex syscall: got {}, expected at least 4", args.len()); + } + let addr = this.read_scalar(args[1])?.check_init()?; + let op = this.read_scalar(args[2])?.to_i32()?; + let val = this.read_scalar(args[3])?.to_i32()?; + + this.memory.check_ptr_access(addr, Size::from_bytes(4), Align::from_bytes(4).unwrap())?; + + let addr = addr.assert_ptr(); + + let thread = this.get_active_thread(); + + let futex_private = this.eval_libc_i32("FUTEX_PRIVATE_FLAG")?; + let futex_wait = this.eval_libc_i32("FUTEX_WAIT")?; + let futex_wake = this.eval_libc_i32("FUTEX_WAKE")?; + + match op & !futex_private { + op if op == futex_wait => { + if args.len() < 5 { + throw_ub_format!("incorrect number of arguments for FUTEX_WAIT syscall: got {}, expected at least 5", args.len()); + } + let timeout = this.read_scalar(args[4])?.check_init()?; + if !this.is_null(timeout)? { + throw_ub_format!("miri does not support timeouts for futex operations"); + } + let futex_val = this.read_scalar_at_offset(args[1], 0, this.machine.layouts.i32)?.to_i32()?; + if val == futex_val { + this.block_thread(thread); + this.futex_wait(addr, thread); + } else { + let eagain = this.eval_libc("EAGAIN")?; + this.set_last_error(eagain)?; + } + } + op if op == futex_wake => { + let mut n = 0; + for _ in 0..val { + if let Some(thread) = this.futex_wake(addr) { + this.unblock_thread(thread); + n += 1; + } else { + break; + } + } + this.write_scalar(Scalar::from_i32(n), dest)?; + } + op => throw_unsup_format!("miri does not support SYS_futex operation {}", op), + } + + Ok(()) +} From 6c2f36eb6b6f35f94bc006c663d1622ebd71ff87 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Thu, 1 Oct 2020 21:06:16 +0200 Subject: [PATCH 03/23] Erase tag from futex pointers. --- src/shims/posix/linux/sync.rs | 2 +- src/sync.rs | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/shims/posix/linux/sync.rs b/src/shims/posix/linux/sync.rs index f9cfb3b8a2..23d3330c74 100644 --- a/src/shims/posix/linux/sync.rs +++ b/src/shims/posix/linux/sync.rs @@ -16,7 +16,7 @@ pub fn futex<'tcx>( this.memory.check_ptr_access(addr, Size::from_bytes(4), Align::from_bytes(4).unwrap())?; - let addr = addr.assert_ptr(); + let addr = addr.assert_ptr().erase_tag(); let thread = this.get_active_thread(); diff --git a/src/sync.rs b/src/sync.rs index d5594fb9ec..986857221b 100644 --- a/src/sync.rs +++ b/src/sync.rs @@ -115,7 +115,7 @@ pub(super) struct SynchronizationState { mutexes: IndexVec, rwlocks: IndexVec, condvars: IndexVec, - futexes: HashMap, Futex>, + futexes: HashMap, } // Private extension trait for local helper methods @@ -418,14 +418,14 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx this.machine.threads.sync.condvars[id].waiters.retain(|waiter| waiter.thread != thread); } - fn futex_wait(&mut self, addr: Pointer, thread: ThreadId) { + fn futex_wait(&mut self, addr: Pointer, thread: ThreadId) { let this = self.eval_context_mut(); let waiters = &mut this.machine.threads.sync.futexes.entry(addr).or_default().waiters; assert!(waiters.iter().all(|waiter| waiter.thread != thread), "thread is already waiting"); waiters.push_back(FutexWaiter { thread }); } - fn futex_wake(&mut self, addr: Pointer) -> Option { + fn futex_wake(&mut self, addr: Pointer) -> Option { let this = self.eval_context_mut(); let waiters = &mut this.machine.threads.sync.futexes.get_mut(&addr)?.waiters; waiters.pop_front().map(|waiter| waiter.thread) From 69cea1dc92695d317145fa057529f8c679e3cfc0 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Thu, 1 Oct 2020 22:57:27 +0200 Subject: [PATCH 04/23] Only check futex pointer in futex_wait and not in futex_wake. futex_wake doesn't access the futex itself, so should accept pointers to memory that's no longer there. --- src/shims/posix/linux/sync.rs | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/shims/posix/linux/sync.rs b/src/shims/posix/linux/sync.rs index 23d3330c74..d92fc0441c 100644 --- a/src/shims/posix/linux/sync.rs +++ b/src/shims/posix/linux/sync.rs @@ -10,14 +10,12 @@ pub fn futex<'tcx>( if args.len() < 4 { throw_ub_format!("incorrect number of arguments for futex syscall: got {}, expected at least 4", args.len()); } - let addr = this.read_scalar(args[1])?.check_init()?; + let addr = args[1]; + let addr_scalar = this.read_scalar(addr)?.check_init()?; + let futex_ptr = this.force_ptr(addr_scalar)?.erase_tag(); let op = this.read_scalar(args[2])?.to_i32()?; let val = this.read_scalar(args[3])?.to_i32()?; - this.memory.check_ptr_access(addr, Size::from_bytes(4), Align::from_bytes(4).unwrap())?; - - let addr = addr.assert_ptr().erase_tag(); - let thread = this.get_active_thread(); let futex_private = this.eval_libc_i32("FUTEX_PRIVATE_FLAG")?; @@ -33,10 +31,11 @@ pub fn futex<'tcx>( if !this.is_null(timeout)? { throw_ub_format!("miri does not support timeouts for futex operations"); } + this.memory.check_ptr_access(addr_scalar, Size::from_bytes(4), Align::from_bytes(4).unwrap())?; let futex_val = this.read_scalar_at_offset(args[1], 0, this.machine.layouts.i32)?.to_i32()?; if val == futex_val { this.block_thread(thread); - this.futex_wait(addr, thread); + this.futex_wait(futex_ptr, thread); } else { let eagain = this.eval_libc("EAGAIN")?; this.set_last_error(eagain)?; @@ -45,7 +44,7 @@ pub fn futex<'tcx>( op if op == futex_wake => { let mut n = 0; for _ in 0..val { - if let Some(thread) = this.futex_wake(addr) { + if let Some(thread) = this.futex_wake(futex_ptr) { this.unblock_thread(thread); n += 1; } else { From 1c582e7c967577d2760e05dee39cf57ea72c3606 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Fri, 2 Oct 2020 01:49:20 +0200 Subject: [PATCH 05/23] Return correct value from futex_wait. --- src/shims/posix/linux/sync.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/shims/posix/linux/sync.rs b/src/shims/posix/linux/sync.rs index d92fc0441c..0892eab467 100644 --- a/src/shims/posix/linux/sync.rs +++ b/src/shims/posix/linux/sync.rs @@ -36,9 +36,11 @@ pub fn futex<'tcx>( if val == futex_val { this.block_thread(thread); this.futex_wait(futex_ptr, thread); + this.write_scalar(Scalar::from_i32(0), dest)?; } else { let eagain = this.eval_libc("EAGAIN")?; this.set_last_error(eagain)?; + this.write_scalar(Scalar::from_i32(-1), dest)?; } } op if op == futex_wake => { From c2fa27c3b8bfd99240bda23fff1b09bb78c5e7fa Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Fri, 2 Oct 2020 10:46:57 +0200 Subject: [PATCH 06/23] Check maximum amount of arguments to SYS_futex. --- src/shims/posix/linux/sync.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/shims/posix/linux/sync.rs b/src/shims/posix/linux/sync.rs index 0892eab467..a891a7dd99 100644 --- a/src/shims/posix/linux/sync.rs +++ b/src/shims/posix/linux/sync.rs @@ -7,8 +7,11 @@ pub fn futex<'tcx>( args: &[OpTy<'tcx, Tag>], dest: PlaceTy<'tcx, Tag>, ) -> InterpResult<'tcx> { - if args.len() < 4 { - throw_ub_format!("incorrect number of arguments for futex syscall: got {}, expected at least 4", args.len()); + // The amount of arguments used depends on the type of futex operation. + // Some users always pass all arguments, even the unused ones, due to how they wrap this syscall in their code base. + // Some other users pass only the arguments the operation actually needs. So we don't use `check_arg_count` here. + if !(4..=7).contains(&args.len()) { + throw_ub_format!("incorrect number of arguments for futex syscall: got {}, expected between 4 and 7 (inclusive)", args.len()); } let addr = args[1]; let addr_scalar = this.read_scalar(addr)?.check_init()?; From 712e8006b3c3a055b53326196081df11da123d38 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Fri, 2 Oct 2020 10:47:36 +0200 Subject: [PATCH 07/23] Improve handling of the `addr` argument in SYS_futex. --- src/shims/posix/linux/sync.rs | 18 +++++++++++++----- src/sync.rs | 8 ++++---- 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/src/shims/posix/linux/sync.rs b/src/shims/posix/linux/sync.rs index a891a7dd99..2b31961559 100644 --- a/src/shims/posix/linux/sync.rs +++ b/src/shims/posix/linux/sync.rs @@ -13,12 +13,18 @@ pub fn futex<'tcx>( if !(4..=7).contains(&args.len()) { throw_ub_format!("incorrect number of arguments for futex syscall: got {}, expected between 4 and 7 (inclusive)", args.len()); } - let addr = args[1]; - let addr_scalar = this.read_scalar(addr)?.check_init()?; - let futex_ptr = this.force_ptr(addr_scalar)?.erase_tag(); + + // The first three arguments (after the syscall number itself) are the same to all futex operations: + // (int *addr, int op, int val). + // Although note that the first one is often passed as a different pointer type, e.g. `*const AtomicU32` or `*mut u32`. + let addr = this.deref_operand(args[1])?; let op = this.read_scalar(args[2])?.to_i32()?; let val = this.read_scalar(args[3])?.to_i32()?; + // The raw pointer value is used to identify the mutex. + // Not all mutex operations actually read from this address or even require this address to exist. + let futex_ptr = addr.ptr.assert_ptr(); + let thread = this.get_active_thread(); let futex_private = this.eval_libc_i32("FUTEX_PRIVATE_FLAG")?; @@ -34,8 +40,10 @@ pub fn futex<'tcx>( if !this.is_null(timeout)? { throw_ub_format!("miri does not support timeouts for futex operations"); } - this.memory.check_ptr_access(addr_scalar, Size::from_bytes(4), Align::from_bytes(4).unwrap())?; - let futex_val = this.read_scalar_at_offset(args[1], 0, this.machine.layouts.i32)?.to_i32()?; + // Check the pointer for alignment. Atomic operations are only available for fully aligned values. + this.memory.check_ptr_access(addr.ptr.into(), Size::from_bytes(4), Align::from_bytes(4).unwrap())?; + // Read an `i32` through the pointer, regardless of any wrapper types (e.g. `AtomicI32`). + let futex_val = this.read_scalar(addr.offset(Size::ZERO, MemPlaceMeta::None, this.machine.layouts.i32, this)?.into())?.to_i32()?; if val == futex_val { this.block_thread(thread); this.futex_wait(futex_ptr, thread); diff --git a/src/sync.rs b/src/sync.rs index 986857221b..f8b6f99f1e 100644 --- a/src/sync.rs +++ b/src/sync.rs @@ -418,16 +418,16 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx this.machine.threads.sync.condvars[id].waiters.retain(|waiter| waiter.thread != thread); } - fn futex_wait(&mut self, addr: Pointer, thread: ThreadId) { + fn futex_wait(&mut self, addr: Pointer, thread: ThreadId) { let this = self.eval_context_mut(); - let waiters = &mut this.machine.threads.sync.futexes.entry(addr).or_default().waiters; + let waiters = &mut this.machine.threads.sync.futexes.entry(addr.erase_tag()).or_default().waiters; assert!(waiters.iter().all(|waiter| waiter.thread != thread), "thread is already waiting"); waiters.push_back(FutexWaiter { thread }); } - fn futex_wake(&mut self, addr: Pointer) -> Option { + fn futex_wake(&mut self, addr: Pointer) -> Option { let this = self.eval_context_mut(); - let waiters = &mut this.machine.threads.sync.futexes.get_mut(&addr)?.waiters; + let waiters = &mut this.machine.threads.sync.futexes.get_mut(&addr.erase_tag())?.waiters; waiters.pop_front().map(|waiter| waiter.thread) } } From ee3eb4b223f823b5bf7df7ece97621aa36315fdc Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Fri, 2 Oct 2020 10:47:53 +0200 Subject: [PATCH 08/23] Add comments that document SYS_futex better. --- src/shims/posix/linux/sync.rs | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/shims/posix/linux/sync.rs b/src/shims/posix/linux/sync.rs index 2b31961559..8da6921653 100644 --- a/src/shims/posix/linux/sync.rs +++ b/src/shims/posix/linux/sync.rs @@ -31,13 +31,20 @@ pub fn futex<'tcx>( let futex_wait = this.eval_libc_i32("FUTEX_WAIT")?; let futex_wake = this.eval_libc_i32("FUTEX_WAKE")?; + // FUTEX_PRIVATE enables an optimization that stops it from working across processes. + // Miri doesn't support that anyway, so we ignore that flag. match op & !futex_private { + // FUTEX_WAIT: (int *addr, int op = FUTEX_WAIT, int val, const timespec *timeout) + // Blocks the thread if *addr still equals val. Wakes up when FUTEX_WAKE is called on the same address, + // or *timeout expires. `timeout == null` for an infinite timeout. op if op == futex_wait => { if args.len() < 5 { throw_ub_format!("incorrect number of arguments for FUTEX_WAIT syscall: got {}, expected at least 5", args.len()); } let timeout = this.read_scalar(args[4])?.check_init()?; if !this.is_null(timeout)? { + // FIXME: Implement timeouts. The condvar waiting code is probably a good example to start with. + // Note that a triggered timeout should have this syscall return with -1 and errno set to ETIMEOUT. throw_ub_format!("miri does not support timeouts for futex operations"); } // Check the pointer for alignment. Atomic operations are only available for fully aligned values. @@ -45,15 +52,23 @@ pub fn futex<'tcx>( // Read an `i32` through the pointer, regardless of any wrapper types (e.g. `AtomicI32`). let futex_val = this.read_scalar(addr.offset(Size::ZERO, MemPlaceMeta::None, this.machine.layouts.i32, this)?.into())?.to_i32()?; 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(futex_ptr, thread); + // Succesfully waking up from FUTEX_WAIT always returns zero. this.write_scalar(Scalar::from_i32(0), dest)?; } else { + // The futex value doesn't match the expected value, so we return failure + // right away without sleeping: -1 and errno set to EAGAIN. let eagain = this.eval_libc("EAGAIN")?; this.set_last_error(eagain)?; this.write_scalar(Scalar::from_i32(-1), dest)?; } } + // FUTEX_WAKE: (int *addr, int op = FUTEX_WAKE, int val) + // Wakes at most `val` threads waiting on the futex at `addr`. + // Returns the amount of threads woken up. + // Does not access the futex value at *addr. op if op == futex_wake => { let mut n = 0; for _ in 0..val { From dabd98056778238e9e1b9f0668183c7cb635d2b1 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Fri, 2 Oct 2020 20:53:31 +0200 Subject: [PATCH 09/23] Update note about number of arguments to SYS_futex. --- src/shims/posix/linux/sync.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/shims/posix/linux/sync.rs b/src/shims/posix/linux/sync.rs index 8da6921653..ae90583686 100644 --- a/src/shims/posix/linux/sync.rs +++ b/src/shims/posix/linux/sync.rs @@ -8,8 +8,13 @@ pub fn futex<'tcx>( dest: PlaceTy<'tcx, Tag>, ) -> InterpResult<'tcx> { // The amount of arguments used depends on the type of futex operation. - // Some users always pass all arguments, even the unused ones, due to how they wrap this syscall in their code base. - // Some other users pass only the arguments the operation actually needs. So we don't use `check_arg_count` here. + // The full futex syscall takes six arguments (excluding the syscall + // number), which is also the maximum amount of arguments a linux syscall + // can take on most architectures. + // However, not all futex operations use all six arguments. The unused ones + // 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 !(4..=7).contains(&args.len()) { throw_ub_format!("incorrect number of arguments for futex syscall: got {}, expected between 4 and 7 (inclusive)", args.len()); } From 422b5053a9a8730de46b6c22997a04afdd2d4d08 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Fri, 2 Oct 2020 20:56:01 +0200 Subject: [PATCH 10/23] Add note about arguments in futex implementation. Co-authored-by: Ralf Jung --- src/shims/posix/linux/sync.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/shims/posix/linux/sync.rs b/src/shims/posix/linux/sync.rs index ae90583686..e01e716879 100644 --- a/src/shims/posix/linux/sync.rs +++ b/src/shims/posix/linux/sync.rs @@ -21,6 +21,7 @@ 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. // Although note that the first one is often passed as a different pointer type, e.g. `*const AtomicU32` or `*mut u32`. let addr = this.deref_operand(args[1])?; let op = this.read_scalar(args[2])?.to_i32()?; From d5b3f54b46125a3da9af7e466e004b6905bc2f26 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Fri, 2 Oct 2020 21:59:11 +0200 Subject: [PATCH 11/23] Use force_ptr in futex implementation. --- src/shims/posix/linux/sync.rs | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/src/shims/posix/linux/sync.rs b/src/shims/posix/linux/sync.rs index e01e716879..1cfcb65bdc 100644 --- a/src/shims/posix/linux/sync.rs +++ b/src/shims/posix/linux/sync.rs @@ -22,15 +22,17 @@ 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. - // Although note that the first one is often passed as a different pointer type, e.g. `*const AtomicU32` or `*mut u32`. - let addr = this.deref_operand(args[1])?; + // + // `addr` is used to identify the mutex, but note that not all futex + // operations actually read from this addres or even require this address + // to exist. Also, the type of `addr` is not consistent. The API requires + // it to be a 4-byte aligned pointer, and will use the 4 bytes at the given + // address as an (atomic) i32. It's not uncommon for `addr` to be passed as + // another type than `*mut i32`, such as `*const AtomicI32`. + let addr = this.force_ptr(this.read_scalar(args[1])?.check_init()?)?; let op = this.read_scalar(args[2])?.to_i32()?; let val = this.read_scalar(args[3])?.to_i32()?; - // The raw pointer value is used to identify the mutex. - // Not all mutex operations actually read from this address or even require this address to exist. - let futex_ptr = addr.ptr.assert_ptr(); - let thread = this.get_active_thread(); let futex_private = this.eval_libc_i32("FUTEX_PRIVATE_FLAG")?; @@ -53,14 +55,15 @@ pub fn futex<'tcx>( // Note that a triggered timeout should have this syscall return with -1 and errno set to ETIMEOUT. throw_ub_format!("miri does not support timeouts for futex operations"); } - // Check the pointer for alignment. Atomic operations are only available for fully aligned values. - this.memory.check_ptr_access(addr.ptr.into(), Size::from_bytes(4), Align::from_bytes(4).unwrap())?; + // Check the pointer for alignment and validity. + // Atomic operations are only available for fully aligned values. + this.memory.check_ptr_access(addr.into(), Size::from_bytes(4), Align::from_bytes(4).unwrap())?; // Read an `i32` through the pointer, regardless of any wrapper types (e.g. `AtomicI32`). - let futex_val = this.read_scalar(addr.offset(Size::ZERO, MemPlaceMeta::None, this.machine.layouts.i32, this)?.into())?.to_i32()?; + let futex_val = this.memory.get_raw(addr.alloc_id)?.read_scalar(this, addr, Size::from_bytes(4))?.to_i32()?; 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(futex_ptr, thread); + this.futex_wait(addr, thread); // Succesfully waking up from FUTEX_WAIT always returns zero. this.write_scalar(Scalar::from_i32(0), dest)?; } else { @@ -78,7 +81,7 @@ pub fn futex<'tcx>( op if op == futex_wake => { let mut n = 0; for _ in 0..val { - if let Some(thread) = this.futex_wake(futex_ptr) { + if let Some(thread) = this.futex_wake(addr) { this.unblock_thread(thread); n += 1; } else { From e64ead2f46144963bc18ba34477422f39577f7f6 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Fri, 2 Oct 2020 23:34:14 +0200 Subject: [PATCH 12/23] Implement timeouts for FUTEX_WAIT. --- src/shims/posix/linux/sync.rs | 47 +++++++++++++++++++++++++++++------ src/sync.rs | 7 ++++++ 2 files changed, 47 insertions(+), 7 deletions(-) diff --git a/src/shims/posix/linux/sync.rs b/src/shims/posix/linux/sync.rs index 1cfcb65bdc..4201ef3f47 100644 --- a/src/shims/posix/linux/sync.rs +++ b/src/shims/posix/linux/sync.rs @@ -1,5 +1,7 @@ +use crate::thread::Time; use crate::*; use rustc_target::abi::{Align, Size}; +use std::time::{Instant, SystemTime}; /// Implementation of the SYS_futex syscall. pub fn futex<'tcx>( @@ -38,6 +40,7 @@ pub fn futex<'tcx>( let futex_private = this.eval_libc_i32("FUTEX_PRIVATE_FLAG")?; let futex_wait = this.eval_libc_i32("FUTEX_WAIT")?; let futex_wake = this.eval_libc_i32("FUTEX_WAKE")?; + let futex_realtime = this.eval_libc_i32("FUTEX_CLOCK_REALTIME")?; // FUTEX_PRIVATE enables an optimization that stops it from working across processes. // Miri doesn't support that anyway, so we ignore that flag. @@ -45,16 +48,29 @@ pub fn futex<'tcx>( // FUTEX_WAIT: (int *addr, int op = FUTEX_WAIT, int val, const timespec *timeout) // Blocks the thread if *addr still equals val. Wakes up when FUTEX_WAKE is called on the same address, // or *timeout expires. `timeout == null` for an infinite timeout. - op if op == futex_wait => { + op if op & !futex_realtime == futex_wait => { if args.len() < 5 { throw_ub_format!("incorrect number of arguments for FUTEX_WAIT syscall: got {}, expected at least 5", args.len()); } - let timeout = this.read_scalar(args[4])?.check_init()?; - if !this.is_null(timeout)? { - // FIXME: Implement timeouts. The condvar waiting code is probably a good example to start with. - // Note that a triggered timeout should have this syscall return with -1 and errno set to ETIMEOUT. - throw_ub_format!("miri does not support timeouts for futex operations"); - } + let timeout = args[4]; + let timeout_time = if this.is_null(this.read_scalar(timeout)?.check_init()?)? { + None + } else { + let duration = match this.read_timespec(timeout)? { + Some(duration) => duration, + None => { + let einval = this.eval_libc("EINVAL")?; + this.set_last_error(einval)?; + this.write_scalar(Scalar::from_i32(-1), dest)?; + return Ok(()); + } + }; + Some(if op & futex_realtime != 0 { + Time::RealTime(SystemTime::now().checked_add(duration).unwrap()) + } else { + Time::Monotonic(Instant::now().checked_add(duration).unwrap()) + }) + }; // Check the pointer for alignment and validity. // Atomic operations are only available for fully aligned values. this.memory.check_ptr_access(addr.into(), Size::from_bytes(4), Align::from_bytes(4).unwrap())?; @@ -66,6 +82,22 @@ pub fn futex<'tcx>( this.futex_wait(addr, thread); // Succesfully waking up from FUTEX_WAIT always returns zero. this.write_scalar(Scalar::from_i32(0), dest)?; + // Register a timeout callback if a timeout was specified. + // This callback will override the return value when the timeout triggers. + if let Some(timeout_time) = timeout_time { + this.register_timeout_callback( + thread, + timeout_time, + Box::new(move |this| { + this.unblock_thread(thread); + this.futex_remove_waiter(addr, thread); + let etimedout = this.eval_libc("ETIMEDOUT")?; + this.set_last_error(etimedout)?; + this.write_scalar(Scalar::from_i32(-1), dest)?; + Ok(()) + }), + ); + } } else { // The futex value doesn't match the expected value, so we return failure // right away without sleeping: -1 and errno set to EAGAIN. @@ -83,6 +115,7 @@ pub fn futex<'tcx>( for _ in 0..val { if let Some(thread) = this.futex_wake(addr) { this.unblock_thread(thread); + this.unregister_timeout_callback_if_exists(thread); n += 1; } else { break; diff --git a/src/sync.rs b/src/sync.rs index f8b6f99f1e..0c12da8d68 100644 --- a/src/sync.rs +++ b/src/sync.rs @@ -430,4 +430,11 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let waiters = &mut this.machine.threads.sync.futexes.get_mut(&addr.erase_tag())?.waiters; waiters.pop_front().map(|waiter| waiter.thread) } + + fn futex_remove_waiter(&mut self, addr: Pointer, thread: ThreadId) { + let this = self.eval_context_mut(); + if let Some(futex) = this.machine.threads.sync.futexes.get_mut(&addr.erase_tag()) { + futex.waiters.retain(|waiter| waiter.thread != thread); + } + } } From 81138825b370e112714d1f16e8f76835d57d8fdf Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Fri, 2 Oct 2020 23:35:00 +0200 Subject: [PATCH 13/23] Add park/park_timeout/unpark test. --- tests/run-pass/concurrency/parking.rs | 37 +++++++++++++++++++++++ tests/run-pass/concurrency/parking.stderr | 2 ++ 2 files changed, 39 insertions(+) create mode 100644 tests/run-pass/concurrency/parking.rs create mode 100644 tests/run-pass/concurrency/parking.stderr diff --git a/tests/run-pass/concurrency/parking.rs b/tests/run-pass/concurrency/parking.rs new file mode 100644 index 0000000000..1ed742931f --- /dev/null +++ b/tests/run-pass/concurrency/parking.rs @@ -0,0 +1,37 @@ +// ignore-windows: Concurrency on Windows is not supported yet. +// compile-flags: -Zmiri-disable-isolation + +use std::thread; +use std::time::{Duration, Instant}; + +// Normally, waiting in park/park_timeout may spuriously wake up early, but we +// know Miri's timed synchronization primitives do not do that. + +fn park_timeout() { + let start = Instant::now(); + + thread::park_timeout(Duration::from_millis(200)); + + assert!((200..500).contains(&start.elapsed().as_millis())); +} + +fn park_unpark() { + let t1 = thread::current(); + let t2 = thread::spawn(move || { + thread::park(); + thread::sleep(Duration::from_millis(200)); + t1.unpark(); + }); + + let start = Instant::now(); + + t2.thread().unpark(); + thread::park(); + + assert!((200..500).contains(&start.elapsed().as_millis())); +} + +fn main() { + park_timeout(); + park_unpark(); +} diff --git a/tests/run-pass/concurrency/parking.stderr b/tests/run-pass/concurrency/parking.stderr new file mode 100644 index 0000000000..2dbfb7721d --- /dev/null +++ b/tests/run-pass/concurrency/parking.stderr @@ -0,0 +1,2 @@ +warning: thread support is experimental. For example, Miri does not detect data races yet. + From c9627b25fb840adfd860aa9e417cc089bd7dc264 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Sat, 3 Oct 2020 00:40:53 +0200 Subject: [PATCH 14/23] Use correct return type for syscall(SYS_futex). --- src/shims/posix/linux/sync.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/shims/posix/linux/sync.rs b/src/shims/posix/linux/sync.rs index 4201ef3f47..17fd5f0eef 100644 --- a/src/shims/posix/linux/sync.rs +++ b/src/shims/posix/linux/sync.rs @@ -61,7 +61,7 @@ pub fn futex<'tcx>( None => { let einval = this.eval_libc("EINVAL")?; this.set_last_error(einval)?; - this.write_scalar(Scalar::from_i32(-1), dest)?; + this.write_scalar(Scalar::from_machine_isize(-1, this), dest)?; return Ok(()); } }; @@ -81,7 +81,7 @@ pub fn futex<'tcx>( this.block_thread(thread); this.futex_wait(addr, thread); // Succesfully waking up from FUTEX_WAIT always returns zero. - this.write_scalar(Scalar::from_i32(0), dest)?; + this.write_scalar(Scalar::from_machine_isize(0, this), dest)?; // Register a timeout callback if a timeout was specified. // This callback will override the return value when the timeout triggers. if let Some(timeout_time) = timeout_time { @@ -93,7 +93,7 @@ pub fn futex<'tcx>( this.futex_remove_waiter(addr, thread); let etimedout = this.eval_libc("ETIMEDOUT")?; this.set_last_error(etimedout)?; - this.write_scalar(Scalar::from_i32(-1), dest)?; + this.write_scalar(Scalar::from_machine_isize(-1, this), dest)?; Ok(()) }), ); @@ -103,7 +103,7 @@ pub fn futex<'tcx>( // right away without sleeping: -1 and errno set to EAGAIN. let eagain = this.eval_libc("EAGAIN")?; this.set_last_error(eagain)?; - this.write_scalar(Scalar::from_i32(-1), dest)?; + this.write_scalar(Scalar::from_machine_isize(-1, this), dest)?; } } // FUTEX_WAKE: (int *addr, int op = FUTEX_WAKE, int val) @@ -121,7 +121,7 @@ pub fn futex<'tcx>( break; } } - this.write_scalar(Scalar::from_i32(n), dest)?; + this.write_scalar(Scalar::from_machine_isize(n, this), dest)?; } op => throw_unsup_format!("miri does not support SYS_futex operation {}", op), } From 924fd56944bbd9138e8e1b59a1a8d5a329ddeb1d Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Sat, 3 Oct 2020 11:35:13 +0200 Subject: [PATCH 15/23] Only allow FUTEX_WAIT with timeout when isoloation is disabled. --- src/shims/posix/linux/sync.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/shims/posix/linux/sync.rs b/src/shims/posix/linux/sync.rs index 17fd5f0eef..3866785083 100644 --- a/src/shims/posix/linux/sync.rs +++ b/src/shims/posix/linux/sync.rs @@ -65,6 +65,7 @@ pub fn futex<'tcx>( return Ok(()); } }; + this.check_no_isolation("FUTEX_WAIT with timeout")?; Some(if op & futex_realtime != 0 { Time::RealTime(SystemTime::now().checked_add(duration).unwrap()) } else { From 66282754ff8d279c11dd946e84f1cd18cae8a24c Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Sat, 3 Oct 2020 11:38:16 +0200 Subject: [PATCH 16/23] Remove backtics from isolation error. Otherwise `FUTEX_WAIT with timeout` will look weird. --- src/helpers.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/helpers.rs b/src/helpers.rs index abf128ff3e..23bc54e76b 100644 --- a/src/helpers.rs +++ b/src/helpers.rs @@ -555,7 +555,7 @@ pub fn check_arg_count<'a, 'tcx, const N: usize>(args: &'a [OpTy<'tcx, Tag>]) -> pub fn isolation_error(name: &str) -> InterpResult<'static> { throw_machine_stop!(TerminationInfo::UnsupportedInIsolation(format!( - "`{}` not available when isolation is enabled", + "{} not available when isolation is enabled", name, ))) } From 5880e7d809d20246ce160710ebf7160e4d0c56c1 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Sat, 3 Oct 2020 12:00:29 +0200 Subject: [PATCH 17/23] Update expected error messages in tests. --- tests/compile-fail/fs/isolated_file.rs | 2 +- tests/compile-fail/fs/isolated_stdin.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/compile-fail/fs/isolated_file.rs b/tests/compile-fail/fs/isolated_file.rs index 5b7270f189..4c6adc8bf4 100644 --- a/tests/compile-fail/fs/isolated_file.rs +++ b/tests/compile-fail/fs/isolated_file.rs @@ -1,5 +1,5 @@ // ignore-windows: File handling is not implemented yet -// error-pattern: `open` not available when isolation is enabled +// error-pattern: open not available when isolation is enabled fn main() { let _file = std::fs::File::open("file.txt").unwrap(); diff --git a/tests/compile-fail/fs/isolated_stdin.rs b/tests/compile-fail/fs/isolated_stdin.rs index 6c467a2d1f..19ce064089 100644 --- a/tests/compile-fail/fs/isolated_stdin.rs +++ b/tests/compile-fail/fs/isolated_stdin.rs @@ -7,7 +7,7 @@ extern crate libc; fn main() -> std::io::Result<()> { let mut bytes = [0u8; 512]; unsafe { - libc::read(0, bytes.as_mut_ptr() as *mut libc::c_void, 512); //~ ERROR `read` not available when isolation is enabled + libc::read(0, bytes.as_mut_ptr() as *mut libc::c_void, 512); //~ ERROR read not available when isolation is enabled } Ok(()) } From 6df54c47a7ac0a0788d68c12198f1369b559b93e Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Sat, 3 Oct 2020 12:11:24 +0200 Subject: [PATCH 18/23] Use read_scalar_at_offset in futex_wait instead of memory.get_raw. --- src/shims/posix/linux/sync.rs | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/src/shims/posix/linux/sync.rs b/src/shims/posix/linux/sync.rs index 3866785083..cc09b33ba6 100644 --- a/src/shims/posix/linux/sync.rs +++ b/src/shims/posix/linux/sync.rs @@ -24,17 +24,14 @@ 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. - // - // `addr` is used to identify the mutex, but note that not all futex - // operations actually read from this addres or even require this address - // to exist. Also, the type of `addr` is not consistent. The API requires - // it to be a 4-byte aligned pointer, and will use the 4 bytes at the given - // address as an (atomic) i32. It's not uncommon for `addr` to be passed as - // another type than `*mut i32`, such as `*const AtomicI32`. - let addr = this.force_ptr(this.read_scalar(args[1])?.check_init()?)?; + 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()?; + // The raw pointer value is used to identify the mutex. + // Not all mutex operations actually read from this address or even require this address to exist. + let futex_ptr = this.force_ptr(addr.to_scalar()?)?; + let thread = this.get_active_thread(); let futex_private = this.eval_libc_i32("FUTEX_PRIVATE_FLAG")?; @@ -73,14 +70,16 @@ pub fn futex<'tcx>( }) }; // Check the pointer for alignment and validity. - // Atomic operations are only available for fully aligned values. - this.memory.check_ptr_access(addr.into(), Size::from_bytes(4), Align::from_bytes(4).unwrap())?; - // Read an `i32` through the pointer, regardless of any wrapper types (e.g. `AtomicI32`). - let futex_val = this.memory.get_raw(addr.alloc_id)?.read_scalar(this, addr, Size::from_bytes(4))?.to_i32()?; + // The API requires `addr` to be a 4-byte aligned pointer, and will + // use the 4 bytes at the given address as an (atomic) i32. + this.memory.check_ptr_access(addr.to_scalar()?, Size::from_bytes(4), Align::from_bytes(4).unwrap())?; + // Read an `i32` through the pointer, regardless of any wrapper types. + // It's not uncommon for `addr` to be passed as another type than `*mut i32`, such as `*const AtomicI32`. + let futex_val = this.read_scalar_at_offset(addr.into(), 0, this.machine.layouts.i32)?.to_i32()?; 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, thread); + this.futex_wait(futex_ptr, thread); // 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. @@ -91,7 +90,7 @@ pub fn futex<'tcx>( timeout_time, Box::new(move |this| { this.unblock_thread(thread); - this.futex_remove_waiter(addr, thread); + this.futex_remove_waiter(futex_ptr, thread); let etimedout = this.eval_libc("ETIMEDOUT")?; this.set_last_error(etimedout)?; this.write_scalar(Scalar::from_machine_isize(-1, this), dest)?; @@ -114,7 +113,7 @@ pub fn futex<'tcx>( op if op == futex_wake => { let mut n = 0; for _ in 0..val { - if let Some(thread) = this.futex_wake(addr) { + if let Some(thread) = this.futex_wake(futex_ptr) { this.unblock_thread(thread); this.unregister_timeout_callback_if_exists(thread); n += 1; From 9d764c57502c1f6badb71684b5d5b5ee081f4dda Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Sat, 3 Oct 2020 12:18:38 +0200 Subject: [PATCH 19/23] Add FIXME note about variadic syscall(). --- src/shims/posix/linux/foreign_items.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/shims/posix/linux/foreign_items.rs b/src/shims/posix/linux/foreign_items.rs index 2241b8d4b3..328280d459 100644 --- a/src/shims/posix/linux/foreign_items.rs +++ b/src/shims/posix/linux/foreign_items.rs @@ -113,6 +113,13 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx // Dynamically invoked syscalls "syscall" => { + // FIXME: The libc syscall() function is a variadic function. + // It's valid to call it with more arguments than a syscall + // needs, so none of these syscalls should use check_arg_count. + // However, depending on the calling convention it might depend + // on the type and size of the arguments whether a call with + // the wrong number of arguments (or types) is valid or not. + // So this needs to be researched first. let sys_getrandom = this .eval_libc("SYS_getrandom")? .to_machine_usize(this)?; From dc36988f388258f8ff10441f04f6b9fb30578165 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Sat, 3 Oct 2020 13:09:11 +0200 Subject: [PATCH 20/23] Add test for futex syscall. --- tests/run-pass/concurrency/linux-futex.rs | 132 ++++++++++++++++++ tests/run-pass/concurrency/linux-futex.stderr | 2 + 2 files changed, 134 insertions(+) create mode 100644 tests/run-pass/concurrency/linux-futex.rs create mode 100644 tests/run-pass/concurrency/linux-futex.stderr diff --git a/tests/run-pass/concurrency/linux-futex.rs b/tests/run-pass/concurrency/linux-futex.rs new file mode 100644 index 0000000000..391e952432 --- /dev/null +++ b/tests/run-pass/concurrency/linux-futex.rs @@ -0,0 +1,132 @@ +// Unfortunately, the test framework does not support 'only-linux', +// so we need to ignore Windows and macOS instead. +// ignore-macos: Uses Linux-only APIs +// ignore-windows: Uses Linux-only APIs +// compile-flags: -Zmiri-disable-isolation + +#![feature(rustc_private)] +extern crate libc; + +use std::ptr; +use std::thread; +use std::time::{Duration, Instant}; + +fn wake_nobody() { + let futex = 0; + + // Wake 1 waiter. Expect zero waiters woken up, as nobody is waiting. + unsafe { + assert_eq!(libc::syscall( + libc::SYS_futex, + &futex as *const i32, + libc::FUTEX_WAKE, + 1, + ), 0); + } + + // Same, but without omitting the unused arguments. + unsafe { + assert_eq!(libc::syscall( + libc::SYS_futex, + &futex as *const i32, + libc::FUTEX_WAKE, + 1, + 0, + 0, + 0, + ), 0); + } +} + +fn wake_dangling() { + let futex = Box::new(0); + let ptr: *const i32 = &*futex; + drop(futex); + + // Wake 1 waiter. Expect zero waiters woken up, as nobody is waiting. + unsafe { + assert_eq!(libc::syscall( + libc::SYS_futex, + ptr, + libc::FUTEX_WAKE, + 1, + ), 0); + } +} + +fn wait_wrong_val() { + let futex: i32 = 123; + + // Only wait if the futex value is 456. + unsafe { + assert_eq!(libc::syscall( + libc::SYS_futex, + &futex as *const i32, + libc::FUTEX_WAIT, + 456, + ptr::null::(), + ), -1); + assert_eq!(*libc::__errno_location(), libc::EAGAIN); + } +} + +fn wait_timeout() { + let start = Instant::now(); + + let futex: i32 = 123; + + // Wait for 200ms, with nobody waking us up early. + unsafe { + assert_eq!(libc::syscall( + libc::SYS_futex, + &futex as *const i32, + libc::FUTEX_WAIT, + 123, + &libc::timespec { + tv_sec: 0, + tv_nsec: 200_000_000, + }, + ), -1); + assert_eq!(*libc::__errno_location(), libc::ETIMEDOUT); + } + + assert!((200..500).contains(&start.elapsed().as_millis())); +} + +fn wait_wake() { + let start = Instant::now(); + + static FUTEX: i32 = 0; + + thread::spawn(move || { + thread::sleep(Duration::from_millis(200)); + unsafe { + assert_eq!(libc::syscall( + libc::SYS_futex, + &FUTEX as *const i32, + libc::FUTEX_WAKE, + 10, // Wake up at most 10 threads. + ), 1); // Woken up one thread. + } + }); + + unsafe { + assert_eq!(libc::syscall( + libc::SYS_futex, + &FUTEX as *const i32, + libc::FUTEX_WAIT, + 0, + ptr::null::(), + ), 0); + } + + assert!((200..500).contains(&start.elapsed().as_millis())); +} + +fn main() { + wake_nobody(); + wake_dangling(); + wait_wrong_val(); + wait_timeout(); + wait_wake(); +} diff --git a/tests/run-pass/concurrency/linux-futex.stderr b/tests/run-pass/concurrency/linux-futex.stderr new file mode 100644 index 0000000000..2dbfb7721d --- /dev/null +++ b/tests/run-pass/concurrency/linux-futex.stderr @@ -0,0 +1,2 @@ +warning: thread support is experimental. For example, Miri does not detect data races yet. + From dfcb46a4e04743c38a5ef355062bad8764b93ff5 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Sat, 3 Oct 2020 13:39:16 +0200 Subject: [PATCH 21/23] Update syscall FIXME to include note about 'wrong' types. --- src/shims/posix/linux/foreign_items.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/shims/posix/linux/foreign_items.rs b/src/shims/posix/linux/foreign_items.rs index 328280d459..364cfde6c0 100644 --- a/src/shims/posix/linux/foreign_items.rs +++ b/src/shims/posix/linux/foreign_items.rs @@ -116,10 +116,14 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx // FIXME: The libc syscall() function is a variadic function. // It's valid to call it with more arguments than a syscall // needs, so none of these syscalls should use check_arg_count. - // However, depending on the calling convention it might depend - // on the type and size of the arguments whether a call with - // the wrong number of arguments (or types) is valid or not. + // It's even valid to call it with the wrong type of arguments, + // as long as they'd end up in the same place with the calling + // convention used. (E.g. using a `usize` instead of a pointer.) + // It's not directly clear which number, size, and type of arguments + // are acceptable in which cases and which aren't. (E.g. some + // types might take up the space of two registers.) // So this needs to be researched first. + let sys_getrandom = this .eval_libc("SYS_getrandom")? .to_machine_usize(this)?; From c268ee2bcbffeef4f0c0e8ef121188729933078b Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Sat, 3 Oct 2020 14:21:37 +0200 Subject: [PATCH 22/23] Add note about use of force_ptr in futex implementation. Co-authored-by: Ralf Jung --- src/shims/posix/linux/sync.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/shims/posix/linux/sync.rs b/src/shims/posix/linux/sync.rs index cc09b33ba6..09558554aa 100644 --- a/src/shims/posix/linux/sync.rs +++ b/src/shims/posix/linux/sync.rs @@ -30,6 +30,8 @@ pub fn futex<'tcx>( // The raw pointer value is used to identify the mutex. // Not all mutex operations actually read from this address or even require this address to exist. + // This will make FUTEX_WAKE fail on an integer cast to a pointer. But FUTEX_WAIT on + // such a pointer can never work anyway, so that seems fine. let futex_ptr = this.force_ptr(addr.to_scalar()?)?; let thread = this.get_active_thread(); From 68776d292196f4e890f860c1464ecc83af80859f Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Sat, 3 Oct 2020 14:32:30 +0200 Subject: [PATCH 23/23] Add FIXME about type of `addr` in futex implementation. Co-authored-by: Ralf Jung --- src/shims/posix/linux/sync.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/shims/posix/linux/sync.rs b/src/shims/posix/linux/sync.rs index 09558554aa..d7ecb45279 100644 --- a/src/shims/posix/linux/sync.rs +++ b/src/shims/posix/linux/sync.rs @@ -77,6 +77,7 @@ pub fn futex<'tcx>( this.memory.check_ptr_access(addr.to_scalar()?, Size::from_bytes(4), Align::from_bytes(4).unwrap())?; // Read an `i32` through the pointer, regardless of any wrapper types. // It's not uncommon for `addr` to be passed as another type than `*mut i32`, such as `*const AtomicI32`. + // FIXME: this fails if `addr` is not a pointer type. let futex_val = this.read_scalar_at_offset(addr.into(), 0, this.machine.layouts.i32)?.to_i32()?; if val == futex_val { // The value still matches, so we block the trait make it wait for FUTEX_WAKE.