[lldb] Fix ThreadList assignment race (#98293)

labath · web-flow · commit e1bd337865fc · 2024-07-11T14:04:19.000+02:00
ThreadList uses the Process mutex to guard its state. This means its not
possible to safely modify its process member, as the member is required
to lock the mutex.

Fortunately for us, we never actually need to change the process member
(we always just juggle different kinds of thread lists belonging to the
same process).

This patch replaces the process member assignment (which is technically
a race even when it assigns the same value) with an assertion.

Since all this means that the class can never change its process member
value (and it also must be non-null at all times), I've also changed the
member type to a reference.
diff --git a/lldb/include/lldb/Target/ThreadList.h b/lldb/include/lldb/Target/ThreadList.h
@@ -27,12 +27,13 @@ class ThreadList : public ThreadCollection {
   friend class Process;
 
 public:
-  ThreadList(Process *process);
+  ThreadList(Process &process);
 
   ThreadList(const ThreadList &rhs);
 
   ~ThreadList() override;
 
+  /// Precondition: both thread lists must be belong to the same process.
   const ThreadList &operator=(const ThreadList &rhs);
 
   uint32_t GetSize(bool can_update = true);
@@ -135,6 +136,7 @@ class ThreadList : public ThreadCollection {
 
   std::recursive_mutex &GetMutex() const override;
 
+  /// Precondition: both thread lists must be belong to the same process.
   void Update(ThreadList &rhs);
 
 protected:
@@ -143,7 +145,7 @@ class ThreadList : public ThreadCollection {
   void NotifySelectedThreadChanged(lldb::tid_t tid);
 
   // Classes that inherit from Process can see and modify these
-  Process *m_process; ///< The process that manages this thread list.
+  Process &m_process; ///< The process that manages this thread list.
   uint32_t
       m_stop_id; ///< The process stop ID that this thread list is valid for.
   lldb::tid_t
diff --git a/lldb/source/Plugins/OperatingSystem/Python/OperatingSystemPython.cpp b/lldb/source/Plugins/OperatingSystem/Python/OperatingSystemPython.cpp
@@ -372,7 +372,7 @@ lldb::ThreadSP OperatingSystemPython::CreateThread(lldb::tid_t tid,
 
     std::vector<bool> core_used_map;
     if (thread_info_dict) {
-      ThreadList core_threads(m_process);
+      ThreadList core_threads(*m_process);
       ThreadList &thread_list = m_process->GetThreadList();
       bool did_create = false;
       ThreadSP thread_sp(
diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
@@ -460,12 +460,10 @@ Process::Process(lldb::TargetSP target_sp, ListenerSP listener_sp,
       m_private_state_listener_sp(
           Listener::MakeListener("lldb.process.internal_state_listener")),
       m_mod_id(), m_process_unique_id(0), m_thread_index_id(0),
-      m_thread_id_to_index_id_map(), m_exit_status(-1), m_exit_string(),
-      m_exit_status_mutex(), m_thread_mutex(), m_thread_list_real(this),
-      m_thread_list(this), m_thread_plans(*this), m_extended_thread_list(this),
-      m_extended_thread_stop_id(0), m_queue_list(this), m_queue_list_stop_id(0),
-      m_watchpoint_resource_list(), m_notifications(), m_image_tokens(),
-      m_breakpoint_site_list(), m_dynamic_checkers_up(),
+      m_thread_id_to_index_id_map(), m_exit_status(-1),
+      m_thread_list_real(*this), m_thread_list(*this), m_thread_plans(*this),
+      m_extended_thread_list(*this), m_extended_thread_stop_id(0),
+      m_queue_list(this), m_queue_list_stop_id(0),
       m_unix_signals_sp(unix_signals_sp), m_abi_sp(), m_process_input_reader(),
       m_stdio_communication("process.stdio"), m_stdio_communication_mutex(),
       m_stdin_forward(false), m_stdout_data(), m_stderr_data(),
@@ -1183,8 +1181,8 @@ void Process::UpdateThreadListIfNeeded() {
       // mutex between the call to UpdateThreadList(...) and the
       // os->UpdateThreadList(...) so it doesn't change on us
       ThreadList &old_thread_list = m_thread_list;
-      ThreadList real_thread_list(this);
-      ThreadList new_thread_list(this);
+      ThreadList real_thread_list(*this);
+      ThreadList new_thread_list(*this);
       // Always update the thread list with the protocol specific thread list,
       // but only update if "true" is returned
       if (UpdateThreadList(m_thread_list_real, real_thread_list)) {
diff --git a/lldb/source/Target/ThreadList.cpp b/lldb/source/Target/ThreadList.cpp
@@ -23,7 +23,7 @@
 using namespace lldb;
 using namespace lldb_private;
 
-ThreadList::ThreadList(Process *process)
+ThreadList::ThreadList(Process &process)
     : ThreadCollection(), m_process(process), m_stop_id(0),
       m_selected_tid(LLDB_INVALID_THREAD_ID) {}
 
@@ -36,14 +36,13 @@ ThreadList::ThreadList(const ThreadList &rhs)
 
 const ThreadList &ThreadList::operator=(const ThreadList &rhs) {
   if (this != &rhs) {
-    // Lock both mutexes to make sure neither side changes anyone on us while
-    // the assignment occurs
-    std::lock(GetMutex(), rhs.GetMutex());
-    std::lock_guard<std::recursive_mutex> guard(GetMutex(), std::adopt_lock);
-    std::lock_guard<std::recursive_mutex> rhs_guard(rhs.GetMutex(), 
-                                                    std::adopt_lock);
-
-    m_process = rhs.m_process;
+    // We only allow assignments between thread lists describing the same
+    // process. Same process implies same mutex, which means it's enough to lock
+    // just the current object.
+    assert(&m_process == &rhs.m_process);
+    assert(&GetMutex() == &rhs.GetMutex());
+    std::lock_guard<std::recursive_mutex> guard(GetMutex());
+
     m_stop_id = rhs.m_stop_id;
     m_threads = rhs.m_threads;
     m_selected_tid = rhs.m_selected_tid;
@@ -84,15 +83,15 @@ uint32_t ThreadList::GetSize(bool can_update) {
   std::lock_guard<std::recursive_mutex> guard(GetMutex());
 
   if (can_update)
-    m_process->UpdateThreadListIfNeeded();
+    m_process.UpdateThreadListIfNeeded();
   return m_threads.size();
 }
 
 ThreadSP ThreadList::GetThreadAtIndex(uint32_t idx, bool can_update) {
   std::lock_guard<std::recursive_mutex> guard(GetMutex());
 
   if (can_update)
-    m_process->UpdateThreadListIfNeeded();
+    m_process.UpdateThreadListIfNeeded();
 
   ThreadSP thread_sp;
   if (idx < m_threads.size())
@@ -104,7 +103,7 @@ ThreadSP ThreadList::FindThreadByID(lldb::tid_t tid, bool can_update) {
   std::lock_guard<std::recursive_mutex> guard(GetMutex());
 
   if (can_update)
-    m_process->UpdateThreadListIfNeeded();
+    m_process.UpdateThreadListIfNeeded();
 
   ThreadSP thread_sp;
   uint32_t idx = 0;
@@ -122,7 +121,7 @@ ThreadSP ThreadList::FindThreadByProtocolID(lldb::tid_t tid, bool can_update) {
   std::lock_guard<std::recursive_mutex> guard(GetMutex());
 
   if (can_update)
-    m_process->UpdateThreadListIfNeeded();
+    m_process.UpdateThreadListIfNeeded();
 
   ThreadSP thread_sp;
   uint32_t idx = 0;
@@ -140,7 +139,7 @@ ThreadSP ThreadList::RemoveThreadByID(lldb::tid_t tid, bool can_update) {
   std::lock_guard<std::recursive_mutex> guard(GetMutex());
 
   if (can_update)
-    m_process->UpdateThreadListIfNeeded();
+    m_process.UpdateThreadListIfNeeded();
 
   ThreadSP thread_sp;
   uint32_t idx = 0;
@@ -160,7 +159,7 @@ ThreadSP ThreadList::RemoveThreadByProtocolID(lldb::tid_t tid,
   std::lock_guard<std::recursive_mutex> guard(GetMutex());
 
   if (can_update)
-    m_process->UpdateThreadListIfNeeded();
+    m_process.UpdateThreadListIfNeeded();
 
   ThreadSP thread_sp;
   uint32_t idx = 0;
@@ -210,7 +209,7 @@ ThreadSP ThreadList::FindThreadByIndexID(uint32_t index_id, bool can_update) {
   std::lock_guard<std::recursive_mutex> guard(GetMutex());
 
   if (can_update)
-    m_process->UpdateThreadListIfNeeded();
+    m_process.UpdateThreadListIfNeeded();
 
   ThreadSP thread_sp;
   const uint32_t num_threads = m_threads.size();
@@ -241,7 +240,7 @@ bool ThreadList::ShouldStop(Event *event_ptr) {
     // Scope for locker
     std::lock_guard<std::recursive_mutex> guard(GetMutex());
 
-    m_process->UpdateThreadListIfNeeded();
+    m_process.UpdateThreadListIfNeeded();
     for (lldb::ThreadSP thread_sp : m_threads) {
       // This is an optimization...  If we didn't let a thread run in between
       // the previous stop and this one, we shouldn't have to consult it for
@@ -377,7 +376,7 @@ Vote ThreadList::ShouldReportStop(Event *event_ptr) {
   std::lock_guard<std::recursive_mutex> guard(GetMutex());
 
   Vote result = eVoteNoOpinion;
-  m_process->UpdateThreadListIfNeeded();
+  m_process.UpdateThreadListIfNeeded();
   collection::iterator pos, end = m_threads.end();
 
   Log *log = GetLog(LLDBLog::Step);
@@ -425,7 +424,7 @@ Vote ThreadList::ShouldReportStop(Event *event_ptr) {
 void ThreadList::SetShouldReportStop(Vote vote) {
   std::lock_guard<std::recursive_mutex> guard(GetMutex());
 
-  m_process->UpdateThreadListIfNeeded();
+  m_process.UpdateThreadListIfNeeded();
   collection::iterator pos, end = m_threads.end();
   for (pos = m_threads.begin(); pos != end; ++pos) {
     ThreadSP thread_sp(*pos);
@@ -438,7 +437,7 @@ Vote ThreadList::ShouldReportRun(Event *event_ptr) {
   std::lock_guard<std::recursive_mutex> guard(GetMutex());
 
   Vote result = eVoteNoOpinion;
-  m_process->UpdateThreadListIfNeeded();
+  m_process.UpdateThreadListIfNeeded();
   collection::iterator pos, end = m_threads.end();
 
   // Run through the threads and ask whether we should report this event. The
@@ -486,7 +485,7 @@ void ThreadList::Destroy() {
 void ThreadList::RefreshStateAfterStop() {
   std::lock_guard<std::recursive_mutex> guard(GetMutex());
 
-  m_process->UpdateThreadListIfNeeded();
+  m_process.UpdateThreadListIfNeeded();
 
   Log *log = GetLog(LLDBLog::Step);
   if (log && log->GetVerbose())
@@ -515,7 +514,7 @@ bool ThreadList::WillResume() {
   // they are.
 
   std::lock_guard<std::recursive_mutex> guard(GetMutex());
-  m_process->UpdateThreadListIfNeeded();
+  m_process.UpdateThreadListIfNeeded();
 
   collection::iterator pos, end = m_threads.end();
 
@@ -546,13 +545,13 @@ bool ThreadList::WillResume() {
     if (log && log->GetVerbose())
       LLDB_LOGF(log, "Turning on notification of new threads while single "
                      "stepping a thread.");
-    m_process->StartNoticingNewThreads();
+    m_process.StartNoticingNewThreads();
   } else {
     Log *log = GetLog(LLDBLog::Step);
     if (log && log->GetVerbose())
       LLDB_LOGF(log, "Turning off notification of new threads while single "
                      "stepping a thread.");
-    m_process->StopNoticingNewThreads();
+    m_process.StopNoticingNewThreads();
   }
 
   // Give all the threads that are likely to run a last chance to set up their
@@ -575,7 +574,7 @@ bool ThreadList::WillResume() {
 
   ThreadList run_me_only_list(m_process);
 
-  run_me_only_list.SetStopID(m_process->GetStopID());
+  run_me_only_list.SetStopID(m_process.GetStopID());
 
   // One or more threads might want to "Stop Others".  We want to handle all
   // those requests first.  And if there is a thread that wanted to "resume
@@ -736,11 +735,13 @@ void ThreadList::NotifySelectedThreadChanged(lldb::tid_t tid) {
 
 void ThreadList::Update(ThreadList &rhs) {
   if (this != &rhs) {
-    // Lock both mutexes to make sure neither side changes anyone on us while
-    // the assignment occurs
-    std::scoped_lock<std::recursive_mutex, std::recursive_mutex> guard(GetMutex(), rhs.GetMutex());
+    // We only allow assignments between thread lists describing the same
+    // process. Same process implies same mutex, which means it's enough to lock
+    // just the current object.
+    assert(&m_process == &rhs.m_process);
+    assert(&GetMutex() == &rhs.GetMutex());
+    std::lock_guard<std::recursive_mutex> guard(GetMutex());
 
-    m_process = rhs.m_process;
     m_stop_id = rhs.m_stop_id;
     m_threads.swap(rhs.m_threads);
     m_selected_tid = rhs.m_selected_tid;
@@ -784,7 +785,7 @@ void ThreadList::Flush() {
 }
 
 std::recursive_mutex &ThreadList::GetMutex() const {
-  return m_process->m_thread_mutex;
+  return m_process.m_thread_mutex;
 }
 
 ThreadList::ExpressionExecutionThreadPusher::ExpressionExecutionThreadPusher(
