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

std: improve handling of timed condition variable waits on macOS

Fixes rust-lang/rust#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/rust#42604 to address rust-lang/rust#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/rust#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

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

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