Skip to content

Commit f12e038

Browse files
[lldb] Guard SBFrame/SBThread methods against running processes (llvm#152020)
Prior to this patch, SBFrame/SBThread methods exhibit racy behavior if called while the process is running, because they do not lock the `Process::RetRunLock` mutex. If they did, they would fail, correctly identifying that the process is not running. Some methods _attempt_ to protect against this with the pattern: ``` ExecutionContext exe_ctx(m_opaque_sp.get(), lock); // this is a different lock Process *process = exe_ctx.GetProcessPtr(); if (process) { Process::StopLocker stop_locker; if (stop_locker.TryLock(&process->GetRunLock())) .... do work ... ``` However, this is also racy: the constructor of `ExecutionContext` will access the frame list, which is something that can only be done once the process is stopped. With this patch: 1. The constructor of `ExecutionContext` now expects a `ProcessRunLock` as an argument. It attempts to lock the run lock, and only fills in information about frames and threads if the lock can be acquired. Callers of the constructor are expected to check the lock. 2. All uses of ExecutionContext are adjusted to conform to the above. 3. The SBThread.cpp-defined helper function ResumeNewPlan now expects a locked ProcessRunLock as _proof_ that the execution is stopped. It will unlock the mutex prior to resuming the process. This commit exposes many opportunities for early-returns, but these would increase the diff of this patch and distract from the important changes, so we opt not to do it here.
1 parent 7fb8a44 commit f12e038

File tree

8 files changed

+832
-777
lines changed

8 files changed

+832
-777
lines changed

lldb/include/lldb/API/SBError.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,7 @@ class LLDB_API SBError {
8787
friend class SBDebugger;
8888
friend class SBFile;
8989
friend class SBFormat;
90+
friend class SBFrame;
9091
friend class SBHostOS;
9192
friend class SBPlatform;
9293
friend class SBProcess;

lldb/include/lldb/API/SBFrame.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -233,6 +233,10 @@ class LLDB_API SBFrame {
233233

234234
void SetFrameSP(const lldb::StackFrameSP &lldb_object_sp);
235235

236+
/// Return an SBValue containing an error message that warns the process is
237+
/// not currently stopped.
238+
static SBValue CreateProcessIsRunningExprEvalError();
239+
236240
lldb::ExecutionContextRefSP m_opaque_sp;
237241
};
238242

lldb/include/lldb/Host/ProcessRunLock.h

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,9 +41,22 @@ class ProcessRunLock {
4141
class ProcessRunLocker {
4242
public:
4343
ProcessRunLocker() = default;
44+
ProcessRunLocker(ProcessRunLocker &&other) : m_lock(other.m_lock) {
45+
other.m_lock = nullptr;
46+
}
47+
ProcessRunLocker &operator=(ProcessRunLocker &&other) {
48+
if (this != &other) {
49+
Unlock();
50+
m_lock = other.m_lock;
51+
other.m_lock = nullptr;
52+
}
53+
return *this;
54+
}
4455

4556
~ProcessRunLocker() { Unlock(); }
4657

58+
bool IsLocked() const { return m_lock; }
59+
4760
// Try to lock the read lock, but only do so if there are no writers.
4861
bool TryLock(ProcessRunLock *lock) {
4962
if (m_lock) {

lldb/include/lldb/Target/ExecutionContext.h

Lines changed: 48 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111

1212
#include <mutex>
1313

14+
#include "lldb/Host/ProcessRunLock.h"
1415
#include "lldb/Target/StackID.h"
1516
#include "lldb/lldb-private.h"
1617

@@ -315,14 +316,6 @@ class ExecutionContext {
315316
ExecutionContext(const ExecutionContextRef *exe_ctx_ref,
316317
bool thread_and_frame_only_if_stopped = false);
317318

318-
// These two variants take in a locker, and grab the target, lock the API
319-
// mutex into locker, then fill in the rest of the shared pointers.
320-
ExecutionContext(const ExecutionContextRef &exe_ctx_ref,
321-
std::unique_lock<std::recursive_mutex> &locker)
322-
: ExecutionContext(&exe_ctx_ref, locker) {}
323-
324-
ExecutionContext(const ExecutionContextRef *exe_ctx_ref,
325-
std::unique_lock<std::recursive_mutex> &locker);
326319
// Create execution contexts from execution context scopes
327320
ExecutionContext(ExecutionContextScope *exe_scope);
328321
ExecutionContext(ExecutionContextScope &exe_scope);
@@ -566,6 +559,53 @@ class ExecutionContext {
566559
lldb::StackFrameSP m_frame_sp; ///< The stack frame in thread.
567560
};
568561

562+
/// A wrapper class representing an execution context with non-null Target
563+
/// and Process pointers, a locked API mutex and a locked ProcessRunLock.
564+
/// The locks are private by design: to unlock them, destroy the
565+
/// StoppedExecutionContext.
566+
struct StoppedExecutionContext : ExecutionContext {
567+
StoppedExecutionContext(lldb::TargetSP &target_sp,
568+
lldb::ProcessSP &process_sp,
569+
lldb::ThreadSP &thread_sp,
570+
lldb::StackFrameSP &frame_sp,
571+
std::unique_lock<std::recursive_mutex> api_lock,
572+
ProcessRunLock::ProcessRunLocker stop_locker)
573+
: m_api_lock(std::move(api_lock)), m_stop_locker(std::move(stop_locker)) {
574+
assert(target_sp);
575+
assert(process_sp);
576+
assert(m_api_lock.owns_lock());
577+
assert(m_stop_locker.IsLocked());
578+
SetTargetSP(target_sp);
579+
SetProcessSP(process_sp);
580+
SetThreadSP(thread_sp);
581+
SetFrameSP(frame_sp);
582+
}
583+
584+
/// Transfers ownership of the locks from `other` to `this`, making `other`
585+
/// unusable.
586+
StoppedExecutionContext(StoppedExecutionContext &&other)
587+
: StoppedExecutionContext(other.m_target_sp, other.m_process_sp,
588+
other.m_thread_sp, other.m_frame_sp,
589+
std::move(other.m_api_lock),
590+
std::move(other.m_stop_locker)) {
591+
other.Clear();
592+
}
593+
594+
/// Clears this context, unlocking the ProcessRunLock and returning the
595+
/// locked API lock. Like after a move operation, this object is no longer
596+
/// usable.
597+
[[nodiscard]] std::unique_lock<std::recursive_mutex> Destroy();
598+
599+
private:
600+
std::unique_lock<std::recursive_mutex> m_api_lock;
601+
ProcessRunLock::ProcessRunLocker m_stop_locker;
602+
};
603+
604+
llvm::Expected<StoppedExecutionContext>
605+
GetStoppedExecutionContext(const ExecutionContextRef *exe_ctx_ref_ptr);
606+
llvm::Expected<StoppedExecutionContext>
607+
GetStoppedExecutionContext(const lldb::ExecutionContextRefSP &exe_ctx_ref_ptr);
608+
569609
} // namespace lldb_private
570610

571611
#endif // LLDB_TARGET_EXECUTIONCONTEXT_H

0 commit comments

Comments
 (0)