llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: Charles Zablit (charles-zablit) <details> <summary>Changes</summary> `DebuggerThread::m_active_exception` and `m_exception_pred` are accessed both from the Windows debug-event loop and from the thread driving the debugger. There is no sync mechanism between the two. That caused two distinct failures. 1. Use after free: The `m_active_exception.reset()` in `ContinueAsyncException()` can destroy the exception while the delegate uses it. 2. `ContinueAsyncException()` can be called between the end of the delegate and `SetValue(result)`. Causing `WaitForValueNotEqualTo(BreakInDebugger) to spin forever`. This patch guards `m_active_exception` with a mutex and returns a `ExceptionRecordSP` from `GetActiveException()` so callers no longer have to lock a weak_ptr. To verify this, I ran the test suite and injected a 50ms delay into the window between the delegate returning and the `SetValue()`. This reproduces the packet timeout in `TestGdbRemoteExitCode` deterministically before this change, and all lldb-server tests pass with the same delay after it. --- Full diff: https://github.com/llvm/llvm-project/pull/213075.diff 4 Files Affected: - (modified) lldb/source/Plugins/Process/Windows/Common/DebuggerThread.cpp (+25-8) - (modified) lldb/source/Plugins/Process/Windows/Common/DebuggerThread.h (+7-4) - (modified) lldb/source/Plugins/Process/Windows/Common/NativeProcessWindows.cpp (+1-1) - (modified) lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp (+3-4) ``````````diff diff --git a/lldb/source/Plugins/Process/Windows/Common/DebuggerThread.cpp b/lldb/source/Plugins/Process/Windows/Common/DebuggerThread.cpp index 228efe18e81d1..ade798baadb65 100644 --- a/lldb/source/Plugins/Process/Windows/Common/DebuggerThread.cpp +++ b/lldb/source/Plugins/Process/Windows/Common/DebuggerThread.cpp @@ -203,7 +203,7 @@ Status DebuggerThread::StopDebugging(bool terminate) { // breakpoint messing around in the debugger), continue it now. But only // AFTER calling TerminateProcess to make sure that the very next call to // WaitForDebugEventEx is an exit process event. - if (m_active_exception.get()) { + if (GetActiveException()) { LLDB_LOG(log, "masking active exception"); ContinueAsyncException(ExceptionResult::MaskException); } @@ -239,15 +239,23 @@ Status DebuggerThread::StopDebugging(bool terminate) { return error; } +ExceptionRecordSP DebuggerThread::GetActiveException() { + std::lock_guard<std::mutex> guard(m_active_exception_mutex); + return m_active_exception; +} + void DebuggerThread::ContinueAsyncException(ExceptionResult result) { - if (!m_active_exception.get()) - return; + { + std::lock_guard<std::mutex> guard(m_active_exception_mutex); + if (!m_active_exception) + return; + m_active_exception.reset(); + } Log *log = GetLog(WindowsLog::Process | WindowsLog::Exception); LLDB_LOG(log, "broadcasting for inferior process {0}.", m_process.GetProcessId()); - m_active_exception.reset(); m_exception_pred.SetValue(result, eBroadcastAlways); } @@ -401,15 +409,24 @@ DebuggerThread::HandleExceptionEvent(const EXCEPTION_DEBUG_INFO &info, bool first_chance = (info.dwFirstChance != 0); - m_active_exception.reset( - new ExceptionRecord(info.ExceptionRecord, thread_id)); + ExceptionRecordSP active_exception = + std::make_shared<ExceptionRecord>(info.ExceptionRecord, thread_id); + { + std::lock_guard<std::mutex> guard(m_active_exception_mutex); + m_active_exception = active_exception; + } + m_exception_pred.SetValue(ExceptionResult::BreakInDebugger, eBroadcastNever); + LLDB_LOG(log, "encountered {0} chance exception {1:x} on thread {2:x}", first_chance ? "first" : "second", info.ExceptionRecord.ExceptionCode, thread_id); ExceptionResult result = - m_debug_delegate->OnDebugException(first_chance, *m_active_exception); - m_exception_pred.SetValue(result, eBroadcastNever); + m_debug_delegate->OnDebugException(first_chance, *active_exception); + // If the delegate dealt with the exception itself, continue it now. This is + // a no-op if the other thread got there first, in which case its result wins. + if (result != ExceptionResult::BreakInDebugger) + ContinueAsyncException(result); LLDB_LOG(log, "waiting for ExceptionPred != BreakInDebugger"); result = *m_exception_pred.WaitForValueNotEqualTo( diff --git a/lldb/source/Plugins/Process/Windows/Common/DebuggerThread.h b/lldb/source/Plugins/Process/Windows/Common/DebuggerThread.h index 70204e2f9e5eb..03202264b01ee 100644 --- a/lldb/source/Plugins/Process/Windows/Common/DebuggerThread.h +++ b/lldb/source/Plugins/Process/Windows/Common/DebuggerThread.h @@ -11,6 +11,7 @@ #include <atomic> #include <memory> +#include <mutex> #include "ForwardDecl.h" #include "lldb/Host/HostProcess.h" @@ -34,9 +35,10 @@ class DebuggerThread : public std::enable_shared_from_this<DebuggerThread> { HostProcess GetProcess() const { return m_process; } HostThread GetMainThread() const { return m_main_thread; } - std::weak_ptr<ExceptionRecord> GetActiveException() { - return m_active_exception; - } + + /// Returns the exception the debug loop is currently reporting, or null if + /// there is none. Safe to call from any thread. + ExceptionRecordSP GetActiveException(); Status StopDebugging(bool terminate); @@ -74,8 +76,9 @@ class DebuggerThread : public std::enable_shared_from_this<DebuggerThread> { // The image file of the process being debugged. HANDLE m_image_file = nullptr; - // The current exception waiting to be handled + // The current exception waiting to be handled. ExceptionRecordSP m_active_exception; + std::mutex m_active_exception_mutex; // A predicate which gets signalled when an exception is finished processing // and the debug loop can be continued. diff --git a/lldb/source/Plugins/Process/Windows/Common/NativeProcessWindows.cpp b/lldb/source/Plugins/Process/Windows/Common/NativeProcessWindows.cpp index f87fd23f5a047..17dd05af35e1b 100644 --- a/lldb/source/Plugins/Process/Windows/Common/NativeProcessWindows.cpp +++ b/lldb/source/Plugins/Process/Windows/Common/NativeProcessWindows.cpp @@ -152,7 +152,7 @@ Status NativeProcessWindows::Resume(const ResumeActionList &resume_actions) { // Resume the debug loop. ExceptionRecordSP active_exception = - m_session_data->m_debugger->GetActiveException().lock(); + m_session_data->m_debugger->GetActiveException(); if (active_exception) { // Resume the process and continue processing debug events. Mask the // exception so that from the process's view, there is no indication that diff --git a/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp b/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp index 561710ccec3c8..289223fb5bb67 100644 --- a/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp +++ b/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp @@ -278,7 +278,7 @@ Status ProcessWindows::DoResume(RunDirection direction) { } ExceptionRecordSP active_exception = - m_session_data->m_debugger->GetActiveException().lock(); + m_session_data->m_debugger->GetActiveException(); if (active_exception) { // Resume the process and continue processing debug events. Mask the // exception so that from the process's view, there is no indication that @@ -337,9 +337,8 @@ void ProcessWindows::RefreshStateAfterStop() { m_thread_list.RefreshStateAfterStop(); - std::weak_ptr<ExceptionRecord> exception_record = + ExceptionRecordSP active_exception = m_session_data->m_debugger->GetActiveException(); - ExceptionRecordSP active_exception = exception_record.lock(); if (!active_exception) { LLDB_LOG(log, "there is no active exception in process {0}. Why is the " @@ -901,7 +900,7 @@ std::optional<uint32_t> ProcessWindows::GetWatchpointSlotCount() { std::optional<DWORD> ProcessWindows::GetActiveExceptionCode() const { if (!m_session_data || !m_session_data->m_debugger) return std::nullopt; - auto exc = m_session_data->m_debugger->GetActiveException().lock(); + auto exc = m_session_data->m_debugger->GetActiveException(); if (!exc) return std::nullopt; return exc->GetExceptionValue(); `````````` </details> https://github.com/llvm/llvm-project/pull/213075 _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
