From 8a145efc70f0eea89de9fae314cb37b64c1f2b75 Mon Sep 17 00:00:00 2001 From: joboet Date: Sat, 13 Sep 2025 13:01:11 +0200 Subject: [PATCH 1/2] std: improve handling of timed condition variable waits on macOS --- library/Cargo.lock | 4 +- library/std/Cargo.toml | 2 +- library/std/src/sys/pal/unix/sync/condvar.rs | 99 ++++++++++++++++---- library/std/tests/sync/condvar.rs | 32 +++++++ 4 files changed, 115 insertions(+), 22 deletions(-) diff --git a/library/Cargo.lock b/library/Cargo.lock index 47fbf5169f491..b5b534c986b99 100644 --- a/library/Cargo.lock +++ b/library/Cargo.lock @@ -139,9 +139,9 @@ dependencies = [ [[package]] name = "libc" -version = "0.2.175" +version = "0.2.177" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6a82ae493e598baaea5209805c49bbf2ea7de956d50d7da0da1164f9c6d28543" +checksum = "2874a2af47a2325c2001a6e6fad9b16a53b802102b528163885171cf92b15976" dependencies = [ "rustc-std-workspace-core", ] diff --git a/library/std/Cargo.toml b/library/std/Cargo.toml index 779b07ce240a6..d4108a57acaf0 100644 --- a/library/std/Cargo.toml +++ b/library/std/Cargo.toml @@ -33,7 +33,7 @@ miniz_oxide = { version = "0.8.0", optional = true, default-features = false } addr2line = { version = "0.25.0", optional = true, default-features = false } [target.'cfg(not(all(windows, target_env = "msvc")))'.dependencies] -libc = { version = "0.2.172", default-features = false, features = [ +libc = { version = "0.2.177", default-features = false, features = [ 'rustc-dep-of-std', ], public = true } diff --git a/library/std/src/sys/pal/unix/sync/condvar.rs b/library/std/src/sys/pal/unix/sync/condvar.rs index efa6f8d776559..b6c3ba4136f2a 100644 --- a/library/std/src/sys/pal/unix/sync/condvar.rs +++ b/library/std/src/sys/pal/unix/sync/condvar.rs @@ -1,11 +1,6 @@ use super::Mutex; use crate::cell::UnsafeCell; use crate::pin::Pin; -#[cfg(not(target_os = "nto"))] -use crate::sys::pal::time::TIMESPEC_MAX; -#[cfg(target_os = "nto")] -use crate::sys::pal::time::TIMESPEC_MAX_CAPPED; -use crate::sys::pal::time::Timespec; use crate::time::Duration; pub struct Condvar { @@ -47,27 +42,29 @@ impl Condvar { let r = unsafe { libc::pthread_cond_wait(self.raw(), mutex.raw()) }; debug_assert_eq!(r, 0); } +} +#[cfg(not(target_vendor = "apple"))] +impl Condvar { /// # Safety /// * `init` must have been called on this instance. /// * `mutex` must be locked by the current thread. /// * This condition variable may only be used with the same mutex. pub unsafe fn wait_timeout(&self, mutex: Pin<&Mutex>, dur: Duration) -> bool { + #[cfg(not(target_os = "nto"))] + use crate::sys::pal::time::TIMESPEC_MAX; + #[cfg(target_os = "nto")] + use crate::sys::pal::time::TIMESPEC_MAX_CAPPED; + use crate::sys::pal::time::Timespec; + let mutex = mutex.raw(); - // OSX implementation of `pthread_cond_timedwait` is buggy - // with super long durations. When duration is greater than - // 0x100_0000_0000_0000 seconds, `pthread_cond_timedwait` - // in macOS Sierra returns error 316. - // - // This program demonstrates the issue: - // https://gist.github.com/stepancheg/198db4623a20aad2ad7cddb8fda4a63c - // - // To work around this issue, the timeout is clamped to 1000 years. - // - // Cygwin implementation is based on NT API and a super large timeout - // makes the syscall block forever. - #[cfg(any(target_vendor = "apple", target_os = "cygwin"))] + // Cygwin's implementation is based on the NT API, which measures time + // in units of 100 ns. Unfortunately, Cygwin does not properly guard + // against overflow when converting the time, hence we clamp the interval + // to 1000 years, which will only become a problem in around 27000 years, + // when the next rollover is less than 1000 years away... + #[cfg(target_os = "cygwin")] let dur = Duration::min(dur, Duration::from_secs(1000 * 365 * 86400)); let timeout = Timespec::now(Self::CLOCK).checked_add_duration(&dur); @@ -84,6 +81,57 @@ impl Condvar { } } +// Apple platforms (since macOS version 10.4 and iOS version 2.0) have +// `pthread_cond_timedwait_relative_np`, a non-standard extension that +// measures timeouts based on the monotonic clock and is thus resilient +// against wall-clock changes. +#[cfg(target_vendor = "apple")] +impl Condvar { + /// # Safety + /// * `init` must have been called on this instance. + /// * `mutex` must be locked by the current thread. + /// * This condition variable may only be used with the same mutex. + pub unsafe fn wait_timeout(&self, mutex: Pin<&Mutex>, dur: Duration) -> bool { + let mutex = mutex.raw(); + + // The macOS implementation of `pthread_cond_timedwait` internally + // converts the timeout passed to `pthread_cond_timedwait_relative_np` + // to nanoseconds. Unfortunately, the "psynch" variant of condvars does + // not guard against overflow during the conversion[^1], which means + // that `pthread_cond_timedwait_relative_np` will return `ETIMEDOUT` + // much earlier than expected if the relative timeout is longer than + // `u64::MAX` nanoseconds. + // + // This can be observed even on newer platforms (by setting the environment + // variable PTHREAD_MUTEX_USE_ULOCK to a value other than "1") by calling e.g. + // ``` + // condvar.wait_timeout(..., Duration::from_secs(u64::MAX.div_ceil(1_000_000_000)); + // ``` + // (see #37440, especially + // https://github.com/rust-lang/rust/issues/37440#issuecomment-3285958326). + // + // To work around this issue, always clamp the timeout to u64::MAX nanoseconds, + // even if the "ulock" variant is used (which does guard against overflow). + // + // [^1]: https://github.com/apple-oss-distributions/libpthread/blob/1ebf56b3a702df53213c2996e5e128a535d2577e/kern/kern_synch.c#L1269 + const MAX_DURATION: Duration = Duration::from_nanos(u64::MAX); + + let (dur, clamped) = if dur <= MAX_DURATION { (dur, false) } else { (MAX_DURATION, true) }; + + let timeout = libc::timespec { + // This cannot overflow because of the clamping above. + tv_sec: dur.as_secs() as i64, + tv_nsec: dur.subsec_nanos() as i64, + }; + + let r = unsafe { libc::pthread_cond_timedwait_relative_np(self.raw(), mutex, &timeout) }; + assert!(r == libc::ETIMEDOUT || r == 0); + // Report clamping as a spurious wakeup. Who knows, maybe some + // interstellar space probe will rely on this ;-). + r == 0 || clamped + } +} + #[cfg(not(any( target_os = "android", target_vendor = "apple", @@ -125,10 +173,23 @@ impl Condvar { } } +#[cfg(target_vendor = "apple")] +impl Condvar { + // `pthread_cond_timedwait_relative_np` measures the timeout + // based on the monotonic clock. + pub const PRECISE_TIMEOUT: bool = true; + + /// # Safety + /// May only be called once per instance of `Self`. + pub unsafe fn init(self: Pin<&mut Self>) { + // `PTHREAD_COND_INITIALIZER` is fully supported and we don't need to + // change clocks, so there's nothing to do here. + } +} + // `pthread_condattr_setclock` is unfortunately not supported on these platforms. #[cfg(any( target_os = "android", - target_vendor = "apple", target_os = "espidf", target_os = "horizon", target_os = "l4re", diff --git a/library/std/tests/sync/condvar.rs b/library/std/tests/sync/condvar.rs index 1b1c33efad58e..2a525f9b5e948 100644 --- a/library/std/tests/sync/condvar.rs +++ b/library/std/tests/sync/condvar.rs @@ -267,3 +267,35 @@ nonpoison_and_poison_unwrap_test!( } } ); + +// Some platforms internally cast the timeout duration into nanoseconds. +// If they fail to consider overflow during the conversion (I'm looking +// at you, macOS), `wait_timeout` will return immediately and indicate a +// timeout for durations that are slightly longer than u64::MAX nanoseconds. +// `std` should guard against this by clamping the timeout. +// See #37440 for context. +nonpoison_and_poison_unwrap_test!( + name: timeout_nanoseconds, + test_body: { + use locks::Mutex; + use locks::Condvar; + + let sent = Mutex::new(false); + let cond = Condvar::new(); + + thread::scope(|s| { + s.spawn(|| { + thread::sleep(Duration::from_secs(2)); + maybe_unwrap(sent.set(true)); + cond.notify_all(); + }); + + let guard = maybe_unwrap(sent.lock()); + // If there is internal overflow, this call will return almost + // immediately, before the other thread has reached the `notify_all` + let (guard, res) = maybe_unwrap(cond.wait_timeout(guard, Duration::from_secs(u64::MAX.div_ceil(1_000_000_000)))); + assert!(!res.timed_out()); + assert!(*guard); + }) + } +); From fe1238e69693c4d466d1ec6d8b70e89e220d204b Mon Sep 17 00:00:00 2001 From: joboet Date: Sat, 13 Sep 2025 17:27:54 +0200 Subject: [PATCH 2/2] miri: shim `pthread_cond_timedwait_relative_np` --- .../miri/src/shims/unix/foreign_items.rs | 2 +- .../src/shims/unix/macos/foreign_items.rs | 6 +++ src/tools/miri/src/shims/unix/sync.rs | 22 +++++++--- ...thread_cond_timedwait_relative_np_apple.rs | 41 +++++++++++++++++++ 4 files changed, 64 insertions(+), 7 deletions(-) create mode 100644 src/tools/miri/tests/pass-dep/libc/pthread_cond_timedwait_relative_np_apple.rs diff --git a/src/tools/miri/src/shims/unix/foreign_items.rs b/src/tools/miri/src/shims/unix/foreign_items.rs index 55906f4eb9548..d04ef5eac541b 100644 --- a/src/tools/miri/src/shims/unix/foreign_items.rs +++ b/src/tools/miri/src/shims/unix/foreign_items.rs @@ -815,7 +815,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { "pthread_cond_timedwait" => { let [cond, mutex, abstime] = this.check_shim_sig_lenient(abi, CanonAbi::C, link_name, args)?; - this.pthread_cond_timedwait(cond, mutex, abstime, dest)?; + this.pthread_cond_timedwait(cond, mutex, abstime, dest, /* macos_relative_np */ false)?; } "pthread_cond_destroy" => { let [cond] = this.check_shim_sig_lenient(abi, CanonAbi::C, link_name, args)?; diff --git a/src/tools/miri/src/shims/unix/macos/foreign_items.rs b/src/tools/miri/src/shims/unix/macos/foreign_items.rs index 297d903c6ba4e..0754eb45a2d29 100644 --- a/src/tools/miri/src/shims/unix/macos/foreign_items.rs +++ b/src/tools/miri/src/shims/unix/macos/foreign_items.rs @@ -307,6 +307,12 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { this.os_unfair_lock_assert_not_owner(lock_op)?; } + "pthread_cond_timedwait_relative_np" => { + let [cond, mutex, reltime] = + this.check_shim_sig_lenient(abi, CanonAbi::C, link_name, args)?; + this.pthread_cond_timedwait(cond, mutex, reltime, dest, /* macos_relative_np */ true)?; + } + _ => return interp_ok(EmulateItemResult::NotSupported), }; diff --git a/src/tools/miri/src/shims/unix/sync.rs b/src/tools/miri/src/shims/unix/sync.rs index cefd8b81ac180..a712279d57628 100644 --- a/src/tools/miri/src/shims/unix/sync.rs +++ b/src/tools/miri/src/shims/unix/sync.rs @@ -834,8 +834,9 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { &mut self, cond_op: &OpTy<'tcx>, mutex_op: &OpTy<'tcx>, - abstime_op: &OpTy<'tcx>, + timeout_op: &OpTy<'tcx>, dest: &MPlaceTy<'tcx>, + macos_relative_np: bool, ) -> InterpResult<'tcx> { let this = self.eval_context_mut(); @@ -844,7 +845,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // Extract the timeout. let duration = match this - .read_timespec(&this.deref_pointer_as(abstime_op, this.libc_ty_layout("timespec"))?)? + .read_timespec(&this.deref_pointer_as(timeout_op, this.libc_ty_layout("timespec"))?)? { Some(duration) => duration, None => { @@ -853,14 +854,23 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { return interp_ok(()); } }; - if data.clock == TimeoutClock::RealTime { - this.check_no_isolation("`pthread_cond_timedwait` with `CLOCK_REALTIME`")?; - } + + let (clock, anchor) = if macos_relative_np { + // `pthread_cond_timedwait_relative_np` always measures time against the + // monotonic clock, regardless of the condvar clock. + (TimeoutClock::Monotonic, TimeoutAnchor::Relative) + } else { + if data.clock == TimeoutClock::RealTime { + this.check_no_isolation("`pthread_cond_timedwait` with `CLOCK_REALTIME`")?; + } + + (data.clock, TimeoutAnchor::Absolute) + }; this.condvar_wait( data.condvar_ref, mutex_ref, - Some((data.clock, TimeoutAnchor::Absolute, duration)), + Some((clock, anchor, duration)), Scalar::from_i32(0), this.eval_libc("ETIMEDOUT"), // retval_timeout dest.clone(), diff --git a/src/tools/miri/tests/pass-dep/libc/pthread_cond_timedwait_relative_np_apple.rs b/src/tools/miri/tests/pass-dep/libc/pthread_cond_timedwait_relative_np_apple.rs new file mode 100644 index 0000000000000..bcf06211b8c41 --- /dev/null +++ b/src/tools/miri/tests/pass-dep/libc/pthread_cond_timedwait_relative_np_apple.rs @@ -0,0 +1,41 @@ +//@only-target: apple # `pthread_cond_timedwait_relative_np` is a non-standard extension + +use std::time::Instant; + +// FIXME: remove once this is in libc. +mod libc { + pub use ::libc::*; + unsafe extern "C" { + pub unsafe fn pthread_cond_timedwait_relative_np( + cond: *mut libc::pthread_cond_t, + lock: *mut libc::pthread_mutex_t, + timeout: *const libc::timespec, + ) -> libc::c_int; + } +} + +fn main() { + unsafe { + let mut mutex: libc::pthread_mutex_t = libc::PTHREAD_MUTEX_INITIALIZER; + let mut cond: libc::pthread_cond_t = libc::PTHREAD_COND_INITIALIZER; + + // Wait for 100 ms. + let timeout = libc::timespec { tv_sec: 0, tv_nsec: 100_000_000 }; + + assert_eq!(libc::pthread_mutex_lock(&mut mutex as *mut _), 0); + + let current_time = Instant::now(); + assert_eq!( + libc::pthread_cond_timedwait_relative_np(&mut cond, &mut mutex, &timeout), + libc::ETIMEDOUT + ); + let elapsed_time = current_time.elapsed().as_millis(); + // This is actually deterministic (since isolation remains enabled), + // but can change slightly with Rust updates. + assert!(90 <= elapsed_time && elapsed_time <= 110); + + assert_eq!(libc::pthread_mutex_unlock(&mut mutex), 0); + assert_eq!(libc::pthread_mutex_destroy(&mut mutex), 0); + assert_eq!(libc::pthread_cond_destroy(&mut cond), 0); + } +}