From eadeedde425e5a8027cf745da233bf3de58419a9 Mon Sep 17 00:00:00 2001 From: 5225225 <5225225@mailbox.org> Date: Sun, 5 Dec 2021 13:54:25 +0000 Subject: [PATCH 1/6] Handle uninit data in pthread_condattr_destroy --- src/shims/posix/sync.rs | 12 ++++++++++- .../libc_pthread_condattr_double_destroy.rs | 19 ++++++++++++++++++ .../concurrency/pthread_condattr_init.rs | 20 +++++++++++++++++++ 3 files changed, 50 insertions(+), 1 deletion(-) create mode 100644 tests/compile-fail/sync/libc_pthread_condattr_double_destroy.rs create mode 100644 tests/run-pass/concurrency/pthread_condattr_init.rs diff --git a/src/shims/posix/sync.rs b/src/shims/posix/sync.rs index 606f58a207e5..abf52a94e7e7 100644 --- a/src/shims/posix/sync.rs +++ b/src/shims/posix/sync.rs @@ -189,6 +189,14 @@ fn condattr_set_clock_id<'mir, 'tcx: 'mir>( ecx.write_scalar_at_offset(attr_op, 0, clock_id, ecx.machine.layouts.i32) } +fn condattr_deinit_clock_id<'mir, 'tcx: 'mir>( + ecx: &mut MiriEvalContext<'mir, 'tcx>, + attr_op: &OpTy<'tcx, Tag>, +) -> InterpResult<'tcx, ()> { + let layout = layout_of_maybe_uninit(ecx.tcx, ecx.machine.layouts.i32.ty); + ecx.write_scalar_at_offset(attr_op, 0, ScalarMaybeUninit::Uninit, layout) +} + // pthread_cond_t // Our chosen memory layout for the emulated conditional variable (does not have @@ -652,7 +660,9 @@ 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(); - condattr_set_clock_id(this, attr_op, ScalarMaybeUninit::Uninit)?; + condattr_get_clock_id(this, attr_op)?.check_init()?; + + condattr_deinit_clock_id(this, attr_op)?; Ok(0) } 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/run-pass/concurrency/pthread_condattr_init.rs b/tests/run-pass/concurrency/pthread_condattr_init.rs new file mode 100644 index 000000000000..285c6014e2d9 --- /dev/null +++ b/tests/run-pass/concurrency/pthread_condattr_init.rs @@ -0,0 +1,20 @@ +// ignore-windows: No libc on Windows +// compile-flags: -Zmiri-check-number-validity + +#![feature(rustc_private)] + +/// Test that pthread_condattr_destroy doesn't trigger a number validity error. +extern crate libc; + +fn main() { + unsafe { + use core::mem::MaybeUninit; + let mut attr = MaybeUninit::::uninit(); + + let r = libc::pthread_condattr_init(attr.as_mut_ptr()); + assert_eq!(r, 0); + + let r = libc::pthread_condattr_destroy(attr.as_mut_ptr()); + assert_eq!(r, 0); + } +} From ae120563cc4ee0ac2f0b687c6ccc1d2d68c6f892 Mon Sep 17 00:00:00 2001 From: 5225225 <5225225@mailbox.org> Date: Mon, 6 Dec 2021 19:26:13 +0000 Subject: [PATCH 2/6] Destroying any uninit posix_ object is UB --- src/shims/posix/sync.rs | 33 +++++++++++++++++++++++---------- 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/src/shims/posix/sync.rs b/src/shims/posix/sync.rs index abf52a94e7e7..29bca11f831e 100644 --- a/src/shims/posix/sync.rs +++ b/src/shims/posix/sync.rs @@ -186,15 +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) -} - -fn condattr_deinit_clock_id<'mir, 'tcx: 'mir>( - ecx: &mut MiriEvalContext<'mir, 'tcx>, - attr_op: &OpTy<'tcx, Tag>, -) -> InterpResult<'tcx, ()> { - let layout = layout_of_maybe_uninit(ecx.tcx, ecx.machine.layouts.i32.ty); - ecx.write_scalar_at_offset(attr_op, 0, ScalarMaybeUninit::Uninit, layout) + ecx.write_scalar_at_offset( + attr_op, + 0, + clock_id, + layout_of_maybe_uninit(ecx.tcx, ecx.machine.layouts.i32.ty), + ) } // pthread_cond_t @@ -367,6 +364,9 @@ 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()?; + mutexattr_set_kind(this, attr_op, ScalarMaybeUninit::Uninit)?; Ok(0) @@ -505,6 +505,10 @@ 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()?; + mutex_set_kind(this, mutex_op, ScalarMaybeUninit::Uninit)?; mutex_set_id(this, mutex_op, ScalarMaybeUninit::Uninit)?; // FIXME: delete interpreter state associated with this mutex. @@ -606,6 +610,9 @@ 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()?; + rwlock_set_id(this, rwlock_op, ScalarMaybeUninit::Uninit)?; // FIXME: delete interpreter state associated with this rwlock. @@ -660,9 +667,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()?; - condattr_deinit_clock_id(this, attr_op)?; + condattr_set_clock_id(this, attr_op, ScalarMaybeUninit::Uninit)?; Ok(0) } @@ -799,6 +807,11 @@ 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()?; + 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. From f0d915703ca8130367d6089cb79369f67888c9ad Mon Sep 17 00:00:00 2001 From: 5225225 <5225225@mailbox.org> Date: Mon, 6 Dec 2021 21:15:02 +0000 Subject: [PATCH 3/6] Add tests for double destroying various pthread items --- .../sync/libc_pthread_cond_double_destroy.rs | 22 ++++++++++++++++++ .../sync/libc_pthread_mutex_double_destroy.rs | 23 +++++++++++++++++++ .../libc_pthread_mutexattr_double_destroy.rs | 19 +++++++++++++++ .../libc_pthread_rwlock_double_destroy.rs | 16 +++++++++++++ 4 files changed, 80 insertions(+) create mode 100644 tests/compile-fail/sync/libc_pthread_cond_double_destroy.rs create mode 100644 tests/compile-fail/sync/libc_pthread_mutex_double_destroy.rs create mode 100644 tests/compile-fail/sync/libc_pthread_mutexattr_double_destroy.rs create mode 100644 tests/compile-fail/sync/libc_pthread_rwlock_double_destroy.rs 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_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 + } +} From a4b2fc0c5a3acd6ab737aa1f4c29df8dbd621a80 Mon Sep 17 00:00:00 2001 From: 5225225 <5225225@mailbox.org> Date: Mon, 6 Dec 2021 21:50:14 +0000 Subject: [PATCH 4/6] Adjust pthread tests --- .../run-pass/concurrency/libc_pthread_cond.rs | 35 ++++++++++--------- .../concurrency/pthread_condattr_init.rs | 20 ----------- tests/run-pass/concurrency/sync.rs | 7 +++- 3 files changed, 24 insertions(+), 38 deletions(-) delete mode 100644 tests/run-pass/concurrency/pthread_condattr_init.rs 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/pthread_condattr_init.rs b/tests/run-pass/concurrency/pthread_condattr_init.rs deleted file mode 100644 index 285c6014e2d9..000000000000 --- a/tests/run-pass/concurrency/pthread_condattr_init.rs +++ /dev/null @@ -1,20 +0,0 @@ -// ignore-windows: No libc on Windows -// compile-flags: -Zmiri-check-number-validity - -#![feature(rustc_private)] - -/// Test that pthread_condattr_destroy doesn't trigger a number validity error. -extern crate libc; - -fn main() { - unsafe { - use core::mem::MaybeUninit; - let mut attr = MaybeUninit::::uninit(); - - let r = libc::pthread_condattr_init(attr.as_mut_ptr()); - assert_eq!(r, 0); - - let r = libc::pthread_condattr_destroy(attr.as_mut_ptr()); - assert_eq!(r, 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(); } From 250d450593633dcb40b5ba515b492afbe6d01c99 Mon Sep 17 00:00:00 2001 From: 5225225 <5225225@mailbox.org> Date: Tue, 7 Dec 2021 08:26:46 +0000 Subject: [PATCH 5/6] Add comment explaining false positives in _destroy --- src/shims/posix/sync.rs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/shims/posix/sync.rs b/src/shims/posix/sync.rs index 29bca11f831e..1d0483e49d51 100644 --- a/src/shims/posix/sync.rs +++ b/src/shims/posix/sync.rs @@ -367,6 +367,18 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx // Destroying an uninit pthread_mutexattr is UB, so check to make sure it's not uninit. mutexattr_get_kind(this, attr_op)?.check_init()?; + // 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(); + // + // 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) @@ -509,6 +521,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx 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. @@ -613,6 +626,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx // 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. @@ -670,6 +684,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx // 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) @@ -812,6 +827,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx 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. From fd830e7b278063c93c57028b9469875fb36718a6 Mon Sep 17 00:00:00 2001 From: 5225225 <5225225@mailbox.org> Date: Tue, 7 Dec 2021 17:25:28 +0000 Subject: [PATCH 6/6] Code comment changes from code review Co-authored-by: Ralf Jung --- src/shims/posix/sync.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/shims/posix/sync.rs b/src/shims/posix/sync.rs index 1d0483e49d51..ea940df1c6e8 100644 --- a/src/shims/posix/sync.rs +++ b/src/shims/posix/sync.rs @@ -367,6 +367,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx // 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 @@ -376,6 +377,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx // 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