[libcxx-commits] [libcxx] [libc++] Save duration/timeout locally for condition_variable waits (PR #148330)

[via libcxx-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-libcxx

Author: John Sheu (johnsheu)

<details>
<summary>Changes</summary>

wait_for() and wait_until() for condition_variable and condition_variable_any take their wait duration or timeout parameters by const-reference.  This can lead to unexpected behavior if the referent changes while the wait_for() or wait_until() blocks.

For condition_variable_any, this is an actual data race if the timeout parameter is protected by the lock, as the implementation accesses the timeout outside of the lock.

Fix this by saving a local copy of the duration or timeout while the condition variable is waiting.

---
Full diff: https://github.com/llvm/llvm-project/pull/148330.diff


2 Files Affected:

- (modified) libcxx/include/__condition_variable/condition_variable.h (+10-7) 
- (modified) libcxx/include/condition_variable (+6-3) 


``````````diff
diff --git a/libcxx/include/__condition_variable/condition_variable.h b/libcxx/include/__condition_variable/condition_variable.h
index 1e8edd5dcb009..07f81c532127a 100644
--- a/libcxx/include/__condition_variable/condition_variable.h
+++ b/libcxx/include/__condition_variable/condition_variable.h
@@ -118,21 +118,23 @@ class _LIBCPP_EXPORTED_FROM_ABI condition_variable {
     using namespace chrono;
     using __clock_tp_ns = time_point<_Clock, nanoseconds>;
 
-    typename _Clock::time_point __now = _Clock::now();
+    const typename _Clock::time_point __now = _Clock::now();
     if (__t <= __now)
       return cv_status::timeout;
+    const typename _Clock::time_point __t_local = __t;
 
-    __clock_tp_ns __t_ns = __clock_tp_ns(std::__safe_nanosecond_cast(__t.time_since_epoch()));
+    __clock_tp_ns __t_ns = __clock_tp_ns(std::__safe_nanosecond_cast(__t_local.time_since_epoch()));
 
     __do_timed_wait(__lk, __t_ns);
-    return _Clock::now() < __t ? cv_status::no_timeout : cv_status::timeout;
+    return _Clock::now() < __t_local ? cv_status::no_timeout : cv_status::timeout;
   }
 
   template <class _Clock, class _Duration, class _Predicate>
   _LIBCPP_HIDE_FROM_ABI bool
   wait_until(unique_lock<mutex>& __lk, const chrono::time_point<_Clock, _Duration>& __t, _Predicate __pred) {
+    const typename _Clock::time_point __t_local = __t;
     while (!__pred()) {
-      if (wait_until(__lk, __t) == cv_status::timeout)
+      if (wait_until(__lk, __t_local) == cv_status::timeout)
         return __pred();
     }
     return true;
@@ -144,7 +146,8 @@ class _LIBCPP_EXPORTED_FROM_ABI condition_variable {
     if (__d <= __d.zero())
       return cv_status::timeout;
     using __ns_rep                   = nanoseconds::rep;
-    steady_clock::time_point __c_now = steady_clock::now();
+    const steady_clock::time_point __c_now  = steady_clock::now();
+    const duration<_Rep, _Period> __d_local = __d;
 
 #  if _LIBCPP_HAS_COND_CLOCKWAIT
     using __clock_tp_ns     = time_point<steady_clock, nanoseconds>;
@@ -154,7 +157,7 @@ class _LIBCPP_EXPORTED_FROM_ABI condition_variable {
     __ns_rep __now_count_ns = std::__safe_nanosecond_cast(system_clock::now().time_since_epoch()).count();
 #  endif
 
-    __ns_rep __d_ns_count = std::__safe_nanosecond_cast(__d).count();
+    __ns_rep __d_ns_count = std::__safe_nanosecond_cast(__d_local).count();
 
     if (__now_count_ns > numeric_limits<__ns_rep>::max() - __d_ns_count) {
       __do_timed_wait(__lk, __clock_tp_ns::max());
@@ -162,7 +165,7 @@ class _LIBCPP_EXPORTED_FROM_ABI condition_variable {
       __do_timed_wait(__lk, __clock_tp_ns(nanoseconds(__now_count_ns + __d_ns_count)));
     }
 
-    return steady_clock::now() - __c_now < __d ? cv_status::no_timeout : cv_status::timeout;
+    return steady_clock::now() - __c_now < __d_local ? cv_status::no_timeout : cv_status::timeout;
   }
 
   template <class _Rep, class _Period, class _Predicate>
diff --git a/libcxx/include/condition_variable b/libcxx/include/condition_variable
index 99c74b02807ae..57e995f5224b1 100644
--- a/libcxx/include/condition_variable
+++ b/libcxx/include/condition_variable
@@ -188,9 +188,10 @@ public:
   _LIBCPP_HIDE_FROM_ABI cv_status wait_until(_Lock& __lock, const chrono::time_point<_Clock, _Duration>& __t) {
     shared_ptr<mutex> __mut = __mut_;
     unique_lock<mutex> __lk(*__mut);
+    const chrono::time_point<_Clock, _Duration> __t_local = __t;
     __unlock_guard<_Lock> __unlock(__lock);
     lock_guard<unique_lock<mutex> > __lx(__lk, adopt_lock_t());
-    return __cv_.wait_until(__lk, __t);
+    return __cv_.wait_until(__lk, __t_local);
   } // __mut_.unlock(), __lock.lock()
 
   template <class _Lock, class _Clock, class _Duration, class _Predicate>
@@ -240,8 +241,9 @@ inline void condition_variable_any::wait(_Lock& __lock, _Predicate __pred) {
 template <class _Lock, class _Clock, class _Duration, class _Predicate>
 inline bool
 condition_variable_any::wait_until(_Lock& __lock, const chrono::time_point<_Clock, _Duration>& __t, _Predicate __pred) {
+  const chrono::time_point<_Clock, _Duration> __t_local = __t;
   while (!__pred())
-    if (wait_until(__lock, __t) == cv_status::timeout)
+    if (wait_until(__lock, __t_local) == cv_status::timeout)
       return __pred();
   return true;
 }
@@ -313,6 +315,7 @@ bool condition_variable_any::wait_until(
 
   shared_ptr<mutex> __mut = __mut_;
   stop_callback __cb(__stoken, [this] { notify_all(); });
+  const chrono::time_point<_Clock, _Duration> __abs_time_local = __abs_time;
 
   while (true) {
     if (__pred())
@@ -326,7 +329,7 @@ bool condition_variable_any::wait_until(
     unique_lock<mutex> __internal_lock2(
         std::move(__internal_lock)); // switch unlock order between __internal_lock and __user_lock
 
-    if (__cv_.wait_until(__internal_lock2, __abs_time) == cv_status::timeout)
+    if (__cv_.wait_until(__internal_lock2, __abs_time_local) == cv_status::timeout)
       break;
   } // __internal_lock2.unlock(), __user_lock.lock()
   return __pred();

``````````

</details>


https://github.com/llvm/llvm-project/pull/148330