[llvm-bugs] [Bug 38682] New: Race condition in __make_async_assoc_state

[via llvm-bugs]

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

            Bug ID: 38682
           Summary: Race condition in __make_async_assoc_state
           Product: libc++
           Version: unspecified
          Hardware: PC
                OS: All
            Status: NEW
          Severity: enhancement
          Priority: P
         Component: All Bugs
          Assignee: unassignedclangbugs at nondot.org
          Reporter: ldionne at apple.com
                CC: llvm-bugs at lists.llvm.org, mclow.lists at gmail.com

The following program exhibits a race condition, which can be seen by running
it under Tsan:

------------------------------------------------------------------------------
#include <cassert>
#include <future>
#include <numeric>
#include <vector>


static int worker(std::vector<int> const& data) {
  return std::accumulate(data.begin(), data.end(), 0);
}

int main() {
  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);
    std::future<int> fut2 = std::async(std::launch::async, worker, v);
    int answer = fut.get();
    int answer2 = fut2.get();
    assert(answer == answer2);
  }
}
------------------------------------------------------------------------------

$ clang++ -std=c++11 -fsanitize=thread main.cpp -o main.exe && ./main.exe

------------------------------------------------------------------------------
WARNING: ThreadSanitizer: data race (pid=97622)
  Write of size 4 at 0x7b2c00000088 by thread T1 (mutexes: write M14):
    #0 void std::__1::__assoc_state<int>::set_value<int>(int&&) <null>:1062560
(main.exe:x86_64+0x10000556b)
    #1 std::__1::__async_assoc_state<int, std::__1::__async_func<int
(*)(std::__1::vector<int, std::__1::allocator<int> > const&),
std::__1::vector<int, std::__1::allocator<int> > > >::__execute()
<null>:1062560 (main.exe:x86_64+0x1000047de)
    #2 void*
std::__1::__thread_proxy<std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct,
std::__1::default_delete<std::__1::__thread_struct> >, void
(std::__1::__async_assoc_state<int, std::__1::__async_func<int
(*)(std::__1::vector<int, std::__1::allocator<int> > const&),
std::__1::vector<int, std::__1::allocator<int> > > >::*)(),
std::__1::__async_assoc_state<int, std::__1::__async_func<int
(*)(std::__1::vector<int, std::__1::allocator<int> > const&),
std::__1::vector<int, std::__1::allocator<int> > > >*> >(void*) <null>:1062560
(main.exe:x86_64+0x100006b82)

  Previous read of size 4 at 0x7b2c00000088 by main thread:
    #0 std::__1::future<int>::future(std::__1::__assoc_state<int>*)
<null>:1062560 (main.exe:x86_64+0x1000071f1)
    #1 std::__1::future<int>::future(std::__1::__assoc_state<int>*)
<null>:1062560 (main.exe:x86_64+0x1000043c8)
    #2 std::__1::future<int> std::__1::__make_async_assoc_state<int,
std::__1::__async_func<int (*)(std::__1::vector<int, std::__1::allocator<int> >
const&), std::__1::vector<int, std::__1::allocator<int> > >
>(std::__1::__async_func<int (*)(std::__1::vector<int, std::__1::allocator<int>
> const&), std::__1::vector<int, std::__1::allocator<int> > >&&) <null>:1062560
(main.exe:x86_64+0x100003a13)
    #3 std::__1::future<std::__1::__invoke_of<std::__1::decay<int
(&)(std::__1::vector<int, std::__1::allocator<int> > const&)>::type,
std::__1::decay<std::__1::vector<int, std::__1::allocator<int> >
const&>::type>::type> std::__1::async<int (&)(std::__1::vector<int,
std::__1::allocator<int> > const&), std::__1::vector<int,
std::__1::allocator<int> > const&>(std::__1::launch, int
(&&&)(std::__1::vector<int, std::__1::allocator<int> > const&),
std::__1::vector<int, std::__1::allocator<int> > const&&&) <null>:1062560
(main.exe:x86_64+0x10000196a)
    #4 main <null>:1062560 (main.exe:x86_64+0x10000104c)

  Location is heap block of size 176 at 0x7b2c00000000 allocated by main
thread:
    #0 operator new(unsigned long) <null>:1062592
(libclang_rt.tsan_osx_dynamic.dylib:x86_64h+0x6e733)
    #1 std::__1::future<int> std::__1::__make_async_assoc_state<int,
std::__1::__async_func<int (*)(std::__1::vector<int, std::__1::allocator<int> >
const&), std::__1::vector<int, std::__1::allocator<int> > >
>(std::__1::__async_func<int (*)(std::__1::vector<int, std::__1::allocator<int>
> const&), std::__1::vector<int, std::__1::allocator<int> > >&&) <null>:1062592
(main.exe:x86_64+0x1000036c8)
    #2 std::__1::future<std::__1::__invoke_of<std::__1::decay<int
(&)(std::__1::vector<int, std::__1::allocator<int> > const&)>::type,
std::__1::decay<std::__1::vector<int, std::__1::allocator<int> >
const&>::type>::type> std::__1::async<int (&)(std::__1::vector<int,
std::__1::allocator<int> > const&), std::__1::vector<int,
std::__1::allocator<int> > const&>(std::__1::launch, int
(&&&)(std::__1::vector<int, std::__1::allocator<int> > const&),
std::__1::vector<int, std::__1::allocator<int> > const&&&) <null>:1062592
(main.exe:x86_64+0x10000196a)
    #3 main <null>:1062592 (main.exe:x86_64+0x10000104c)

  Mutex M14 (0x7b2c00000018) created at:
    #0 pthread_mutex_lock <null>:1062448
(libclang_rt.tsan_osx_dynamic.dylib:x86_64h+0x3945e)
    #1 std::__1::mutex::lock() <null>:1062448 (libc++.1.dylib:x86_64+0x3a698)
    #2 std::__1::__async_assoc_state<int, std::__1::__async_func<int
(*)(std::__1::vector<int, std::__1::allocator<int> > const&),
std::__1::vector<int, std::__1::allocator<int> > > >::__execute()
<null>:1062448 (main.exe:x86_64+0x1000047de)
    #3 void*
std::__1::__thread_proxy<std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct,
std::__1::default_delete<std::__1::__thread_struct> >, void
(std::__1::__async_assoc_state<int, std::__1::__async_func<int
(*)(std::__1::vector<int, std::__1::allocator<int> > const&),
std::__1::vector<int, std::__1::allocator<int> > > >::*)(),
std::__1::__async_assoc_state<int, std::__1::__async_func<int
(*)(std::__1::vector<int, std::__1::allocator<int> > const&),
std::__1::vector<int, std::__1::allocator<int> > > >*> >(void*) <null>:1062448
(main.exe:x86_64+0x100006b82)

  Thread T1 (tid=572648, running) created by main thread at:
    #0 pthread_create <null>:1062640
(libclang_rt.tsan_osx_dynamic.dylib:x86_64h+0x2a13d)
    #1 std::__1::thread::thread<void (std::__1::__async_assoc_state<int,
std::__1::__async_func<int (*)(std::__1::vector<int, std::__1::allocator<int> >
const&), std::__1::vector<int, std::__1::allocator<int> > > >::*)(),
std::__1::__async_assoc_state<int, std::__1::__async_func<int
(*)(std::__1::vector<int, std::__1::allocator<int> > const&),
std::__1::vector<int, std::__1::allocator<int> > > >*, void>(void
(std::__1::__async_assoc_state<int, std::__1::__async_func<int
(*)(std::__1::vector<int, std::__1::allocator<int> > const&),
std::__1::vector<int, std::__1::allocator<int> > > >::*&&)(),
std::__1::__async_assoc_state<int, std::__1::__async_func<int
(*)(std::__1::vector<int, std::__1::allocator<int> > const&),
std::__1::vector<int, std::__1::allocator<int> > > >*&&) <null>:1062640
(main.exe:x86_64+0x1000060d0)
    #2 std::__1::thread::thread<void (std::__1::__async_assoc_state<int,
std::__1::__async_func<int (*)(std::__1::vector<int, std::__1::allocator<int> >
const&), std::__1::vector<int, std::__1::allocator<int> > > >::*)(),
std::__1::__async_assoc_state<int, std::__1::__async_func<int
(*)(std::__1::vector<int, std::__1::allocator<int> > const&),
std::__1::vector<int, std::__1::allocator<int> > > >*, void>(void
(std::__1::__async_assoc_state<int, std::__1::__async_func<int
(*)(std::__1::vector<int, std::__1::allocator<int> > const&),
std::__1::vector<int, std::__1::allocator<int> > > >::*&&)(),
std::__1::__async_assoc_state<int, std::__1::__async_func<int
(*)(std::__1::vector<int, std::__1::allocator<int> > const&),
std::__1::vector<int, std::__1::allocator<int> > > >*&&) <null>:1062640
(main.exe:x86_64+0x100004358)
    #3 std::__1::future<int> std::__1::__make_async_assoc_state<int,
std::__1::__async_func<int (*)(std::__1::vector<int, std::__1::allocator<int> >
const&), std::__1::vector<int, std::__1::allocator<int> > >
>(std::__1::__async_func<int (*)(std::__1::vector<int, std::__1::allocator<int>
> const&), std::__1::vector<int, std::__1::allocator<int> > >&&) <null>:1062640
(main.exe:x86_64+0x1000039b0)
    #4 std::__1::future<std::__1::__invoke_of<std::__1::decay<int
(&)(std::__1::vector<int, std::__1::allocator<int> > const&)>::type,
std::__1::decay<std::__1::vector<int, std::__1::allocator<int> >
const&>::type>::type> std::__1::async<int (&)(std::__1::vector<int,
std::__1::allocator<int> > const&), std::__1::vector<int,
std::__1::allocator<int> > const&>(std::__1::launch, int
(&&&)(std::__1::vector<int, std::__1::allocator<int> > const&),
std::__1::vector<int, std::__1::allocator<int> > const&&&) <null>:1062640
(main.exe:x86_64+0x10000196a)
    #5 main <null>:1062640 (main.exe:x86_64+0x10000104c)

SUMMARY: ThreadSanitizer: data race (main.exe:x86_64+0x10000556b) in void
std::__1::__assoc_state<int>::set_value<int>(int&&)
------------------------------------------------------------------------------

The race seems to be between the following functions:

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

and:

    template <class _Rp>
    template <class _Arg>
    void __assoc_state<_Rp>::set_value(_Arg&& __arg) {
        unique_lock<mutex> __lk(this->__mut_);
        if (this->__has_value())
            __throw_future_error(future_errc::promise_already_satisfied);
        ::new(&__value_) _Rp(_VSTD::forward<_Arg>(__arg));
        this->__state_ |= base::__constructed | base::ready;
        __cv_.notify_all();
    }

Those functions are run concurrently from __make_async_assoc_state:

    template <class _Rp, class _Fp>
    future<_Rp> __make_async_assoc_state(_Fp&& __f) {
        unique_ptr<__async_assoc_state<_Rp, _Fp>, __release_shared_count>
            __h(new __async_assoc_state<_Rp, _Fp>(_VSTD::forward<_Fp>(__f)));
        _VSTD::thread([h = __h.get()]{
            h->set_value(h->__func_()); // call to set_value()
        }).detach();
        return future<_Rp>(__h.get()); // call to future's constructor
    }

The problem is that the constructor reads the state of the future (with
__has_future_attached()) without synchronization, and set_value() sets that
state concurrently. set_value() properly acquires the mutex, but the
constructor does not.

My proposed resolution would be to take a lock in the constructor at the very
beginning, like so:

    template <class _Rp>
    future<_Rp>::future(__assoc_state<_Rp>* __state)
        : __state_(__state)
    {
        unique_lock<mutex> __lk(__state_->__mut_);
        if (__state_->__has_future_attached())
            __throw_future_error(future_errc::future_already_retrieved);
        __state_->__add_shared();
        __state_->__set_future_attached();
    }

-- 
You are receiving this mail because:
You are on the CC list for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-bugs/attachments/20180823/23eb5b32/attachment-0001.html>