Skip to content

Commit 301d8ac

Browse files
[lldb] Guard SBFrame/SBThread methods against running processes
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 849daa4 commit 301d8ac

File tree

7 files changed

+508
-386
lines changed

7 files changed

+508
-386
lines changed

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 with an error message warning that the process is not
237+
/// currently stopped.
238+
static SBValue CreateProcessIsRunningExprEvalError();
239+
236240
lldb::ExecutionContextRefSP m_opaque_sp;
237241
};
238242

lldb/include/lldb/Host/ProcessRunLock.h

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,9 +41,14 @@ 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+
}
4447

4548
~ProcessRunLocker() { Unlock(); }
4649

50+
bool IsLocked() const { return m_lock; }
51+
4752
// Try to lock the read lock, but only do so if there are no writers.
4853
bool TryLock(ProcessRunLock *lock) {
4954
if (m_lock) {

lldb/include/lldb/Target/ExecutionContext.h

Lines changed: 6 additions & 3 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

@@ -318,11 +319,13 @@ class ExecutionContext {
318319
// These two variants take in a locker, and grab the target, lock the API
319320
// mutex into locker, then fill in the rest of the shared pointers.
320321
ExecutionContext(const ExecutionContextRef &exe_ctx_ref,
321-
std::unique_lock<std::recursive_mutex> &locker)
322-
: ExecutionContext(&exe_ctx_ref, locker) {}
322+
std::unique_lock<std::recursive_mutex> &locker,
323+
ProcessRunLock::ProcessRunLocker &stop_locker)
324+
: ExecutionContext(&exe_ctx_ref, locker, stop_locker) {}
323325

324326
ExecutionContext(const ExecutionContextRef *exe_ctx_ref,
325-
std::unique_lock<std::recursive_mutex> &locker);
327+
std::unique_lock<std::recursive_mutex> &locker,
328+
ProcessRunLock::ProcessRunLocker &stop_locker);
326329
// Create execution contexts from execution context scopes
327330
ExecutionContext(ExecutionContextScope *exe_scope);
328331
ExecutionContext(ExecutionContextScope &exe_scope);

0 commit comments

Comments
 (0)