Skip to content

Commit e823449

Browse files
authored
[lldb][debugserver] Synchronize interrupt and resume signals (#131073)
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 the 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.
1 parent bbb244c commit e823449

File tree

2 files changed

+14
-10
lines changed

2 files changed

+14
-10
lines changed

lldb/tools/debugserver/source/MacOSX/MachProcess.h

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -427,16 +427,16 @@ class MachProcess {
427427
m_profile_data_mutex; // Multithreaded protection for profile info data
428428
std::vector<std::string>
429429
m_profile_data; // Profile data, must be protected by m_profile_data_mutex
430-
PThreadEvent m_profile_events; // Used for the profile thread cancellable wait
430+
PThreadEvent m_profile_events; // Used for the profile thread cancellable wait
431431
DNBThreadResumeActions m_thread_actions; // The thread actions for the current
432432
// MachProcess::Resume() call
433433
MachException::Message::collection m_exception_messages; // A collection of
434434
// exception messages
435435
// caught when
436436
// listening to the
437437
// exception port
438-
PThreadMutex m_exception_messages_mutex; // Multithreaded protection for
439-
// m_exception_messages
438+
PThreadMutex m_exception_and_signal_mutex; // Multithreaded protection for
439+
// exceptions and signals.
440440

441441
MachThreadList m_thread_list; // A list of threads that is maintained/updated
442442
// after each stop

lldb/tools/debugserver/source/MacOSX/MachProcess.mm

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -528,7 +528,7 @@ static bool FBSAddEventDataToOptions(NSMutableDictionary *options,
528528
m_profile_data_mutex(PTHREAD_MUTEX_RECURSIVE), m_profile_data(),
529529
m_profile_events(0, eMachProcessProfileCancel), m_thread_actions(),
530530
m_exception_messages(),
531-
m_exception_messages_mutex(PTHREAD_MUTEX_RECURSIVE), m_thread_list(),
531+
m_exception_and_signal_mutex(PTHREAD_MUTEX_RECURSIVE), m_thread_list(),
532532
m_activities(), m_state(eStateUnloaded),
533533
m_state_mutex(PTHREAD_MUTEX_RECURSIVE), m_events(0, kAllEventsMask),
534534
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) {
13381338
m_stop_count = 0;
13391339
m_thread_list.Clear();
13401340
{
1341-
PTHREAD_MUTEX_LOCKER(locker, m_exception_messages_mutex);
1341+
PTHREAD_MUTEX_LOCKER(locker, m_exception_and_signal_mutex);
13421342
m_exception_messages.clear();
1343+
m_sent_interrupt_signo = 0;
1344+
m_auto_resume_signo = 0;
1345+
13431346
}
13441347
m_activities.Clear();
13451348
StopProfileThread();
@@ -1575,6 +1578,7 @@ static uint64_t bits(uint64_t value, uint32_t msbit, uint32_t lsbit) {
15751578
bool MachProcess::Interrupt() {
15761579
nub_state_t state = GetState();
15771580
if (IsRunning(state)) {
1581+
PTHREAD_MUTEX_LOCKER(locker, m_exception_and_signal_mutex);
15781582
if (m_sent_interrupt_signo == 0) {
15791583
m_sent_interrupt_signo = SIGSTOP;
15801584
if (Signal(m_sent_interrupt_signo)) {
@@ -1728,7 +1732,7 @@ static uint64_t bits(uint64_t value, uint32_t msbit, uint32_t lsbit) {
17281732
m_thread_actions.Append(thread_action);
17291733
m_thread_actions.SetDefaultThreadActionIfNeeded(eStateRunning, 0);
17301734

1731-
PTHREAD_MUTEX_LOCKER(locker, m_exception_messages_mutex);
1735+
PTHREAD_MUTEX_LOCKER(locker, m_exception_and_signal_mutex);
17321736

17331737
ReplyToAllExceptions();
17341738
}
@@ -1854,7 +1858,7 @@ static uint64_t bits(uint64_t value, uint32_t msbit, uint32_t lsbit) {
18541858
}
18551859

18561860
void MachProcess::ReplyToAllExceptions() {
1857-
PTHREAD_MUTEX_LOCKER(locker, m_exception_messages_mutex);
1861+
PTHREAD_MUTEX_LOCKER(locker, m_exception_and_signal_mutex);
18581862
if (!m_exception_messages.empty()) {
18591863
MachException::Message::iterator pos;
18601864
MachException::Message::iterator begin = m_exception_messages.begin();
@@ -1888,7 +1892,7 @@ static uint64_t bits(uint64_t value, uint32_t msbit, uint32_t lsbit) {
18881892
}
18891893
}
18901894
void MachProcess::PrivateResume() {
1891-
PTHREAD_MUTEX_LOCKER(locker, m_exception_messages_mutex);
1895+
PTHREAD_MUTEX_LOCKER(locker, m_exception_and_signal_mutex);
18921896

18931897
m_auto_resume_signo = m_sent_interrupt_signo;
18941898
if (m_auto_resume_signo)
@@ -2290,7 +2294,7 @@ static uint64_t bits(uint64_t value, uint32_t msbit, uint32_t lsbit) {
22902294
// data has already been copied.
22912295
void MachProcess::ExceptionMessageReceived(
22922296
const MachException::Message &exceptionMessage) {
2293-
PTHREAD_MUTEX_LOCKER(locker, m_exception_messages_mutex);
2297+
PTHREAD_MUTEX_LOCKER(locker, m_exception_and_signal_mutex);
22942298

22952299
if (m_exception_messages.empty())
22962300
m_task.Suspend();
@@ -2304,7 +2308,7 @@ static uint64_t bits(uint64_t value, uint32_t msbit, uint32_t lsbit) {
23042308

23052309
task_t MachProcess::ExceptionMessageBundleComplete() {
23062310
// We have a complete bundle of exceptions for our child process.
2307-
PTHREAD_MUTEX_LOCKER(locker, m_exception_messages_mutex);
2311+
PTHREAD_MUTEX_LOCKER(locker, m_exception_and_signal_mutex);
23082312
DNBLogThreadedIf(LOG_EXCEPTIONS, "%s: %llu exception messages.",
23092313
__PRETTY_FUNCTION__, (uint64_t)m_exception_messages.size());
23102314
bool auto_resume = false;

0 commit comments

Comments
 (0)