From 98bb8acb5b437b7cc3a174d23660587469e4c8d1 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 4 May 2024 17:52:50 +0200 Subject: [PATCH 1/3] sync: better error in invalid synchronization primitive ID --- src/tools/miri/src/concurrency/sync.rs | 9 +++++++++ 1 file changed, 9 insertions(+) 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 @@ fn mutex_get_or_create(&mut self, existing: F) -> InterpResult<'tcx, MutexId> 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 @@ fn rwlock_get_or_create(&mut self, existing: F) -> InterpResult<'tcx, RwLockI 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 @@ fn condvar_get_or_create(&mut self, existing: F) -> InterpResult<'tcx, Condva 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()); From 86d7dff0b81748b45e43e0e4073ee9ec97c1e08f Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 4 May 2024 18:08:41 +0200 Subject: [PATCH 2/3] factor some pthread offset into constants --- src/tools/miri/src/shims/unix/sync.rs | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/src/tools/miri/src/shims/unix/sync.rs b/src/tools/miri/src/shims/unix/sync.rs index e50a8934e09..544f3259eea 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, From 9503c41eccd35239c1a35d4e1e38b620f3e6262b Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 4 May 2024 18:18:10 +0200 Subject: [PATCH 3/3] also test pthread_condattr_getclock --- src/tools/miri/src/shims/unix/sync.rs | 16 ++++++++++++ .../tests/pass-dep/shims/libc-rsfs.stdout | 1 - .../miri/tests/pass-dep/shims/pthread-sync.rs | 26 +++++++++++++++++++ 3 files changed, 42 insertions(+), 1 deletion(-) delete mode 100644 src/tools/miri/tests/pass-dep/shims/libc-rsfs.stdout diff --git a/src/tools/miri/src/shims/unix/sync.rs b/src/tools/miri/src/shims/unix/sync.rs index 544f3259eea..9c096760415 100644 --- a/src/tools/miri/src/shims/unix/sync.rs +++ b/src/tools/miri/src/shims/unix/sync.rs @@ -727,6 +727,14 @@ fn pthread_condattr_setclock( ) -> InterpResult<'tcx, Scalar> { 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") @@ -747,6 +755,14 @@ fn pthread_condattr_getclock( ) -> InterpResult<'tcx, Scalar> { 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 = 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::::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 = 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() {