Auto merge of #3560 - RalfJung:sync-check-id, r=RalfJung

sync: better error in invalid synchronization primitive ID

`@devnexen` this should fix the ICE in your PR (but it won't fix the code, it will just report proper UB instead).

4 files changed, 67 insertions, 9 deletions

diff --git a/src/tools/miri/src/concurrency/sync.rs b/src/tools/miri/src/concurrency/sync.rs
index d3cef8bf5f3..94676955999 100644
--- a/src/tools/miri/src/concurrency/sync.rs
+++ b/src/tools/miri/src/concurrency/sync.rs
@@ -305,6 +305,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
         let this = self.eval_context_mut();
         let next_index = this.machine.threads.sync.mutexes.next_index();
         if let Some(old) = existing(this, next_index)? {
+            if this.machine.threads.sync.mutexes.get(old).is_none() {
+                throw_ub_format!("mutex has invalid ID");
+            }
             Ok(old)
         } else {
             let new_index = this.machine.threads.sync.mutexes.push(Default::default());
@@ -399,6 +402,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
         let this = self.eval_context_mut();
         let next_index = this.machine.threads.sync.rwlocks.next_index();
         if let Some(old) = existing(this, next_index)? {
+            if this.machine.threads.sync.rwlocks.get(old).is_none() {
+                throw_ub_format!("rwlock has invalid ID");
+            }
             Ok(old)
         } else {
             let new_index = this.machine.threads.sync.rwlocks.push(Default::default());
@@ -563,6 +569,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
         let this = self.eval_context_mut();
         let next_index = this.machine.threads.sync.condvars.next_index();
         if let Some(old) = existing(this, next_index)? {
+            if this.machine.threads.sync.condvars.get(old).is_none() {
+                throw_ub_format!("condvar has invalid ID");
+            }
             Ok(old)
         } else {
             let new_index = this.machine.threads.sync.condvars.push(Default::default());
diff --git a/src/tools/miri/src/shims/unix/sync.rs b/src/tools/miri/src/shims/unix/sync.rs
index e50a8934e09..9c096760415 100644
--- a/src/tools/miri/src/shims/unix/sync.rs
+++ b/src/tools/miri/src/shims/unix/sync.rs
@@ -65,7 +65,8 @@ fn mutexattr_set_kind<'mir, 'tcx: 'mir>(
 // (need to avoid this because it is set by static initializer macros)
 // bytes 4-7: mutex id as u32 or 0 if id is not assigned yet.
 // bytes 12-15 or 16-19 (depending on platform): mutex kind, as an i32
-// (the kind has to be at its offset for compatibility with static initializer macros)
+// (the kind has to be at this particular offset for compatibility with Linux's static initializer
+// macros, e.g. PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP.)
 
 fn mutex_get_id<'mir, 'tcx: 'mir>(
     ecx: &mut MiriInterpCx<'mir, 'tcx>,
@@ -123,11 +124,13 @@ fn mutex_set_kind<'mir, 'tcx: 'mir>(
 // (need to avoid this because it is set by static initializer macros)
 // bytes 4-7: rwlock id as u32 or 0 if id is not assigned yet.
 
+const RWLOCK_ID_OFFSET: u64 = 4;
+
 fn rwlock_get_id<'mir, 'tcx: 'mir>(
     ecx: &mut MiriInterpCx<'mir, 'tcx>,
     rwlock_op: &OpTy<'tcx, Provenance>,
 ) -> InterpResult<'tcx, RwLockId> {
-    ecx.rwlock_get_or_create_id(rwlock_op, ecx.libc_ty_layout("pthread_rwlock_t"), 4)
+    ecx.rwlock_get_or_create_id(rwlock_op, ecx.libc_ty_layout("pthread_rwlock_t"), RWLOCK_ID_OFFSET)
 }
 
 // pthread_condattr_t
@@ -136,13 +139,15 @@ fn rwlock_get_id<'mir, 'tcx: 'mir>(
 // store an i32 in the first four bytes equal to the corresponding libc clock id constant
 // (e.g. CLOCK_REALTIME).
 
+const CONDATTR_CLOCK_OFFSET: u64 = 0;
+
 fn condattr_get_clock_id<'mir, 'tcx: 'mir>(
     ecx: &MiriInterpCx<'mir, 'tcx>,
     attr_op: &OpTy<'tcx, Provenance>,
 ) -> InterpResult<'tcx, i32> {
     ecx.deref_pointer_and_read(
         attr_op,
-        0,
+        CONDATTR_CLOCK_OFFSET,
         ecx.libc_ty_layout("pthread_condattr_t"),
         ecx.machine.layouts.i32,
     )?
@@ -156,7 +161,7 @@ fn condattr_set_clock_id<'mir, 'tcx: 'mir>(
 ) -> InterpResult<'tcx, ()> {
     ecx.deref_pointer_and_write(
         attr_op,
-        0,
+        CONDATTR_CLOCK_OFFSET,
         Scalar::from_i32(clock_id),
         ecx.libc_ty_layout("pthread_condattr_t"),
         ecx.machine.layouts.i32,
@@ -172,11 +177,14 @@ fn condattr_set_clock_id<'mir, 'tcx: 'mir>(
 // bytes 4-7: the conditional variable id as u32 or 0 if id is not assigned yet.
 // bytes 8-11: the clock id constant as i32
 
+const CONDVAR_ID_OFFSET: u64 = 4;
+const CONDVAR_CLOCK_OFFSET: u64 = 8;
+
 fn cond_get_id<'mir, 'tcx: 'mir>(
     ecx: &mut MiriInterpCx<'mir, 'tcx>,
     cond_op: &OpTy<'tcx, Provenance>,
 ) -> InterpResult<'tcx, CondvarId> {
-    ecx.condvar_get_or_create_id(cond_op, ecx.libc_ty_layout("pthread_cond_t"), 4)
+    ecx.condvar_get_or_create_id(cond_op, ecx.libc_ty_layout("pthread_cond_t"), CONDVAR_ID_OFFSET)
 }
 
 fn cond_reset_id<'mir, 'tcx: 'mir>(
@@ -185,7 +193,7 @@ fn cond_reset_id<'mir, 'tcx: 'mir>(
 ) -> InterpResult<'tcx, ()> {
     ecx.deref_pointer_and_write(
         cond_op,
-        4,
+        CONDVAR_ID_OFFSET,
         Scalar::from_i32(0),
         ecx.libc_ty_layout("pthread_cond_t"),
         ecx.machine.layouts.u32,
@@ -198,7 +206,7 @@ fn cond_get_clock_id<'mir, 'tcx: 'mir>(
 ) -> InterpResult<'tcx, i32> {
     ecx.deref_pointer_and_read(
         cond_op,
-        8,
+        CONDVAR_CLOCK_OFFSET,
         ecx.libc_ty_layout("pthread_cond_t"),
         ecx.machine.layouts.i32,
     )?
@@ -212,7 +220,7 @@ fn cond_set_clock_id<'mir, 'tcx: 'mir>(
 ) -> InterpResult<'tcx, ()> {
     ecx.deref_pointer_and_write(
         cond_op,
-        8,
+        CONDVAR_CLOCK_OFFSET,
         Scalar::from_i32(clock_id),
         ecx.libc_ty_layout("pthread_cond_t"),
         ecx.machine.layouts.i32,
@@ -719,6 +727,14 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
     ) -> InterpResult<'tcx, Scalar<Provenance>> {
         let this = self.eval_context_mut();
 
+        // Does not exist on macOS!
+        if !matches!(&*this.tcx.sess.target.os, "linux") {
+            throw_unsup_format!(
+                "`pthread_condattr_init` is not supported on {}",
+                this.tcx.sess.target.os
+            );
+        }
+
         let clock_id = this.read_scalar(clock_id_op)?.to_i32()?;
         if clock_id == this.eval_libc_i32("CLOCK_REALTIME")
             || clock_id == this.eval_libc_i32("CLOCK_MONOTONIC")
@@ -739,6 +755,14 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
     ) -> InterpResult<'tcx, Scalar<Provenance>> {
         let this = self.eval_context_mut();
 
+        // Does not exist on macOS!
+        if !matches!(&*this.tcx.sess.target.os, "linux") {
+            throw_unsup_format!(
+                "`pthread_condattr_init` is not supported on {}",
+                this.tcx.sess.target.os
+            );
+        }
+
         let clock_id = condattr_get_clock_id(this, attr_op)?;
         this.write_scalar(Scalar::from_i32(clock_id), &this.deref_pointer(clk_id_op)?)?;
 
diff --git a/src/tools/miri/tests/pass-dep/shims/libc-rsfs.stdout b/src/tools/miri/tests/pass-dep/shims/libc-rsfs.stdout
deleted file mode 100644
index b6fa69e3d5d..00000000000
--- a/src/tools/miri/tests/pass-dep/shims/libc-rsfs.stdout
+++ /dev/null
@@ -1 +0,0 @@
-hello dup fd
diff --git a/src/tools/miri/tests/pass-dep/shims/pthread-sync.rs b/src/tools/miri/tests/pass-dep/shims/pthread-sync.rs
index c9d10cb83d4..12d3f2b6f14 100644
--- a/src/tools/miri/tests/pass-dep/shims/pthread-sync.rs
+++ b/src/tools/miri/tests/pass-dep/shims/pthread-sync.rs
@@ -19,6 +19,7 @@ fn main() {
     check_rwlock_write();
     check_rwlock_read_no_deadlock();
     check_cond();
+    check_condattr();
 }
 
 fn test_mutex_libc_init_recursive() {
@@ -261,6 +262,31 @@ fn check_cond() {
     }
 }
 
+fn check_condattr() {
+    unsafe {
+        // Just smoke-testing that these functions can be called.
+        let mut attr: MaybeUninit<libc::pthread_condattr_t> = MaybeUninit::uninit();
+        assert_eq!(libc::pthread_condattr_init(attr.as_mut_ptr()), 0);
+
+        #[cfg(not(target_os = "macos"))] // setclock-getclock do not exist on macOS
+        {
+            let clock_id = libc::CLOCK_MONOTONIC;
+            assert_eq!(libc::pthread_condattr_setclock(attr.as_mut_ptr(), clock_id), 0);
+            let mut check_clock_id = MaybeUninit::<libc::clockid_t>::uninit();
+            assert_eq!(
+                libc::pthread_condattr_getclock(attr.as_mut_ptr(), check_clock_id.as_mut_ptr()),
+                0
+            );
+            assert_eq!(check_clock_id.assume_init(), clock_id);
+        }
+
+        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);
+        assert_eq!(libc::pthread_cond_destroy(cond.as_mut_ptr()), 0);
+    }
+}
+
 // std::sync::RwLock does not even used pthread_rwlock any more.
 // Do some smoke testing of the API surface.
 fn test_rwlock_libc_static_initializer() {