[libc++] Hold mutex lock while notify_all is called at notify_all_at_thread_exit

Releasing the mutex before the call to notify_all is an optimization.
This optimization cannot be used here. The thread waiting on the
condition might destroy the associated resources — mutex + condition
variable — and the notifier thread will access an destroyed variable
— the condition variable. In fact, notify_all_at_thread_exit is meant
exactly to join on detached threads, and the waiting thread doesn't
expect for the notifier thread to access any further shared resources,
making this scenario very likely to happen. The waiting thread might
awake spuriously on the release of the mutex lock. The reorder is
necessary to prevent this race.

Further details can be found at https://cplusplus.github.io/LWG/issue3343.

Differential Revision: https://reviews.llvm.org/D105758

diff --git a/libcxx/docs/Status/Cxx2bIssues.csv b/libcxx/docs/Status/Cxx2bIssues.csv
index 1b8a359..2acfc80 100644 (file)
--- a/libcxx/docs/Status/Cxx2bIssues.csv
+++ b/libcxx/docs/Status/Cxx2bIssues.csv
@@ -247,3 +247,4 @@
 "","","","","",""
 "`3631 <https://wg21.link/LWG3631>`__","``basic_format_arg(T&&)`` should use ``remove_cvref_t<T>`` throughout","Not voted in","|Complete|","15.0",""
 "`3645 <https://wg21.link/LWG3645>`__","``resize_and_overwrite`` is overspecified to call its callback with lvalues","Not voted in","|Complete|","14.0",""
+"`3343 <https://wg21.link/LWG3343>`__","Ordering of calls to ``unlock()`` and ``notify_all()`` in Effects element of ``notify_all_at_thread_exit()`` should be reversed","Not Yet Adopted","|Complete|","16.0",""

diff --git a/libcxx/src/thread.cpp b/libcxx/src/thread.cpp
index ce2822d..ec4f65f 100644 (file)
--- a/libcxx/src/thread.cpp
+++ b/libcxx/src/thread.cpp
@@ -164,8 +164,8 @@ __thread_struct_imp::~__thread_struct_imp()
     for (_Notify::iterator i = notify_.begin(), e = notify_.end();
             i != e; ++i)
     {
-        i->second->unlock();
         i->first->notify_all();
+        i->second->unlock();
     }
     for (_AsyncStates::iterator i = async_states_.begin(), e = async_states_.end();
             i != e; ++i)

diff --git a/libcxx/test/std/thread/thread.condition/notify_all_at_thread_exit.pass.cpp b/libcxx/test/std/thread/thread.condition/notify_all_at_thread_exit.pass.cpp
index 19da74c..1f23c13 100644 (file)
--- a/libcxx/test/std/thread/thread.condition/notify_all_at_thread_exit.pass.cpp
+++ b/libcxx/test/std/thread/thread.condition/notify_all_at_thread_exit.pass.cpp
@@ -8,14 +8,12 @@
 //
 // UNSUPPORTED: no-threads
 
-// notify_all_at_thread_exit(...) requires move semantics to transfer the
-// unique_lock.
+// notify_all_at_thread_exit(...) requires move semantics to transfer the unique_lock.
 // UNSUPPORTED: c++03
 
 // <condition_variable>
-
-// void
-//   notify_all_at_thread_exit(condition_variable& cond, unique_lock<mutex> lk);
+//
+// void notify_all_at_thread_exit(condition_variable& cond, unique_lock<mutex> lk);
 
 #include <condition_variable>
 #include <mutex>

diff --git a/libcxx/test/std/thread/thread.condition/notify_all_at_thread_exit_lwg3343.pass.cpp b/libcxx/test/std/thread/thread.condition/notify_all_at_thread_exit_lwg3343.pass.cpp
new file mode 100644 (file)
index 0000000..7e3b2d3
--- /dev/null
+++ b/libcxx/test/std/thread/thread.condition/notify_all_at_thread_exit_lwg3343.pass.cpp
@@ -0,0 +1,94 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// UNSUPPORTED: no-threads
+
+// notify_all_at_thread_exit(...) requires move semantics to transfer the unique_lock.
+// UNSUPPORTED: c++03
+
+// This is a regression test for LWG3343.
+//
+// <condition_variable>
+//
+// void notify_all_at_thread_exit(condition_variable& cond, unique_lock<mutex> lk);
+
+#include "make_test_thread.h"
+#include "test_macros.h"
+
+#include <condition_variable>
+#include <cassert>
+#include <chrono>
+#include <memory>
+#include <mutex>
+#include <thread>
+
+union X {
+    X() : cv_() {}
+    ~X() {}
+    std::condition_variable cv_;
+    unsigned char bytes_[sizeof(std::condition_variable)];
+};
+
+void test()
+{
+    constexpr int N = 3;
+
+    X x;
+    std::mutex m;
+    int threads_active = N;
+
+    for (int i = 0; i < N; ++i) {
+        std::thread t = support::make_test_thread([&] {
+            // Signal thread completion
+            std::unique_lock<std::mutex> lk(m);
+            --threads_active;
+            std::notify_all_at_thread_exit(x.cv_, std::move(lk));
+            std::this_thread::sleep_for(std::chrono::milliseconds(1));
+        });
+        t.detach();
+    }
+
+    // Wait until all threads complete, i.e. until they've all
+    // decremented `threads_active` and then unlocked `m` at thread exit.
+    // It is possible that this `wait` may spuriously wake up,
+    // but it won't be able to continue until the last thread
+    // unlocks `m`.
+    {
+        std::unique_lock<std::mutex> lk(m);
+        x.cv_.wait(lk, [&]() { return threads_active == 0; });
+    }
+
+    // Destroy the condition_variable and shred the bytes.
+    // Simulate reusing the memory for something else.
+    x.cv_.~condition_variable();
+    for (unsigned char& c : x.bytes_) {
+        c = 0xcd;
+    }
+
+    DoNotOptimize(x.bytes_);
+
+    // Check that the bytes still have the same value we just wrote to them.
+    // If any thread wrongly unlocked `m` before calling cv.notify_all(), and
+    // cv.notify_all() writes to the memory of the cv, then we have a chance
+    // to detect the problem here.
+    int sum = 0;
+    for (unsigned char c : x.bytes_) {
+       sum += c;
+    }
+    DoNotOptimize(sum);
+    assert(sum == (0xcd * sizeof(std::condition_variable)));
+}
+
+int main(int, char**)
+{
+    for (int i = 0; i < 1000; ++i) {
+        test();
+    }
+
+    return 0;
+}