absl: fix use-after-free in Mutex/CondVar

Both Mutex and CondVar signal PerThreadSem/Waiter after satisfying the wait condition,
as the result the waiting thread may return w/o waiting on the
PerThreadSem/Waiter at all. If the waiting thread then exits, it currently
destroys Waiter object. As the result Waiter::Post can be called on
already destroyed object.
PerThreadSem/Waiter must be type-stable after creation and must not be destroyed.
The futex-based implementation is the only one that is not affected by the bug
since there is effectively nothing to destroy (maybe only UBSan/ASan
could complain about calling methods on a destroyed object).

Here is the problematic sequence of events:

1: void Mutex::Block(PerThreadSynch *s) {
2:   while (s->state.load(std::memory_order_acquire) == PerThreadSynch::kQueued) {
3:     if (!DecrementSynchSem(this, s, s->waitp->timeout)) {

4: PerThreadSynch *Mutex::Wakeup(PerThreadSynch *w) {
5:   ...
6:   w->state.store(PerThreadSynch::kAvailable, std::memory_order_release);
7:   IncrementSynchSem(this, w);
8:   ...
9: }

Consider line 6 is executed, then line 2 observes kAvailable and
line 3 is not called. The thread executing Mutex::Block returns from
the method, acquires the mutex, releases the mutex, exits and destroys
PerThreadSem/Waiter.
Now Mutex::Wakeup resumes and executes line 7 on the destroyed object. Boom!

CondVar uses a similar pattern.

Moreover the semaphore-based Waiter implementation is not even destruction-safe
(the Waiter cannot be used to signal own destruction). So even if Mutex/CondVar
would always pair Waiter::Post with Waiter::Wait before destroying PerThreadSem/Waiter,
it would still be subject to use-after-free bug on the semaphore.

PiperOrigin-RevId: 449159939
Change-Id: I497134fa8b6ce1294a422827c5f0de0e897cea31

1 files changed, 26 insertions, 0 deletions

diff --git a/absl/synchronization/mutex_test.cc b/absl/synchronization/mutex_test.cc
index 4f403176..99bb0175 100644
--- a/absl/synchronization/mutex_test.cc
+++ b/absl/synchronization/mutex_test.cc
@@ -1704,4 +1704,30 @@ TEST(Mutex, MuTime) {
   EXPECT_EQ(RunTest(&TestMuTime, threads, iterations, 1), threads * iterations);
 }
 
+TEST(Mutex, SignalExitedThread) {
+  // The test may expose a race when Mutex::Unlock signals a thread
+  // that has already exited.
+#if defined(__wasm__) || defined(__asmjs__)
+  constexpr int kThreads = 1;  // OOMs under WASM
+#else
+  constexpr int kThreads = 100;
+#endif
+  std::vector<std::thread> top;
+  for (unsigned i = 0; i < 2 * std::thread::hardware_concurrency(); i++) {
+    top.emplace_back([&]() {
+      for (int i = 0; i < kThreads; i++) {
+        absl::Mutex mu;
+        std::thread t([&]() {
+          mu.Lock();
+          mu.Unlock();
+        });
+        mu.Lock();
+        mu.Unlock();
+        t.join();
+      }
+    });
+  }
+  for (auto &th : top) th.join();
+}
+
 }  // namespace