Auto merge of #1933 - 5225225:1931-condvar-false-positive, r=RalfJung

Fix false positive use of uninit bytes when calling `libc::pthread_condattr_destroy`

Fixes: #1931

Author: bors

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<ScalarMaybeUninit<Tag>>,
 ) -> 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<libc::pthread_mutexattr_t> = 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.

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::<libc::pthread_condattr_t>::uninit();
+        libc::pthread_condattr_init(attr.as_mut_ptr());
+
+        let mut cond = MaybeUninit::<libc::pthread_cond_t>::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
+    }
+}

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::<libc::pthread_condattr_t>::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
+    }
+}

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::<libc::pthread_mutexattr_t>::uninit();
+        libc::pthread_mutexattr_init(attr.as_mut_ptr());
+
+        let mut mutex = MaybeUninit::<libc::pthread_mutex_t>::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
+    }
+}

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::<libc::pthread_mutexattr_t>::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
+    }
+}

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
+    }
+}

tests/run-pass/concurrency/libc_pthread_cond.rs

@@ -1,46 +1,47 @@
 // 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)]
 
 /// Test that conditional variable timeouts are working properly with both
 /// 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<libc::pthread_condattr_t> = 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<libc::pthread_cond_t> = 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<libc::timespec> = 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 };
 
         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();
         assert!(900 <= elapsed_time && elapsed_time <= 1300);
 
         // 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);
     }
 }
 

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