[vm/concurrency] Refactor SafepointRwLock to ensure unlocking never causes safepoint transition.
The previous implementation of SafepointRwLock has an issue (discovered
by Slava, see bug) with the following code pattern:
{
SafepointReadRwLocker locker(...);
return Foo(); // <-- Returns raw ptr.
}
After the return expression has been evaluated to a raw pointer
the destructor might enter safepoint and cause GC, which leaves
the raw pointer unchanged (since it's not in a handle).
This CL refactors the implementation to ensure we don't perform any
state transition in the destructor, therefore eliminate the
possibility of GC moving the object we return.
Issue https://github.com/dart-lang/sdk/issues/44998
TEST=Relying on existing SafepointRwLock.
Change-Id: Ib7a62b36838edd4b39ad67a8c58f048aa05aa144
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/185062
Commit-Queue: Martin Kustermann <kustermann@google.com>
Reviewed-by: Alexander Aprelev <aam@google.com>
Reviewed-by: Vyacheslav Egorov <vegorov@google.com>
diff --git a/runtime/vm/lockers.cc b/runtime/vm/lockers.cc
index cb04006..30e6ef8 100644
--- a/runtime/vm/lockers.cc
+++ b/runtime/vm/lockers.cc
@@ -86,4 +86,120 @@
}
}
+#if defined(DEBUG)
+bool SafepointRwLock::IsCurrentThreadReader() {
+ ThreadId id = OSThread::GetCurrentThreadId();
+ if (IsCurrentThreadWriter()) {
+ return true;
+ }
+ MonitorLocker ml(&monitor_);
+ for (intptr_t i = readers_ids_.length() - 1; i >= 0; i--) {
+ if (readers_ids_.At(i) == id) {
+ return true;
+ }
+ }
+ return false;
+}
+#endif // defined(DEBUG)
+
+bool SafepointRwLock::EnterRead() {
+ // No need to safepoint if the current thread is not attached.
+ auto thread = Thread::Current();
+ const bool can_block_without_safepoint = thread == nullptr;
+
+ bool acquired_read_lock = false;
+ if (!TryEnterRead(can_block_without_safepoint, &acquired_read_lock)) {
+ // Important: must never hold monitor_ when blocking for safepoint.
+ TransitionVMToBlocked transition(thread);
+ const bool ok = TryEnterRead(/*can_block=*/true, &acquired_read_lock);
+ RELEASE_ASSERT(ok);
+ RELEASE_ASSERT(acquired_read_lock);
+ }
+ return acquired_read_lock;
+}
+
+bool SafepointRwLock::TryEnterRead(bool can_block, bool* acquired_read_lock) {
+ MonitorLocker ml(&monitor_);
+ if (IsCurrentThreadWriter()) {
+ *acquired_read_lock = false;
+ return true;
+ }
+ if (can_block) {
+ while (state_ < 0) {
+ ml.Wait();
+ }
+ }
+ if (state_ >= 0) {
+ ++state_;
+ DEBUG_ONLY(readers_ids_.Add(OSThread::GetCurrentThreadId()));
+ *acquired_read_lock = true;
+ return true;
+ }
+ return false;
+}
+
+void SafepointRwLock::LeaveRead() {
+ MonitorLocker ml(&monitor_);
+ ASSERT(state_ > 0);
+#if defined(DEBUG)
+ {
+ intptr_t i = readers_ids_.length() - 1;
+ ThreadId id = OSThread::GetCurrentThreadId();
+ while (i >= 0) {
+ if (readers_ids_.At(i) == id) {
+ readers_ids_.RemoveAt(i);
+ break;
+ }
+ i--;
+ }
+ ASSERT(i >= 0);
+ }
+#endif
+ if (--state_ == 0) {
+ ml.NotifyAll();
+ }
+}
+
+void SafepointRwLock::EnterWrite() {
+ // No need to safepoint if the current thread is not attached.
+ auto thread = Thread::Current();
+ const bool can_block_without_safepoint = thread == nullptr;
+
+ if (!TryEnterWrite(can_block_without_safepoint)) {
+ // Important: must never hold monitor_ when blocking for safepoint.
+ TransitionVMToBlocked transition(thread);
+ const bool ok = TryEnterWrite(/*can_block=*/true);
+ RELEASE_ASSERT(ok);
+ }
+}
+
+bool SafepointRwLock::TryEnterWrite(bool can_block) {
+ MonitorLocker ml(&monitor_);
+ if (IsCurrentThreadWriter()) {
+ state_--;
+ return true;
+ }
+ if (can_block) {
+ while (state_ != 0) {
+ ml.Wait();
+ }
+ }
+ if (state_ == 0) {
+ writer_id_ = OSThread::GetCurrentThreadId();
+ state_ = -1;
+ return true;
+ }
+ return false;
+}
+
+void SafepointRwLock::LeaveWrite() {
+ MonitorLocker ml(&monitor_);
+ ASSERT(state_ < 0);
+ if (++state_ < 0) {
+ return;
+ }
+ writer_id_ = OSThread::kInvalidThreadId;
+ ml.NotifyAll();
+}
+
} // namespace dart
diff --git a/runtime/vm/lockers.h b/runtime/vm/lockers.h
index d5b2576..7213808 100644
--- a/runtime/vm/lockers.h
+++ b/runtime/vm/lockers.h
@@ -322,21 +322,7 @@
SafepointRwLock() {}
~SafepointRwLock() {}
-#if defined(DEBUG)
- bool IsCurrentThreadReader() {
- ThreadId id = OSThread::GetCurrentThreadId();
- if (IsCurrentThreadWriter()) {
- return true;
- }
- MutexLocker ml(&reader_ids_mutex_);
- for (intptr_t i = readers_ids_.length() - 1; i >= 0; i--) {
- if (readers_ids_.At(i) == id) {
- return true;
- }
- }
- return false;
- }
-#endif // defined(DEBUG)
+ DEBUG_ONLY(bool IsCurrentThreadReader());
bool IsCurrentThreadWriter() {
return writer_id_ == OSThread::GetCurrentThreadId();
@@ -346,82 +332,33 @@
friend class SafepointReadRwLocker;
friend class SafepointWriteRwLocker;
- // returns [true] if read lock was acuired,
+ // returns [true] if read lock was acquired,
// returns [false] if the thread didn't have to acquire read lock due
// to the thread already holding write lock
- bool EnterRead() {
- SafepointMonitorLocker ml(&monitor_);
- if (IsCurrentThreadWriter()) {
- return false;
- }
- while (state_ < 0) {
- ml.Wait();
- }
-#if defined(DEBUG)
- {
- MutexLocker ml(&reader_ids_mutex_);
- readers_ids_.Add(OSThread::GetCurrentThreadId());
- }
-#endif
- ++state_;
- return true;
- }
- void LeaveRead() {
- SafepointMonitorLocker ml(&monitor_);
- ASSERT(state_ > 0);
-#if defined(DEBUG)
- {
- MutexLocker ml(&reader_ids_mutex_);
- intptr_t i = readers_ids_.length() - 1;
- ThreadId id = OSThread::GetCurrentThreadId();
- while (i >= 0) {
- if (readers_ids_.At(i) == id) {
- readers_ids_.RemoveAt(i);
- break;
- }
- i--;
- }
- ASSERT(i >= 0);
- }
-#endif
- if (--state_ == 0) {
- ml.NotifyAll();
- }
- }
+ bool EnterRead();
+ bool TryEnterRead(bool can_block, bool* acquired_read_lock);
+ void LeaveRead();
- void EnterWrite() {
- SafepointMonitorLocker ml(&monitor_);
- if (IsCurrentThreadWriter()) {
- state_--;
- return;
- }
- while (state_ != 0) {
- ml.Wait();
- }
- writer_id_ = OSThread::GetCurrentThreadId();
- state_ = -1;
- }
- void LeaveWrite() {
- SafepointMonitorLocker ml(&monitor_);
- ASSERT(state_ < 0);
- state_++;
- if (state_ < 0) {
- return;
- }
- writer_id_ = OSThread::kInvalidThreadId;
- ml.NotifyAll();
- }
+ void EnterWrite();
+ bool TryEnterWrite(bool can_block);
+ void LeaveWrite();
+ // We maintain an invariant that this monitor is never locked for long periods
+ // of time: Any thread that acquired this monitor must always be able to do
+ // it's work and release it (or wait on the monitor which will also release
+ // it).
+ //
+ // In particular we must ensure the monitor is never held and then a potential
+ // safepoint operation is triggered, since another thread could try to acquire
+ // the lock and it would deadlock.
Monitor monitor_;
+
// [state_] > 0 : The lock is held by multiple readers.
// [state_] == 0 : The lock is free (no readers/writers).
// [state_] < 0 : The lock is held by a single writer (possibly nested).
intptr_t state_ = 0;
-#if defined(DEBUG)
- Mutex reader_ids_mutex_;
- MallocGrowableArray<ThreadId> readers_ids_;
-#endif
+ DEBUG_ONLY(MallocGrowableArray<ThreadId> readers_ids_);
ThreadId writer_id_ = OSThread::kInvalidThreadId;
};
diff --git a/runtime/vm/os_thread.h b/runtime/vm/os_thread.h
index 675836c1..edd66f5 100644
--- a/runtime/vm/os_thread.h
+++ b/runtime/vm/os_thread.h
@@ -377,6 +377,7 @@
friend class MonitorLocker;
friend class SafepointMonitorLocker;
+ friend class SafepointRwLock;
friend void Dart_TestMonitor();
DISALLOW_COPY_AND_ASSIGN(Monitor);
};
