[libcxx-commits] [libcxx] 64fc3cd - [libc++] Hold mutex lock while notify_all is called at notify_all_at_thread_exit
Louis Dionne via libcxx-commits
libcxx-commits at lists.llvm.org
Wed Jan 11 14:01:38 PST 2023
Author: Arthur O'Dwyer
Date: 2023-01-11T17:01:21-05:00
New Revision: 64fc3cd55d586498dd21c5b3cfaa755793913772
URL: https://github.com/llvm/llvm-project/commit/64fc3cd55d586498dd21c5b3cfaa755793913772
DIFF: https://github.com/llvm/llvm-project/commit/64fc3cd55d586498dd21c5b3cfaa755793913772.diff
LOG: [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
Added:
libcxx/test/std/thread/thread.condition/notify_all_at_thread_exit_lwg3343.pass.cpp
Modified:
libcxx/docs/Status/Cxx2bIssues.csv
libcxx/src/thread.cpp
libcxx/test/std/thread/thread.condition/notify_all_at_thread_exit.pass.cpp
Removed:
################################################################################
diff --git a/libcxx/docs/Status/Cxx2bIssues.csv b/libcxx/docs/Status/Cxx2bIssues.csv
index 1b8a359453f80..2acfc800baf36 100644
--- 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 ce2822dbac85e..ec4f65f9823b2 100644
--- 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 19da74ca65735..1f23c13ddeb89 100644
--- 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
index 0000000000000..7e3b2d304d959
--- /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;
+}
More information about the libcxx-commits
mailing list