[lldb] Fixed deadlock when SBProcess is Kill()ed and inspected

[cameron314]

Summary

This patch changes the order that mutexes are acquired in SBProcess such that the target API mutex is now always acquired before the public run lock mutex is acquired.

This fixes a deadlock in LLDB when calling SBProcess::Kill() while calling e.g. SBProcess::GetThreadByID(): Kill takes the target API mutex, then waits for the private state thread to exit, which tries to lock the public run lock mutex, but it's already being held by GetThreadByID, which is waiting in turn for the target API mutex.

[clayborg]

Since this is duplicated in so many spots, can we make a helper function to do this to ensure no one gets it wrong. We might be able to pass in the "guard" and "stop_locker" as reference variables and modify them in the helper function.

[cameron314]

I suppose we could but there's a few places outside of SBProcess that also use the run lock and API mutex; personally, I prefer it to be explicit which mutex is being taken first.

[cameron314]

Ping?

[cameron314]

Anyone?
We still have this patch applied on our recently-rebased fork with no problems...

[clayborg]

Or better yet, create a structure that everyone must use and have the locking exist in the class itself:

struct ProcessLocker {
  std::lock_guard<std::recursive_mutex> guard;
  Process::StopLocker stop_locker;

  void ProcessLocker(Process &process) {
     ...
  }
};

[cameron314]

@clayborg, I'm not sure how that would work. There's many places that lock the process run lock without locking the target API mutex, and vice versa.

[clayborg]

In D53412#1446252, @cameron314 wrote:

@clayborg, I'm not sure how that would work. There's many places that lock the process run lock without locking the target API mutex, and vice versa.

Add an argument to the ProcessLocker constructor maybe?

[cameron314]

There's dozens of places that take the API mutex without taking the process mutex. Take Kill for example: It needs to take the API mutex, but cannot take the run lock since it will be taken by the private state thread. Another example is HandleCommand, which takes the API mutex but has no process that it could ask to lock the API mutex for it.

On the flip side, all the SBFrame methods lock the process run lock but not the API mutex. And so on.

I just really don't think this can be refactored in a useful way without rewriting the way all SB locks are taken, which is almost impossible to do at this point.

[clayborg]

All of these are in SBProcess. No need to change everywhere else, I just see a ton of duplicated code here in the SBProcess.cpp. If we contain those in a small struct/class, then we can easily make changes as needed without 50 diffs in this file each time. So no need to fix all the call sites in this patch, just the ones in SBProcess.cpp

[clayborg]

Added Jim Ingham in case he wants to chime in here.

[clayborg]

In general any call that is doing anything through the public API needs to take the target API mutex. It is ok for quick accessors or other things to not do so, but anything that would need to keep the process in its current state, like asking a frame for a register value, should take the API mutex to ensure one thread doesn't say "run" and another says "what is the PC from frame 0 of thread 1".

[cameron314]

In D53412#1450182, @clayborg wrote:

All of these are in SBProcess. No need to change everywhere else, I just see a ton of duplicated code here in the SBProcess.cpp. If we contain those in a small struct/class, then we can easily make changes as needed without 50 diffs in this file each time. So no need to fix all the call sites in this patch, just the ones in SBProcess.cpp

Ah, that makes more sense :-) Sorry I misinterpreted your suggestion.

I can add a RAII lock-helper struct in SBProcess, but the order that locks are taken within it must not be modified without changing *all* the functions that take both locks (both directly and indirectly), which includes a significant portion of the SB API (and not just SBProcess).

source/API/SBProcess.cpp

 }
 
 uint32_t SBProcess::GetNumThreads() {
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_API));
 
   uint32_t num_threads = 0;
   ProcessSP process_sp(GetSP());
   if (process_sp) {
-    Process::StopLocker stop_locker;
-
-    const bool can_update = stop_locker.TryLock(&process_sp->GetRunLock());
     std::lock_guard<std::recursive_mutex> guard(
         process_sp->GetTarget().GetAPIMutex());
+    Process::StopLocker stop_locker;
+    const bool can_update = stop_locker.TryLock(&process_sp->GetRunLock());
     num_threads = process_sp->GetThreadList().GetSize(can_update);
   }
 
   if (log)
     log->Printf("SBProcess(%p)::GetNumThreads () => %d",
                 static_cast<void *>(process_sp.get()), num_threads);
 
   return num_threads;

 
 SBThread SBProcess::GetThreadAtIndex(size_t index) {
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_API));
 
   SBThread sb_thread;
   ThreadSP thread_sp;
   ProcessSP process_sp(GetSP());
   if (process_sp) {
-    Process::StopLocker stop_locker;
-    const bool can_update = stop_locker.TryLock(&process_sp->GetRunLock());
     std::lock_guard<std::recursive_mutex> guard(
         process_sp->GetTarget().GetAPIMutex());
+    Process::StopLocker stop_locker;
+    const bool can_update = stop_locker.TryLock(&process_sp->GetRunLock());
     thread_sp = process_sp->GetThreadList().GetThreadAtIndex(index, can_update);
     sb_thread.SetThread(thread_sp);
   }
 
   if (log)
     log->Printf("SBProcess(%p)::GetThreadAtIndex (index=%d) => SBThread(%p)",
                 static_cast<void *>(process_sp.get()),
                 static_cast<uint32_t>(index),
                 static_cast<void *>(thread_sp.get()));
 
   return sb_thread;
 }
 
 uint32_t SBProcess::GetNumQueues() {
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_API));
 
   uint32_t num_queues = 0;
   ProcessSP process_sp(GetSP());
   if (process_sp) {
-    Process::StopLocker stop_locker;
-    if (stop_locker.TryLock(&process_sp->GetRunLock())) {
-      std::lock_guard<std::recursive_mutex> guard(
-          process_sp->GetTarget().GetAPIMutex());
+    Process::StopLocker stop_locker;
+    if (stop_locker.TryLock(&process_sp->GetRunLock())) {
       num_queues = process_sp->GetQueueList().GetSize();
     }
   }
 
   if (log)
     log->Printf("SBProcess(%p)::GetNumQueues () => %d",
                 static_cast<void *>(process_sp.get()), num_queues);
 
   return num_queues;
 }
 
 SBQueue SBProcess::GetQueueAtIndex(size_t index) {
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_API));
 
   SBQueue sb_queue;
   QueueSP queue_sp;
   ProcessSP process_sp(GetSP());
   if (process_sp) {
-    Process::StopLocker stop_locker;
-    if (stop_locker.TryLock(&process_sp->GetRunLock())) {
-      std::lock_guard<std::recursive_mutex> guard(
-          process_sp->GetTarget().GetAPIMutex());
+    Process::StopLocker stop_locker;
+    if (stop_locker.TryLock(&process_sp->GetRunLock())) {
       queue_sp = process_sp->GetQueueList().GetQueueAtIndex(index);
       sb_queue.SetQueue(queue_sp);
     }
   }
 
   if (log)
     log->Printf("SBProcess(%p)::GetQueueAtIndex (index=%d) => SBQueue(%p)",
                 static_cast<void *>(process_sp.get()),
void SBProcess::SendAsyncInterrupt() {
   }
 }
 
 SBThread SBProcess::GetThreadByID(tid_t tid) {
   SBThread sb_thread;
   ThreadSP thread_sp;
   ProcessSP process_sp(GetSP());
   if (process_sp) {
-    Process::StopLocker stop_locker;
-    const bool can_update = stop_locker.TryLock(&process_sp->GetRunLock());
     std::lock_guard<std::recursive_mutex> guard(
         process_sp->GetTarget().GetAPIMutex());
+    Process::StopLocker stop_locker;
+    const bool can_update = stop_locker.TryLock(&process_sp->GetRunLock());
     thread_sp = process_sp->GetThreadList().FindThreadByID(tid, can_update);
     sb_thread.SetThread(thread_sp);
   }
 
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_API));
   if (log)
     log->Printf("SBProcess(%p)::GetThreadByID (tid=0x%4.4" PRIx64
                 ") => SBThread (%p)",
                 static_cast<void *>(process_sp.get()), tid,
                 static_cast<void *>(thread_sp.get()));
 
   return sb_thread;
 }
 
 SBThread SBProcess::GetThreadByIndexID(uint32_t index_id) {
   SBThread sb_thread;
   ThreadSP thread_sp;
   ProcessSP process_sp(GetSP());
   if (process_sp) {
-    Process::StopLocker stop_locker;
-    const bool can_update = stop_locker.TryLock(&process_sp->GetRunLock());
     std::lock_guard<std::recursive_mutex> guard(
         process_sp->GetTarget().GetAPIMutex());
+    Process::StopLocker stop_locker;
+    const bool can_update = stop_locker.TryLock(&process_sp->GetRunLock());
     thread_sp =
         process_sp->GetThreadList().FindThreadByIndexID(index_id, can_update);
     sb_thread.SetThread(thread_sp);
   }
 
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_API));
   if (log)
     log->Printf("SBProcess(%p)::GetThreadByID (tid=0x%x) => SBThread (%p)",
size_t SBProcess::ReadMemory(addr_t addr, void *dst, size_t dst_len,
   if (log)
     log->Printf("SBProcess(%p)::ReadMemory (addr=0x%" PRIx64
                 ", dst=%p, dst_len=%" PRIu64 ", SBError (%p))...",
                 static_cast<void *>(process_sp.get()), addr,
                 static_cast<void *>(dst), static_cast<uint64_t>(dst_len),
                 static_cast<void *>(sb_error.get()));
 
   if (process_sp) {
-    Process::StopLocker stop_locker;
-    if (stop_locker.TryLock(&process_sp->GetRunLock())) {
-      std::lock_guard<std::recursive_mutex> guard(
-          process_sp->GetTarget().GetAPIMutex());
+    Process::StopLocker stop_locker;
+    if (stop_locker.TryLock(&process_sp->GetRunLock())) {
       bytes_read = process_sp->ReadMemory(addr, dst, dst_len, sb_error.ref());
     } else {
       if (log)
         log->Printf("SBProcess(%p)::ReadMemory() => error: process is running",
                     static_cast<void *>(process_sp.get()));
       sb_error.SetErrorString("process is running");
     }
   } else {
size_t SBProcess::ReadMemory(addr_t addr, void *dst, size_t dst_len,
   return bytes_read;
 }
 
 size_t SBProcess::ReadCStringFromMemory(addr_t addr, void *buf, size_t size,
                                         lldb::SBError &sb_error) {
   size_t bytes_read = 0;
   ProcessSP process_sp(GetSP());
   if (process_sp) {
-    Process::StopLocker stop_locker;
-    if (stop_locker.TryLock(&process_sp->GetRunLock())) {
-      std::lock_guard<std::recursive_mutex> guard(
-          process_sp->GetTarget().GetAPIMutex());
+    Process::StopLocker stop_locker;
+    if (stop_locker.TryLock(&process_sp->GetRunLock())) {
       bytes_read = process_sp->ReadCStringFromMemory(addr, (char *)buf, size,
                                                      sb_error.ref());
     } else {
       Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_API));
       if (log)
         log->Printf("SBProcess(%p)::ReadCStringFromMemory() => error: process "
                     "is running",
                     static_cast<void *>(process_sp.get()));
       sb_error.SetErrorString("process is running");
     }
   } else {
     sb_error.SetErrorString("SBProcess is invalid");
   }
   return bytes_read;
 }
 
 uint64_t SBProcess::ReadUnsignedFromMemory(addr_t addr, uint32_t byte_size,
                                            lldb::SBError &sb_error) {
   uint64_t value = 0;
   ProcessSP process_sp(GetSP());
   if (process_sp) {
-    Process::StopLocker stop_locker;
-    if (stop_locker.TryLock(&process_sp->GetRunLock())) {
-      std::lock_guard<std::recursive_mutex> guard(
-          process_sp->GetTarget().GetAPIMutex());
+    Process::StopLocker stop_locker;
+    if (stop_locker.TryLock(&process_sp->GetRunLock())) {
       value = process_sp->ReadUnsignedIntegerFromMemory(addr, byte_size, 0,
                                                         sb_error.ref());
     } else {
       Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_API));
       if (log)
         log->Printf("SBProcess(%p)::ReadUnsignedFromMemory() => error: process "
                     "is running",
                     static_cast<void *>(process_sp.get()));
       sb_error.SetErrorString("process is running");
     }
   } else {
     sb_error.SetErrorString("SBProcess is invalid");
   }
   return value;
 }
 
 lldb::addr_t SBProcess::ReadPointerFromMemory(addr_t addr,
                                               lldb::SBError &sb_error) {
   lldb::addr_t ptr = LLDB_INVALID_ADDRESS;
   ProcessSP process_sp(GetSP());
   if (process_sp) {
-    Process::StopLocker stop_locker;
-    if (stop_locker.TryLock(&process_sp->GetRunLock())) {
-      std::lock_guard<std::recursive_mutex> guard(
-          process_sp->GetTarget().GetAPIMutex());
+    Process::StopLocker stop_locker;
+    if (stop_locker.TryLock(&process_sp->GetRunLock())) {
       ptr = process_sp->ReadPointerFromMemory(addr, sb_error.ref());
     } else {
       Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_API));
       if (log)
         log->Printf("SBProcess(%p)::ReadPointerFromMemory() => error: process "
                     "is running",
                     static_cast<void *>(process_sp.get()));
       sb_error.SetErrorString("process is running");
size_t SBProcess::WriteMemory(addr_t addr, const void *src, size_t src_len,
   if (log)
     log->Printf("SBProcess(%p)::WriteMemory (addr=0x%" PRIx64
                 ", src=%p, src_len=%" PRIu64 ", SBError (%p))...",
                 static_cast<void *>(process_sp.get()), addr,
                 static_cast<const void *>(src), static_cast<uint64_t>(src_len),
                 static_cast<void *>(sb_error.get()));
 
   if (process_sp) {
-    Process::StopLocker stop_locker;
-    if (stop_locker.TryLock(&process_sp->GetRunLock())) {
-      std::lock_guard<std::recursive_mutex> guard(
-          process_sp->GetTarget().GetAPIMutex());
+    Process::StopLocker stop_locker;
+    if (stop_locker.TryLock(&process_sp->GetRunLock())) {
       bytes_written =
           process_sp->WriteMemory(addr, src, src_len, sb_error.ref());
     } else {
       if (log)
         log->Printf("SBProcess(%p)::WriteMemory() => error: process is running",
                     static_cast<void *>(process_sp.get()));
       sb_error.SetErrorString("process is running");
     }

 }
 
 uint32_t SBProcess::LoadImage(const lldb::SBFileSpec &sb_local_image_spec,
                               const lldb::SBFileSpec &sb_remote_image_spec,
                               lldb::SBError &sb_error) {
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_API));
   ProcessSP process_sp(GetSP());
   if (process_sp) {
+    std::lock_guard<std::recursive_mutex> guard(
+        process_sp->GetTarget().GetAPIMutex());
     Process::StopLocker stop_locker;
     if (stop_locker.TryLock(&process_sp->GetRunLock())) {
       if (log)
         log->Printf("SBProcess(%p)::LoadImage() => calling Platform::LoadImage"
                     "for: %s",
                     static_cast<void *>(process_sp.get()),
                     sb_local_image_spec.GetFilename());
-
-      std::lock_guard<std::recursive_mutex> guard(
-        process_sp->GetTarget().GetAPIMutex());
       PlatformSP platform_sp = process_sp->GetTarget().GetPlatform();
       return platform_sp->LoadImage(process_sp.get(), *sb_local_image_spec,
                                     *sb_remote_image_spec, sb_error.ref());
     } else {
       if (log)
         log->Printf("SBProcess(%p)::LoadImage() => error: process is running",
                     static_cast<void *>(process_sp.get()));
       sb_error.SetErrorString("process is running");

 
 uint32_t SBProcess::LoadImageUsingPaths(const lldb::SBFileSpec &image_spec,
                                         SBStringList &paths,
                                         lldb::SBFileSpec &loaded_path, 
                                         lldb::SBError &error) {
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_API));
   ProcessSP process_sp(GetSP());
   if (process_sp) {
+    std::lock_guard<std::recursive_mutex> guard(
+        process_sp->GetTarget().GetAPIMutex());
     Process::StopLocker stop_locker;
     if (stop_locker.TryLock(&process_sp->GetRunLock())) {
       if (log)
         log->Printf("SBProcess(%p)::LoadImageUsingPaths() => "
                     "calling Platform::LoadImageUsingPaths for: %s",
                     static_cast<void *>(process_sp.get()),
                     image_spec.GetFilename());
-
-      std::lock_guard<std::recursive_mutex> guard(
-        process_sp->GetTarget().GetAPIMutex());
       PlatformSP platform_sp = process_sp->GetTarget().GetPlatform();
       size_t num_paths = paths.GetSize();
       std::vector<std::string> paths_vec;
       paths_vec.reserve(num_paths);
       for (size_t i = 0; i < num_paths; i++)
         paths_vec.push_back(paths.GetStringAtIndex(i));
       FileSpec loaded_spec;
       
uint32_t SBProcess::LoadImageUsingPaths(const lldb::SBFileSpec &image_spec,
   
   return LLDB_INVALID_IMAGE_TOKEN;
 }
 
 lldb::SBError SBProcess::UnloadImage(uint32_t image_token) {
   lldb::SBError sb_error;
   ProcessSP process_sp(GetSP());
   if (process_sp) {
-    Process::StopLocker stop_locker;
-    if (stop_locker.TryLock(&process_sp->GetRunLock())) {
-      std::lock_guard<std::recursive_mutex> guard(
-          process_sp->GetTarget().GetAPIMutex());
+    Process::StopLocker stop_locker;
+    if (stop_locker.TryLock(&process_sp->GetRunLock())) {
       PlatformSP platform_sp = process_sp->GetTarget().GetPlatform();
       sb_error.SetError(
           platform_sp->UnloadImage(process_sp.get(), image_token));
     } else {
       Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_API));
       if (log)
         log->Printf("SBProcess(%p)::UnloadImage() => error: process is running",
                     static_cast<void *>(process_sp.get()));
       sb_error.SetErrorString("process is running");
     }
   } else
     sb_error.SetErrorString("invalid process");
   return sb_error;
 }
 
 lldb::SBError SBProcess::SendEventData(const char *event_data) {
   lldb::SBError sb_error;
   ProcessSP process_sp(GetSP());
   if (process_sp) {
-    Process::StopLocker stop_locker;
-    if (stop_locker.TryLock(&process_sp->GetRunLock())) {
-      std::lock_guard<std::recursive_mutex> guard(
-          process_sp->GetTarget().GetAPIMutex());
+    Process::StopLocker stop_locker;
+    if (stop_locker.TryLock(&process_sp->GetRunLock())) {
       sb_error.SetError(process_sp->SendEventData(event_data));
     } else {
       Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_API));
       if (log)
         log->Printf(
             "SBProcess(%p)::SendEventData() => error: process is running",
             static_cast<void *>(process_sp.get()));
       sb_error.SetErrorString("process is running");

 lldb::SBError
 SBProcess::GetMemoryRegionInfo(lldb::addr_t load_addr,
                                SBMemoryRegionInfo &sb_region_info) {
   lldb::SBError sb_error;
   ProcessSP process_sp(GetSP());
   MemoryRegionInfoSP region_info_sp =
       std::make_shared<lldb_private::MemoryRegionInfo>();
   if (process_sp) {
-    Process::StopLocker stop_locker;
-    if (stop_locker.TryLock(&process_sp->GetRunLock())) {
-      std::lock_guard<std::recursive_mutex> guard(
-          process_sp->GetTarget().GetAPIMutex());
+    Process::StopLocker stop_locker;
+    if (stop_locker.TryLock(&process_sp->GetRunLock())) {
       sb_error.ref() =
           process_sp->GetMemoryRegionInfo(load_addr, *region_info_sp);
       if (sb_error.Success()) {
         sb_region_info.ref() = *region_info_sp;
       }
     } else {
       Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_API));
       if (log)