diff --git a/src/shims/posix/sync.rs b/src/shims/posix/sync.rs index 606f58a207e5..ea940df1c6e8 100644 --- a/src/shims/posix/sync.rs +++ b/src/shims/posix/sync.rs @@ -186,7 +186,12 @@ fn condattr_set_clock_id<'mir, 'tcx: 'mir>( attr_op: &OpTy<'tcx, Tag>, clock_id: impl Into>, ) -> InterpResult<'tcx, ()> { - ecx.write_scalar_at_offset(attr_op, 0, clock_id, ecx.machine.layouts.i32) + ecx.write_scalar_at_offset( + attr_op, + 0, + clock_id, + layout_of_maybe_uninit(ecx.tcx, ecx.machine.layouts.i32.ty), + ) } // pthread_cond_t @@ -359,6 +364,23 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx fn pthread_mutexattr_destroy(&mut self, attr_op: &OpTy<'tcx, Tag>) -> InterpResult<'tcx, i32> { let this = self.eval_context_mut(); + // Destroying an uninit pthread_mutexattr is UB, so check to make sure it's not uninit. + mutexattr_get_kind(this, attr_op)?.check_init()?; + + // To catch double-destroys, we de-initialize the mutexattr. + // This is technically not right and might lead to false positives. For example, the below + // code is *likely* sound, even assuming uninit numbers are UB, but miri with + // -Zmiri-check-number-validity complains + // + // let mut x: MaybeUninit = MaybeUninit::zeroed(); + // libc::pthread_mutexattr_init(x.as_mut_ptr()); + // libc::pthread_mutexattr_destroy(x.as_mut_ptr()); + // x.assume_init(); + // + // However, the way libstd uses the pthread APIs works in our favor here, so we can get away with this. + // This can always be revisited to have some external state to catch double-destroys + // but not complain about the above code. See https://github.com/rust-lang/miri/pull/1933 + mutexattr_set_kind(this, attr_op, ScalarMaybeUninit::Uninit)?; Ok(0) @@ -497,6 +519,11 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx throw_ub_format!("destroyed a locked mutex"); } + // Destroying an uninit pthread_mutex is UB, so check to make sure it's not uninit. + mutex_get_kind(this, mutex_op)?.check_init()?; + mutex_get_id(this, mutex_op)?.check_init()?; + + // This might lead to false positives, see comment in pthread_mutexattr_destroy mutex_set_kind(this, mutex_op, ScalarMaybeUninit::Uninit)?; mutex_set_id(this, mutex_op, ScalarMaybeUninit::Uninit)?; // FIXME: delete interpreter state associated with this mutex. @@ -598,6 +625,10 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx throw_ub_format!("destroyed a locked rwlock"); } + // Destroying an uninit pthread_rwlock is UB, so check to make sure it's not uninit. + rwlock_get_id(this, rwlock_op)?.check_init()?; + + // This might lead to false positives, see comment in pthread_mutexattr_destroy rwlock_set_id(this, rwlock_op, ScalarMaybeUninit::Uninit)?; // FIXME: delete interpreter state associated with this rwlock. @@ -652,6 +683,10 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx fn pthread_condattr_destroy(&mut self, attr_op: &OpTy<'tcx, Tag>) -> InterpResult<'tcx, i32> { let this = self.eval_context_mut(); + // Destroying an uninit pthread_condattr is UB, so check to make sure it's not uninit. + condattr_get_clock_id(this, attr_op)?.check_init()?; + + // This might lead to false positives, see comment in pthread_mutexattr_destroy condattr_set_clock_id(this, attr_op, ScalarMaybeUninit::Uninit)?; Ok(0) @@ -789,6 +824,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx if this.condvar_is_awaited(id) { throw_ub_format!("destroying an awaited conditional variable"); } + + // Destroying an uninit pthread_cond is UB, so check to make sure it's not uninit. + cond_get_id(this, cond_op)?.check_init()?; + cond_get_clock_id(this, cond_op)?.check_init()?; + + // This might lead to false positives, see comment in pthread_mutexattr_destroy cond_set_id(this, cond_op, ScalarMaybeUninit::Uninit)?; cond_set_clock_id(this, cond_op, ScalarMaybeUninit::Uninit)?; // FIXME: delete interpreter state associated with this condvar. diff --git a/tests/compile-fail/sync/libc_pthread_cond_double_destroy.rs b/tests/compile-fail/sync/libc_pthread_cond_double_destroy.rs new file mode 100644 index 000000000000..c376618357d2 --- /dev/null +++ b/tests/compile-fail/sync/libc_pthread_cond_double_destroy.rs @@ -0,0 +1,22 @@ +// ignore-windows: No libc on Windows +#![feature(rustc_private)] + +/// Test that destroying a pthread_cond twice fails, even without a check for number validity +extern crate libc; + +fn main() { + unsafe { + use core::mem::MaybeUninit; + let mut attr = MaybeUninit::::uninit(); + libc::pthread_condattr_init(attr.as_mut_ptr()); + + let mut cond = MaybeUninit::::uninit(); + + libc::pthread_cond_init(cond.as_mut_ptr(), attr.as_ptr()); + + libc::pthread_cond_destroy(cond.as_mut_ptr()); + + libc::pthread_cond_destroy(cond.as_mut_ptr()); + //~^ Undefined Behavior: using uninitialized data, but this operation requires initialized memory + } +} diff --git a/tests/compile-fail/sync/libc_pthread_condattr_double_destroy.rs b/tests/compile-fail/sync/libc_pthread_condattr_double_destroy.rs new file mode 100644 index 000000000000..44af51a3e871 --- /dev/null +++ b/tests/compile-fail/sync/libc_pthread_condattr_double_destroy.rs @@ -0,0 +1,19 @@ +// ignore-windows: No libc on Windows +#![feature(rustc_private)] + +/// Test that destroying a pthread_condattr twice fails, even without a check for number validity +extern crate libc; + +fn main() { + unsafe { + use core::mem::MaybeUninit; + let mut attr = MaybeUninit::::uninit(); + + libc::pthread_condattr_init(attr.as_mut_ptr()); + + libc::pthread_condattr_destroy(attr.as_mut_ptr()); + + libc::pthread_condattr_destroy(attr.as_mut_ptr()); + //~^ Undefined Behavior: using uninitialized data, but this operation requires initialized memory + } +} diff --git a/tests/compile-fail/sync/libc_pthread_mutex_double_destroy.rs b/tests/compile-fail/sync/libc_pthread_mutex_double_destroy.rs new file mode 100644 index 000000000000..08abc0ca12c5 --- /dev/null +++ b/tests/compile-fail/sync/libc_pthread_mutex_double_destroy.rs @@ -0,0 +1,23 @@ +// ignore-windows: No libc on Windows +#![feature(rustc_private)] + +/// Test that destroying a pthread_mutex twice fails, even without a check for number validity +extern crate libc; + +fn main() { + unsafe { + use core::mem::MaybeUninit; + + let mut attr = MaybeUninit::::uninit(); + libc::pthread_mutexattr_init(attr.as_mut_ptr()); + + let mut mutex = MaybeUninit::::uninit(); + + libc::pthread_mutex_init(mutex.as_mut_ptr(), attr.as_ptr()); + + libc::pthread_mutex_destroy(mutex.as_mut_ptr()); + + libc::pthread_mutex_destroy(mutex.as_mut_ptr()); + //~^ Undefined Behavior: using uninitialized data, but this operation requires initialized memory + } +} diff --git a/tests/compile-fail/sync/libc_pthread_mutexattr_double_destroy.rs b/tests/compile-fail/sync/libc_pthread_mutexattr_double_destroy.rs new file mode 100644 index 000000000000..69ca3ad512fa --- /dev/null +++ b/tests/compile-fail/sync/libc_pthread_mutexattr_double_destroy.rs @@ -0,0 +1,19 @@ +// ignore-windows: No libc on Windows +#![feature(rustc_private)] + +/// Test that destroying a pthread_mutexattr twice fails, even without a check for number validity +extern crate libc; + +fn main() { + unsafe { + use core::mem::MaybeUninit; + let mut attr = MaybeUninit::::uninit(); + + libc::pthread_mutexattr_init(attr.as_mut_ptr()); + + libc::pthread_mutexattr_destroy(attr.as_mut_ptr()); + + libc::pthread_mutexattr_destroy(attr.as_mut_ptr()); + //~^ Undefined Behavior: using uninitialized data, but this operation requires initialized memory + } +} diff --git a/tests/compile-fail/sync/libc_pthread_rwlock_double_destroy.rs b/tests/compile-fail/sync/libc_pthread_rwlock_double_destroy.rs new file mode 100644 index 000000000000..d20c78155fd2 --- /dev/null +++ b/tests/compile-fail/sync/libc_pthread_rwlock_double_destroy.rs @@ -0,0 +1,16 @@ +// ignore-windows: No libc on Windows +#![feature(rustc_private)] + +/// Test that destroying a pthread_rwlock twice fails, even without a check for number validity +extern crate libc; + +fn main() { + unsafe { + let mut lock = libc::PTHREAD_RWLOCK_INITIALIZER; + + libc::pthread_rwlock_destroy(&mut lock); + + libc::pthread_rwlock_destroy(&mut lock); + //~^ Undefined Behavior: using uninitialized data, but this operation requires initialized memory + } +} diff --git a/tests/run-pass/concurrency/libc_pthread_cond.rs b/tests/run-pass/concurrency/libc_pthread_cond.rs index a545c922db1a..0e09ec9126a5 100644 --- a/tests/run-pass/concurrency/libc_pthread_cond.rs +++ b/tests/run-pass/concurrency/libc_pthread_cond.rs @@ -1,6 +1,6 @@ // ignore-windows: No libc on Windows // ignore-macos: pthread_condattr_setclock is not supported on MacOS. -// compile-flags: -Zmiri-disable-isolation +// compile-flags: -Zmiri-disable-isolation -Zmiri-check-number-validity #![feature(rustc_private)] @@ -8,23 +8,24 @@ /// monotonic and system clocks. extern crate libc; -use std::mem; +use std::mem::MaybeUninit; use std::time::Instant; fn test_timed_wait_timeout(clock_id: i32) { unsafe { - let mut attr: libc::pthread_condattr_t = mem::zeroed(); - assert_eq!(libc::pthread_condattr_init(&mut attr as *mut _), 0); - assert_eq!(libc::pthread_condattr_setclock(&mut attr as *mut _, clock_id), 0); + let mut attr: MaybeUninit = MaybeUninit::uninit(); + assert_eq!(libc::pthread_condattr_init(attr.as_mut_ptr()), 0); + assert_eq!(libc::pthread_condattr_setclock(attr.as_mut_ptr(), clock_id), 0); - let mut cond: libc::pthread_cond_t = mem::zeroed(); - assert_eq!(libc::pthread_cond_init(&mut cond as *mut _, &attr as *const _), 0); - assert_eq!(libc::pthread_condattr_destroy(&mut attr as *mut _), 0); + let mut cond: MaybeUninit = MaybeUninit::uninit(); + assert_eq!(libc::pthread_cond_init(cond.as_mut_ptr(), attr.as_ptr()), 0); + assert_eq!(libc::pthread_condattr_destroy(attr.as_mut_ptr()), 0); - let mut mutex: libc::pthread_mutex_t = mem::zeroed(); + let mut mutex: libc::pthread_mutex_t = libc::PTHREAD_MUTEX_INITIALIZER; - let mut now: libc::timespec = mem::zeroed(); - assert_eq!(libc::clock_gettime(clock_id, &mut now), 0); + let mut now_mu: MaybeUninit = MaybeUninit::uninit(); + assert_eq!(libc::clock_gettime(clock_id, now_mu.as_mut_ptr()), 0); + let now = now_mu.assume_init(); // Waiting for a second... mostly because waiting less requires mich more tricky arithmetic. // FIXME: wait less. let timeout = libc::timespec { tv_sec: now.tv_sec + 1, tv_nsec: now.tv_nsec }; @@ -32,7 +33,7 @@ fn test_timed_wait_timeout(clock_id: i32) { assert_eq!(libc::pthread_mutex_lock(&mut mutex as *mut _), 0); let current_time = Instant::now(); assert_eq!( - libc::pthread_cond_timedwait(&mut cond as *mut _, &mut mutex as *mut _, &timeout), + libc::pthread_cond_timedwait(cond.as_mut_ptr(), &mut mutex as *mut _, &timeout), libc::ETIMEDOUT ); let elapsed_time = current_time.elapsed().as_millis(); @@ -40,7 +41,7 @@ fn test_timed_wait_timeout(clock_id: i32) { // Test calling `pthread_cond_timedwait` again with an already elapsed timeout. assert_eq!( - libc::pthread_cond_timedwait(&mut cond as *mut _, &mut mutex as *mut _, &timeout), + libc::pthread_cond_timedwait(cond.as_mut_ptr(), &mut mutex as *mut _, &timeout), libc::ETIMEDOUT ); @@ -49,7 +50,7 @@ fn test_timed_wait_timeout(clock_id: i32) { let invalid_timeout_1 = libc::timespec { tv_sec: now.tv_sec + 1, tv_nsec: 1_000_000_000 }; assert_eq!( libc::pthread_cond_timedwait( - &mut cond as *mut _, + cond.as_mut_ptr(), &mut mutex as *mut _, &invalid_timeout_1 ), @@ -58,7 +59,7 @@ fn test_timed_wait_timeout(clock_id: i32) { let invalid_timeout_2 = libc::timespec { tv_sec: now.tv_sec + 1, tv_nsec: -1 }; assert_eq!( libc::pthread_cond_timedwait( - &mut cond as *mut _, + cond.as_mut_ptr(), &mut mutex as *mut _, &invalid_timeout_2 ), @@ -68,7 +69,7 @@ fn test_timed_wait_timeout(clock_id: i32) { let invalid_timeout_3 = libc::timespec { tv_sec: -1, tv_nsec: 0 }; assert_eq!( libc::pthread_cond_timedwait( - &mut cond as *mut _, + cond.as_mut_ptr(), &mut mutex as *mut _, &invalid_timeout_3 ), @@ -77,7 +78,7 @@ fn test_timed_wait_timeout(clock_id: i32) { assert_eq!(libc::pthread_mutex_unlock(&mut mutex as *mut _), 0); assert_eq!(libc::pthread_mutex_destroy(&mut mutex as *mut _), 0); - assert_eq!(libc::pthread_cond_destroy(&mut cond as *mut _), 0); + assert_eq!(libc::pthread_cond_destroy(cond.as_mut_ptr()), 0); } } diff --git a/tests/run-pass/concurrency/sync.rs b/tests/run-pass/concurrency/sync.rs index 88187d64e604..da6f6f25ec1c 100644 --- a/tests/run-pass/concurrency/sync.rs +++ b/tests/run-pass/concurrency/sync.rs @@ -1,5 +1,5 @@ // ignore-windows: Concurrency on Windows is not supported yet. -// compile-flags: -Zmiri-disable-isolation +// compile-flags: -Zmiri-disable-isolation -Zmiri-check-number-validity use std::sync::mpsc::{channel, sync_channel}; use std::sync::{Arc, Barrier, Condvar, Mutex, Once, RwLock}; @@ -340,6 +340,10 @@ fn park_unpark() { assert!((200..1000).contains(&start.elapsed().as_millis())); } +fn check_condvar() { + let _ = std::sync::Condvar::new(); +} + fn main() { check_barriers(); check_conditional_variables_notify_one(); @@ -357,4 +361,5 @@ fn main() { check_rwlock_unlock_bug2(); park_timeout(); park_unpark(); + check_condvar(); }