[libc++] Remove race condition in std::async

[ldionne]

Summary

The state associated to the future was set in one thread (with synchronization)
but read in another thread without synchronization, which led to a data race.

https://bugs.llvm.org/show_bug.cgi?id=38682
rdar://problem/42548261

[jfb]

libcxx/include/future

556 ↗

(On Diff #162200)

I'm not auditing everything, but it seems like code above can still access state_ without holding mut_? Like in the dtor.

Generally this patch lgtm because it's a step forward, but maybe we should separately refactor the code to make it so that accesses to state_ require passing in a reference to lock_guard to show we actually hold mut_. It would ignore that reference, but that's a way to enforce, in the type system, that __state_ is only touched when the lock is held.

WDYT?

[ldionne]

libcxx/include/future

556 ↗

(On Diff #162200)

I think you're right, and I filed this bug to keep track of the issue: https://bugs.llvm.org/show_bug.cgi?id=38688

Not all of them need a lock (some are in the constructor where only one thread has a reference to the data, for example), but most of them probably do.

libcxx/trunk/include/future

public:
     _LIBCPP_INLINE_VISIBILITY
     __assoc_sub_state() : __state_(0) {}
 
     _LIBCPP_INLINE_VISIBILITY
     bool __has_value() const
         {return (__state_ & __constructed) || (__exception_ != nullptr);}
 
     _LIBCPP_INLINE_VISIBILITY
-    void __set_future_attached()
-    {
+    void __attach_future() {
         lock_guard<mutex> __lk(__mut_);
+        bool __has_future_attached = (__state_ & __future_attached) != 0;
+        if (__has_future_attached)
+            __throw_future_error(future_errc::future_already_retrieved);
+        this->__add_shared();
         __state_ |= __future_attached;
     }
-    _LIBCPP_INLINE_VISIBILITY
-    bool __has_future_attached() const {return (__state_ & __future_attached) != 0;}
 
     _LIBCPP_INLINE_VISIBILITY
     void __set_deferred() {__state_ |= deferred;}
 
     void __make_ready();
     _LIBCPP_INLINE_VISIBILITY
     bool __is_ready() const {return (__state_ & ready) != 0;}
 
template <class _Clock, class _Duration>
         wait_until(const chrono::time_point<_Clock, _Duration>& __abs_time) const
             {return __state_->wait_until(__abs_time);}
 };
 
 template <class _Rp>
 future<_Rp>::future(__assoc_state<_Rp>* __state)
     : __state_(__state)
 {
-    if (__state_->__has_future_attached())
-        __throw_future_error(future_errc::future_already_retrieved);
-    __state_->__add_shared();
-    __state_->__set_future_attached();
+    __state_->__attach_future();
 }
 
 struct __release_shared_count
 {
     void operator()(__shared_count* p) {p->__release_shared();}
 };
 
 template <class _Rp>
template <class _Clock, class _Duration>
         wait_until(const chrono::time_point<_Clock, _Duration>& __abs_time) const
             {return __state_->wait_until(__abs_time);}
 };
 
 template <class _Rp>
 future<_Rp&>::future(__assoc_state<_Rp&>* __state)
     : __state_(__state)
 {
-    if (__state_->__has_future_attached())
-        __throw_future_error(future_errc::future_already_retrieved);
-    __state_->__add_shared();
-    __state_->__set_future_attached();
+    __state_->__attach_future();
 }
 
 template <class _Rp>
 future<_Rp&>::~future()
 {
     if (__state_)
         __state_->__release_shared();
 }

libcxx/trunk/src/future.cpp

 __assoc_sub_state::__execute()
 {
     __throw_future_error(future_errc::no_state);
 }
 
 future<void>::future(__assoc_sub_state* __state)
     : __state_(__state)
 {
-    if (__state_->__has_future_attached())
-        __throw_future_error(future_errc::future_already_retrieved);
-    __state_->__add_shared();
-    __state_->__set_future_attached();
+    __state_->__attach_future();
 }
 
 future<void>::~future()
 {
     if (__state_)
         __state_->__release_shared();
 }
 

libcxx/trunk/test/std/thread/futures/futures.async/async_race.38682.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// UNSUPPORTED: libcpp-has-no-threads
+// UNSUPPORTED: c++98, c++03
+
+// This test is designed to cause and allow TSAN to detect a race condition
+// in std::async, as reported in https://bugs.llvm.org/show_bug.cgi?id=38682.
+
+#include <cassert>
+#include <functional>
+#include <future>
+#include <numeric>
+#include <vector>
+
+
+static int worker(std::vector<int> const& data) {
+  return std::accumulate(data.begin(), data.end(), 0);
+}
+
+static int& worker_ref(int& i) { return i; }
+
+static void worker_void() { }
+
+int main() {
+  // future<T>
+  {
+    std::vector<int> const v{1, 2, 3, 4, 5, 6, 7, 8, 9, 10};
+    for (int i = 0; i != 20; ++i) {
+      std::future<int> fut = std::async(std::launch::async, worker, v);
+      int answer = fut.get();
+      assert(answer == 55);
+    }
+  }
+
+  // future<T&>
+  {
+    for (int i = 0; i != 20; ++i) {
+      std::future<int&> fut = std::async(std::launch::async, worker_ref, std::ref(i));
+      int& answer = fut.get();
+      assert(answer == i);
+    }
+  }
+
+  // future<void>
+  {
+    for (int i = 0; i != 20; ++i) {
+      std::future<void> fut = std::async(std::launch::async, worker_void);
+      fut.get();
+    }
+  }
+}