Rollup merge of rust-lang#146503 - joboet:macos-condvar-timeout, r=ibraheemdev

std: improve handling of timed condition variable waits on macOS

Fixes rust-lang#37440 (for good).

This fixes two issues with `Condvar::wait_timeout` on macOS:

Apple's implementation of `pthread_cond_timedwait` internally converts the absolute timeout to a relative one, measured in nanoseconds, but fails to consider overflow when doing so. This results in `wait_timeout` returning much earlier than anticipated when passed a duration that is slightly longer than `u64::MAX` nanoseconds (around 584 years). The existing clamping introduced by rust-lang#42604 to address rust-lang#37440 unfortunately used a maximum duration of 1000 years and thus still runs into the bug when run on older macOS versions (or with `PTHREAD_MUTEX_USE_ULOCK` set to a value other than "1"). See rust-lang#37440 (comment) for context.

Reducing the maximum duration alone however would not be enough to make the implementation completely correct. As macOS does not support `pthread_condattr_setclock`, the deadline passed to `pthread_cond_timedwait` is measured against the wall-time clock. `std` currently calculates the deadline by retrieving the current time and adding the duration to that, only for macOS to convert the deadline back to a relative duration by [retrieving the current time itself](https://github.com/apple-oss-distributions/libpthread/blob/1ebf56b3a702df53213c2996e5e128a535d2577e/src/pthread_cond.c#L802-L819) (this conversion is performed before the aforementioned problematic one). Thus, if the wall-time clock is adjusted between the `std` lookup and the system lookup, the relative duration could have changed, possibly even to a value larger than $2^{64}\ \textrm{ns}$. Luckily however, macOS supports the non-standard, tongue-twisting `pthread_cond_timedwait_relative_np` function which avoids the wall-clock-time roundtrip by taking a relative timeout. Even apart from that, this function is perfectly suited for `std`'s purposes: it is public (albeit badly-documented) API, [available since macOS 10.4](https://github.com/apple-oss-distributions/libpthread/blob/1ebf56b3a702df53213c2996e5e128a535d2577e/include/pthread/pthread.h#L555-L559) (that's way below our minimum of 10.12) and completely resilient against wall-time changes as all timeouts are [measured against the monotonic clock](https://github.com/apple-oss-distributions/xnu/blob/e3723e1f17661b24996789d8afc084c0c3303b26/bsd/kern/sys_ulock.c#L741) inside the kernel.

Thus, this PR switches `Condvar::wait_timeout` to `pthread_cond_timedwait_relative_np`, making sure to clamp the duration to a maximum of $2^{64} - 1 \ \textrm{ns}$. I've added a miri shim as well, so the only thing missing is a definition of `pthread_cond_timedwait_relative_np` inside `libc`.

Author: matthiaskrgr

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",
 ]

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 }
 

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",

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

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)?;

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

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(),

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