Page MenuHomePhabricator

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

Authored by cameron314 on Oct 18 2018, 2:34 PM.

Details

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.

Diff Detail

Event Timeline

cameron314 created this revision.Oct 18 2018, 2:34 PM

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.

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.

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

Herald added a project: Restricted Project. · View Herald TranscriptMar 28 2019, 6:56 AM

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) {
     ...
  }
};

@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, 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?

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.

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

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

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".

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).