[base] DCHECK threads do not re-lock SharedMutexes

Otherwise, its behaviour is undefined and shouldn't be permitted.

Bug: v8:7790, v8:11135
Change-Id: I9fa5ec02b9126f776ad3ef095e8c70fc5d3620dc
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2547293Reviewed-by: Clemens Backes <clemensb@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
Commit-Queue: Santiago Aboy Solanes <solanes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#71456}

src/base/platform/mutex.cc

@@ -6,9 +6,72 @@
 
 #include <errno.h>
 
+#if DEBUG
+#include <unordered_set>
+#endif  // DEBUG
+
 namespace v8 {
 namespace base {
 
+#if DEBUG
+namespace {
+// Used for asserts to guarantee we are not re-locking a mutex on the same
+// thread. If this thread has only one held shared mutex (common case), we use
+// {single_held_shared_mutex}. If it has more than one we allocate a set for it.
+// Said set has to manually be constructed and destroyed.
+thread_local base::SharedMutex* single_held_shared_mutex = nullptr;
+using TSet = std::unordered_set<base::SharedMutex*>;
+thread_local TSet* held_shared_mutexes = nullptr;
+
+// Returns true iff {shared_mutex} is not a held mutex.
+bool SharedMutexNotHeld(SharedMutex* shared_mutex) {
+  DCHECK_NOT_NULL(shared_mutex);
+  return single_held_shared_mutex != shared_mutex &&
+         (!held_shared_mutexes ||
+          held_shared_mutexes->count(shared_mutex) == 0);
+}
+
+// Tries to hold {shared_mutex}. Returns true iff it hadn't been held prior to
+// this function call.
+bool TryHoldSharedMutex(SharedMutex* shared_mutex) {
+  DCHECK_NOT_NULL(shared_mutex);
+  if (single_held_shared_mutex) {
+    if (shared_mutex == single_held_shared_mutex) {
+      return false;
+    }
+    DCHECK_NULL(held_shared_mutexes);
+    held_shared_mutexes = new TSet({single_held_shared_mutex, shared_mutex});
+    single_held_shared_mutex = nullptr;
+    return true;
+  } else if (held_shared_mutexes) {
+    return held_shared_mutexes->insert(shared_mutex).second;
+  } else {
+    DCHECK_NULL(single_held_shared_mutex);
+    single_held_shared_mutex = shared_mutex;
+    return true;
+  }
+}
+
+// Tries to release {shared_mutex}. Returns true iff it had been held prior to
+// this function call.
+bool TryReleaseSharedMutex(SharedMutex* shared_mutex) {
+  DCHECK_NOT_NULL(shared_mutex);
+  if (single_held_shared_mutex == shared_mutex) {
+    single_held_shared_mutex = nullptr;
+    return true;
+  }
+  if (held_shared_mutexes && held_shared_mutexes->erase(shared_mutex)) {
+    if (held_shared_mutexes->empty()) {
+      delete held_shared_mutexes;
+      held_shared_mutexes = nullptr;
+    }
+    return true;
+  }
+  return false;
+}
+}  // namespace
+#endif  // DEBUG
+
 #if V8_OS_POSIX
 
 static V8_INLINE void InitializeNativeHandle(pthread_mutex_t* mutex) {
@@ -164,18 +227,21 @@ SharedMutex::~SharedMutex() {
 }
 
 void SharedMutex::LockShared() {
+  DCHECK(TryHoldSharedMutex(this));
   int result = pthread_rwlock_rdlock(&native_handle_);
   DCHECK_EQ(0, result);
   USE(result);
 }
 
 void SharedMutex::LockExclusive() {
+  DCHECK(TryHoldSharedMutex(this));
   int result = pthread_rwlock_wrlock(&native_handle_);
   DCHECK_EQ(0, result);
   USE(result);
 }
 
 void SharedMutex::UnlockShared() {
+  DCHECK(TryReleaseSharedMutex(this));
   int result = pthread_rwlock_unlock(&native_handle_);
   DCHECK_EQ(0, result);
   USE(result);
@@ -187,11 +253,17 @@ void SharedMutex::UnlockExclusive() {
 }
 
 bool SharedMutex::TryLockShared() {
-  return pthread_rwlock_tryrdlock(&native_handle_) == 0;
+  DCHECK(SharedMutexNotHeld(this));
+  bool result = pthread_rwlock_tryrdlock(&native_handle_) == 0;
+  if (result) DCHECK(TryHoldSharedMutex(this));
+  return result;
 }
 
 bool SharedMutex::TryLockExclusive() {
-  return pthread_rwlock_trywrlock(&native_handle_) == 0;
+  DCHECK(SharedMutexNotHeld(this));
+  bool result = pthread_rwlock_trywrlock(&native_handle_) == 0;
+  if (result) DCHECK(TryHoldSharedMutex(this));
+  return result;
 }
 
 #elif V8_OS_WIN
@@ -276,22 +348,38 @@ SharedMutex::SharedMutex() : native_handle_(SRWLOCK_INIT) {}
 
 SharedMutex::~SharedMutex() {}
 
-void SharedMutex::LockShared() { AcquireSRWLockShared(&native_handle_); }
+void SharedMutex::LockShared() {
+  DCHECK(TryHoldSharedMutex(this));
+  AcquireSRWLockShared(&native_handle_);
+}
 
-void SharedMutex::LockExclusive() { AcquireSRWLockExclusive(&native_handle_); }
+void SharedMutex::LockExclusive() {
+  DCHECK(TryHoldSharedMutex(this));
+  AcquireSRWLockExclusive(&native_handle_);
+}
 
-void SharedMutex::UnlockShared() { ReleaseSRWLockShared(&native_handle_); }
+void SharedMutex::UnlockShared() {
+  DCHECK(TryReleaseSharedMutex(this));
+  ReleaseSRWLockShared(&native_handle_);
+}
 
 void SharedMutex::UnlockExclusive() {
+  DCHECK(TryReleaseSharedMutex(this));
   ReleaseSRWLockExclusive(&native_handle_);
 }
 
 bool SharedMutex::TryLockShared() {
-  return TryAcquireSRWLockShared(&native_handle_);
+  DCHECK(SharedMutexNotHeld(this));
+  bool result = TryAcquireSRWLockShared(&native_handle_);
+  if (result) DCHECK(TryHoldSharedMutex(this));
+  return result;
 }
 
 bool SharedMutex::TryLockExclusive() {
-  return TryAcquireSRWLockExclusive(&native_handle_);
+  DCHECK(SharedMutexNotHeld(this));
+  bool result = TryAcquireSRWLockExclusive(&native_handle_);
+  if (result) DCHECK(TryHoldSharedMutex(this));
+  return result;
 }
 
 #elif V8_OS_STARBOARD
@@ -318,17 +406,35 @@ SharedMutex::SharedMutex() = default;
 
 SharedMutex::~SharedMutex() = default;
 
-void SharedMutex::LockShared() { native_handle_.AcquireReadLock(); }
+void SharedMutex::LockShared() {
+  DCHECK(TryHoldSharedMutex(this));
+  native_handle_.AcquireReadLock();
+}
 
-void SharedMutex::LockExclusive() { native_handle_.AcquireWriteLock(); }
+void SharedMutex::LockExclusive() {
+  DCHECK(TryHoldSharedMutex(this));
+  native_handle_.AcquireWriteLock();
+}
 
-void SharedMutex::UnlockShared() { native_handle_.ReleaseReadLock(); }
+void SharedMutex::UnlockShared() {
+  DCHECK(TryReleaseSharedMutex(this));
+  native_handle_.ReleaseReadLock();
+}
 
-void SharedMutex::UnlockExclusive() { native_handle_.ReleaseWriteLock(); }
+void SharedMutex::UnlockExclusive() {
+  DCHECK(TryReleaseSharedMutex(this));
+  native_handle_.ReleaseWriteLock();
+}
 
-bool SharedMutex::TryLockShared() { return false; }
+bool SharedMutex::TryLockShared() {
+  DCHECK(SharedMutexNotHeld(this));
+  return false;
+}
 
-bool SharedMutex::TryLockExclusive() { return false; }
+bool SharedMutex::TryLockExclusive() {
+  DCHECK(SharedMutexNotHeld(this));
+  return false;
+}
 #endif  // V8_OS_STARBOARD
 
 }  // namespace base

src/base/platform/mutex.h

@@ -222,36 +222,43 @@ class V8_BASE_EXPORT SharedMutex final {
   // holding the mutex in exclusive ownership, a call to {LockShared()} will
   // block execution until shared ownership can be acquired.
   // If {LockShared()} is called by a thread that already owns the mutex in any
-  // mode (exclusive or shared), the behavior is undefined.
+  // mode (exclusive or shared), the behavior is undefined and outright fails
+  // with dchecks on.
   void LockShared();
 
   // Locks the SharedMutex. If another thread has already locked the mutex, a
   // call to {LockExclusive()} will block execution until the lock is acquired.
   // If {LockExclusive()} is called by a thread that already owns the mutex in
-  // any mode (shared or exclusive), the behavior is undefined.
+  // any mode (shared or exclusive), the behavior is undefined and outright
+  // fails with dchecks on.
   void LockExclusive();
 
   // Releases the {SharedMutex} from shared ownership by the calling thread.
   // The mutex must be locked by the current thread of execution in shared mode,
-  // otherwise, the behavior is undefined.
+  // otherwise, the behavior is undefined and outright fails with dchecks on.
   void UnlockShared();
 
   // Unlocks the {SharedMutex}. It must be locked by the current thread of
-  // execution, otherwise, the behavior is undefined.
+  // execution, otherwise, the behavior is undefined and outright fails with
+  // dchecks on.
   void UnlockExclusive();
 
   // Tries to lock the {SharedMutex} in shared mode. Returns immediately. On
   // successful lock acquisition returns true, otherwise returns false.
   // This function is allowed to fail spuriously and return false even if the
   // mutex is not currenly exclusively locked by any other thread.
+  // If it is called by a thread that already owns the mutex in any mode
+  // (shared or exclusive), the behavior is undefined, and outright fails with
+  // dchecks on.
   bool TryLockShared() V8_WARN_UNUSED_RESULT;
 
   // Tries to lock the {SharedMutex}. Returns immediately. On successful lock
   // acquisition returns true, otherwise returns false.
   // This function is allowed to fail spuriously and return false even if the
   // mutex is not currently locked by any other thread.
-  // If try_lock is called by a thread that already owns the mutex in any mode
-  // (shared or exclusive), the behavior is undefined.
+  // If it is called by a thread that already owns the mutex in any mode
+  // (shared or exclusive), the behavior is undefined, and outright fails with
+  // dchecks on.
   bool TryLockExclusive() V8_WARN_UNUSED_RESULT;
 
  private: