[Lldb-commits] [lldb] [lldb] Synchronize interrupt and resume signals in debugserver (PR #131073)

[via lldb-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-lldb

Author: Jonas Devlieghere (JDevlieghere)

<details>
<summary>Changes</summary>

This PR fixes a race condition in debugserver where the main thread calls MachProcess::Interrupt, setting `m_sent_interrupt_signo` while the exception monitoring thread is checking value of the variable.

I was on the fence between introducing a new mutex and reusing the existing exception mutex. With the notable exception of MachProcess::Interrupt, all the other places where we were already locking this mutex before accessing the variable. I renamed the mutex to make it clear that it's now protecting more than the exception messages.

Jason, while investigating a real issue, had a suspicion there was race condition related to interrupts and I was able to narrow it down by building debugserver with TSan.

---
Full diff: https://github.com/llvm/llvm-project/pull/131073.diff


2 Files Affected:

- (modified) lldb/tools/debugserver/source/MacOSX/MachProcess.h (+2-2) 
- (modified) lldb/tools/debugserver/source/MacOSX/MachProcess.mm (+12-7) 


``````````diff
diff --git a/lldb/tools/debugserver/source/MacOSX/MachProcess.h b/lldb/tools/debugserver/source/MacOSX/MachProcess.h
index db673693a1b21..d7fa859863162 100644
--- a/lldb/tools/debugserver/source/MacOSX/MachProcess.h
+++ b/lldb/tools/debugserver/source/MacOSX/MachProcess.h
@@ -435,8 +435,8 @@ class MachProcess {
                                                            // caught when
                                                            // listening to the
                                                            // exception port
-  PThreadMutex m_exception_messages_mutex; // Multithreaded protection for
-                                           // m_exception_messages
+  PThreadMutex m_exception_and_signal_mutex; // Multithreaded protection for
+                                           // exceptions and signals.
 
   MachThreadList m_thread_list; // A list of threads that is maintained/updated
                                 // after each stop
diff --git a/lldb/tools/debugserver/source/MacOSX/MachProcess.mm b/lldb/tools/debugserver/source/MacOSX/MachProcess.mm
index a2179bf2f91e5..7dcc04c07bbff 100644
--- a/lldb/tools/debugserver/source/MacOSX/MachProcess.mm
+++ b/lldb/tools/debugserver/source/MacOSX/MachProcess.mm
@@ -528,7 +528,7 @@ static bool FBSAddEventDataToOptions(NSMutableDictionary *options,
       m_profile_data_mutex(PTHREAD_MUTEX_RECURSIVE), m_profile_data(),
       m_profile_events(0, eMachProcessProfileCancel), m_thread_actions(),
       m_exception_messages(),
-      m_exception_messages_mutex(PTHREAD_MUTEX_RECURSIVE), m_thread_list(),
+      m_exception_and_signal_mutex(PTHREAD_MUTEX_RECURSIVE), m_thread_list(),
       m_activities(), m_state(eStateUnloaded),
       m_state_mutex(PTHREAD_MUTEX_RECURSIVE), m_events(0, kAllEventsMask),
       m_private_events(0, kAllEventsMask), m_breakpoints(), m_watchpoints(),
@@ -1338,8 +1338,11 @@ static bool mach_header_validity_test(uint32_t magic, uint32_t cputype) {
   m_stop_count = 0;
   m_thread_list.Clear();
   {
-    PTHREAD_MUTEX_LOCKER(locker, m_exception_messages_mutex);
+    PTHREAD_MUTEX_LOCKER(locker, m_exception_and_signal_mutex);
     m_exception_messages.clear();
+    m_sent_interrupt_signo = 0;
+    m_auto_resume_signo = 0;
+
   }
   m_activities.Clear();
   StopProfileThread();
@@ -1573,6 +1576,8 @@ static uint64_t bits(uint64_t value, uint32_t msbit, uint32_t lsbit) {
 }
 
 bool MachProcess::Interrupt() {
+  PTHREAD_MUTEX_LOCKER(locker, m_exception_and_signal_mutex);
+
   nub_state_t state = GetState();
   if (IsRunning(state)) {
     if (m_sent_interrupt_signo == 0) {
@@ -1728,7 +1733,7 @@ static uint64_t bits(uint64_t value, uint32_t msbit, uint32_t lsbit) {
     m_thread_actions.Append(thread_action);
     m_thread_actions.SetDefaultThreadActionIfNeeded(eStateRunning, 0);
 
-    PTHREAD_MUTEX_LOCKER(locker, m_exception_messages_mutex);
+    PTHREAD_MUTEX_LOCKER(locker, m_exception_and_signal_mutex);
 
     ReplyToAllExceptions();
   }
@@ -1854,7 +1859,7 @@ static uint64_t bits(uint64_t value, uint32_t msbit, uint32_t lsbit) {
 }
 
 void MachProcess::ReplyToAllExceptions() {
-  PTHREAD_MUTEX_LOCKER(locker, m_exception_messages_mutex);
+  PTHREAD_MUTEX_LOCKER(locker, m_exception_and_signal_mutex);
   if (!m_exception_messages.empty()) {
     MachException::Message::iterator pos;
     MachException::Message::iterator begin = m_exception_messages.begin();
@@ -1888,7 +1893,7 @@ static uint64_t bits(uint64_t value, uint32_t msbit, uint32_t lsbit) {
   }
 }
 void MachProcess::PrivateResume() {
-  PTHREAD_MUTEX_LOCKER(locker, m_exception_messages_mutex);
+  PTHREAD_MUTEX_LOCKER(locker, m_exception_and_signal_mutex);
 
   m_auto_resume_signo = m_sent_interrupt_signo;
   if (m_auto_resume_signo)
@@ -2290,7 +2295,7 @@ static uint64_t bits(uint64_t value, uint32_t msbit, uint32_t lsbit) {
 // data has already been copied.
 void MachProcess::ExceptionMessageReceived(
     const MachException::Message &exceptionMessage) {
-  PTHREAD_MUTEX_LOCKER(locker, m_exception_messages_mutex);
+  PTHREAD_MUTEX_LOCKER(locker, m_exception_and_signal_mutex);
 
   if (m_exception_messages.empty())
     m_task.Suspend();
@@ -2304,7 +2309,7 @@ static uint64_t bits(uint64_t value, uint32_t msbit, uint32_t lsbit) {
 
 task_t MachProcess::ExceptionMessageBundleComplete() {
   // We have a complete bundle of exceptions for our child process.
-  PTHREAD_MUTEX_LOCKER(locker, m_exception_messages_mutex);
+  PTHREAD_MUTEX_LOCKER(locker, m_exception_and_signal_mutex);
   DNBLogThreadedIf(LOG_EXCEPTIONS, "%s: %llu exception messages.",
                    __PRETTY_FUNCTION__, (uint64_t)m_exception_messages.size());
   bool auto_resume = false;

``````````

</details>


https://github.com/llvm/llvm-project/pull/131073