https://github.com/medismailben updated https://github.com/llvm/llvm-project/pull/195774
>From 480680e80c7d9a520b51765a04ffeb20d604d038 Mon Sep 17 00:00:00 2001 From: Med Ismail Bennani <[email protected]> Date: Fri, 5 Jun 2026 17:38:50 -0700 Subject: [PATCH] [lldb] Harden PolicyStack against cross-thread Guard misuse Two related correctness/clarity improvements to the Policy infrastructure introduced by 504a11278112: PolicyStack::Guard now stores the std::thread::id of the thread that created it. Destruction and move operations call llvm::report_fatal_error when they happen on a different thread, since the PolicyStack is thread_local: popping from the wrong thread would silently corrupt that thread's stack. Push/Pop on PolicyStack are now private. Callers go through named factories (PushPrivateState, PushPublicStateRunningExpression) that return RAII Guards. The transition factories on Policy (CreatePrivateState, CreatePublicStateRunningExpression) inherit from PolicyStack::Get().Current() and apply their named change on top, so pushed policies preserve existing stack state rather than resetting unrelated fields. CreatePublicState remains the baseline reference value (returns a default Policy{}); the stack returns to public state by popping the private guards, not by pushing a "public" policy on top. rdar://176223894 Signed-off-by: Med Ismail Bennani <[email protected]> --- lldb/include/lldb/Utility/Policy.h | 79 +++++++++++++------- lldb/source/Target/Process.cpp | 12 ++-- lldb/source/Target/StopInfo.cpp | 4 +- lldb/source/Target/Thread.cpp | 2 +- lldb/source/Utility/Policy.cpp | 56 +++++++++++++++ lldb/unittests/Utility/PolicyTest.cpp | 100 +++++++++++++++++++------- 6 files changed, 194 insertions(+), 59 deletions(-) diff --git a/lldb/include/lldb/Utility/Policy.h b/lldb/include/lldb/Utility/Policy.h index e785f13ab9287..0435bdd4a0e15 100644 --- a/lldb/include/lldb/Utility/Policy.h +++ b/lldb/include/lldb/Utility/Policy.h @@ -12,6 +12,7 @@ #include "llvm/ADT/SmallVector.h" #include <cassert> +#include <thread> namespace lldb_private { @@ -28,7 +29,7 @@ class Stream; /// top of the private unwinder stack. The private state thread must see the /// raw unwinder frames, while public clients see the augmented view. Rather /// than checking thread identity at every callsite, the private state thread -/// pushes Policy::PrivateState() and the rest follows from the policy. +/// pushes Policy::CreatePrivateState() and the rest follows from the policy. struct Policy { /// What view of the process this thread sees. enum class View { @@ -54,21 +55,16 @@ struct Policy { View view = View::Public; Capabilities capabilities; - static Policy PublicState() { return {}; } - - static Policy PrivateState() { - Policy p; - p.view = View::Private; - p.capabilities.can_load_frame_providers = false; - p.capabilities.can_run_frame_recognizers = false; - return p; - } - - static Policy PublicStateRunningExpression() { - Policy p; - p.capabilities.can_run_breakpoint_actions = false; - return p; - } + /// @name Factories + /// + /// CreatePublicState is the baseline (returns default Policy{}). The + /// transition factories below start from PolicyStack::Get().Current() and + /// apply their named change on top. + /// @{ + static Policy CreatePublicState(); + static Policy CreatePrivateState(); + static Policy CreatePublicStateRunningExpression(); + /// @} void Dump(Stream &s) const; }; @@ -79,6 +75,10 @@ struct Policy { /// initialized with a default-constructed base entry that is never popped. /// RAII guards (Guard) push and pop policies. /// +/// Policies are pushed via named factory methods (PushPrivateState, etc.) +/// that return an RAII Guard. Direct Push is private to prevent callers +/// from assembling arbitrary capability combinations. +/// /// For thread pool workers that don't inherit thread_local storage, the /// policy must be passed into the lambda and pushed onto the worker /// thread's stack when the task starts. @@ -91,26 +91,53 @@ class PolicyStack { Policy Current() const; - void Push(Policy policy) { m_stack.push_back(std::move(policy)); } - - void Pop() { - assert(!m_stack.empty() && "can't pop the base policy"); - m_stack.pop_back(); - } - void Dump(Stream &s) const; - /// RAII guard that pushes a policy on construction and pops on destruction. + /// RAII guard that pops a policy on destruction. + /// + /// A Guard is bound to the thread that created it: the policy stack lives + /// in thread_local storage, so popping from a different thread would + /// corrupt that thread's stack. Guards may be moved, but only on the + /// owning thread; a cross-thread move or destruction is a fatal error. class Guard { + friend class PolicyStack; + public: - explicit Guard(Policy policy) { Get().Push(std::move(policy)); } - ~Guard() { Get().Pop(); } + ~Guard(); + Guard(Guard &&other); + Guard &operator=(Guard &&other); Guard(const Guard &) = delete; Guard &operator=(const Guard &) = delete; + + private: + Guard() : m_thread_id(std::this_thread::get_id()), m_active(true) {} + std::thread::id m_thread_id; + bool m_active = false; }; + /// All Push* methods delegate to the named static factories on Policy, + /// which already inherit from Current(). So the pushed policy preserves + /// existing stack state instead of resetting unrelated fields. + + [[nodiscard]] Guard PushPrivateState() { + Push(Policy::CreatePrivateState()); + return Guard(); + } + + [[nodiscard]] Guard PushPublicStateRunningExpression() { + Push(Policy::CreatePublicStateRunningExpression()); + return Guard(); + } + private: + void Push(Policy policy) { m_stack.push_back(std::move(policy)); } + + void Pop() { + assert(!m_stack.empty() && "can't pop the base policy"); + m_stack.pop_back(); + } + llvm::SmallVector<Policy> m_stack = {Policy{}}; }; diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp index 1661ae5734b0a..4360250b21475 100644 --- a/lldb/source/Target/Process.cpp +++ b/lldb/source/Target/Process.cpp @@ -4372,7 +4372,7 @@ thread_result_t Process::RunPrivateStateThread(bool is_override) { // They must see parent frames, not provider-augmented frames. std::optional<PolicyStack::Guard> policy_guard; if (is_override) - policy_guard.emplace(Policy::PrivateState()); + policy_guard = PolicyStack::Get().PushPrivateState(); bool control_only = true; @@ -5427,7 +5427,7 @@ Process::RunThreadPlan(ExecutionContext &exe_ctx, // GetStackFrameList returns parent frames during event processing. std::optional<PolicyStack::Guard> policy_guard; if (backup_private_state_thread) - policy_guard.emplace(Policy::PrivateState()); + policy_guard = PolicyStack::Get().PushPrivateState(); while (true) { // We usually want to resume the process if we get to the top of the @@ -5499,10 +5499,10 @@ Process::RunThreadPlan(ExecutionContext &exe_ctx, Halt(clear_thread_plans, use_run_lock); } - diagnostic_manager.Printf( - lldb::eSeverityError, - "didn't get running event after initial resume, got %s instead.", - StateAsCString(stop_state)); + diagnostic_manager.Printf(lldb::eSeverityError, + "didn't get running event after initial " + "resume, got %s instead.", + StateAsCString(stop_state)); return_value = eExpressionSetupError; break; } diff --git a/lldb/source/Target/StopInfo.cpp b/lldb/source/Target/StopInfo.cpp index f438d368c9ba8..c20b0ed07ee3c 100644 --- a/lldb/source/Target/StopInfo.cpp +++ b/lldb/source/Target/StopInfo.cpp @@ -193,7 +193,7 @@ class StopInfoBreakpoint : public StopInfo { bool ShouldStopSynchronous(Event *event_ptr) override { // Breakpoint callbacks run on the PST during stop processing. Push // private state context so callback code sees the private reality. - PolicyStack::Guard policy_guard(Policy::PrivateState()); + PolicyStack::Guard policy_guard = PolicyStack::Get().PushPrivateState(); ThreadSP thread_sp(m_thread_wp.lock()); if (thread_sp) { @@ -903,7 +903,7 @@ class StopInfoWatchpoint : public StopInfo { bool ShouldStopSynchronous(Event *event_ptr) override { // Watchpoint callbacks run on the PST during stop processing. Push // private state context so callback code sees the private reality. - PolicyStack::Guard policy_guard(Policy::PrivateState()); + PolicyStack::Guard policy_guard = PolicyStack::Get().PushPrivateState(); // If we are running our step-over the watchpoint plan, stop if it's done // and continue if it's not: diff --git a/lldb/source/Target/Thread.cpp b/lldb/source/Target/Thread.cpp index 87ec77452f2cd..f25a14fbe92e8 100644 --- a/lldb/source/Target/Thread.cpp +++ b/lldb/source/Target/Thread.cpp @@ -1558,7 +1558,7 @@ StackFrameListSP Thread::GetStackFrameList() { return m_curr_frames_sp; // The private state thread must see the raw unwinder frames, not the - // provider-augmented public view. Policy::PrivateState is pushed by + // provider-augmented public view. Policy::CreatePrivateState is pushed by // RunThreadPlan and RunPrivateStateThread. Policy policy = PolicyStack::Get().Current(); if (policy.view == Policy::View::Private) { diff --git a/lldb/source/Utility/Policy.cpp b/lldb/source/Utility/Policy.cpp index 2abc0d6281161..104df17df7a97 100644 --- a/lldb/source/Utility/Policy.cpp +++ b/lldb/source/Utility/Policy.cpp @@ -11,6 +11,7 @@ #include "lldb/Utility/Log.h" #include "lldb/Utility/Stream.h" #include "lldb/Utility/StreamString.h" +#include "llvm/Support/ErrorHandling.h" using namespace lldb_private; @@ -24,6 +25,61 @@ Policy PolicyStack::Current() const { return p; } +// CreatePublicState is the baseline, not a transition. The stack returns to +// public state by popping the private-state guards, not by pushing a +// "public" policy on top. This factory exists only as a reference value +// (tests, dump comparisons); it never reads the current stack. +Policy Policy::CreatePublicState() { return {}; } + +Policy Policy::CreatePrivateState() { + Policy p = PolicyStack::Get().Current(); + p.view = View::Private; + p.capabilities.can_load_frame_providers = false; + p.capabilities.can_run_frame_recognizers = false; + return p; +} + +Policy Policy::CreatePublicStateRunningExpression() { + Policy p = PolicyStack::Get().Current(); + p.capabilities.can_run_breakpoint_actions = false; + return p; +} + +PolicyStack::Guard::~Guard() { + if (!m_active) + return; + if (m_thread_id != std::this_thread::get_id()) + llvm::report_fatal_error( + "PolicyStack::Guard destroyed on a different thread than the one " + "that created it"); + Get().Pop(); +} + +PolicyStack::Guard::Guard(Guard &&other) + : m_thread_id(other.m_thread_id), m_active(other.m_active) { + if (m_active && m_thread_id != std::this_thread::get_id()) + llvm::report_fatal_error("PolicyStack::Guard moved across threads"); + other.m_active = false; +} + +PolicyStack::Guard &PolicyStack::Guard::operator=(Guard &&other) { + if (this != &other) { + if (other.m_active && other.m_thread_id != std::this_thread::get_id()) + llvm::report_fatal_error("PolicyStack::Guard moved across threads"); + if (m_active) { + if (m_thread_id != std::this_thread::get_id()) + llvm::report_fatal_error( + "PolicyStack::Guard destroyed on a different thread than the " + "one that created it"); + Get().Pop(); + } + m_thread_id = other.m_thread_id; + m_active = other.m_active; + other.m_active = false; + } + return *this; +} + void Policy::Dump(Stream &s) const { s << "policy: view=" << (view == View::Public ? "public" : "private"); s << ", capabilities={"; diff --git a/lldb/unittests/Utility/PolicyTest.cpp b/lldb/unittests/Utility/PolicyTest.cpp index b7771e52b46e4..c582f16a3d7bb 100644 --- a/lldb/unittests/Utility/PolicyTest.cpp +++ b/lldb/unittests/Utility/PolicyTest.cpp @@ -26,7 +26,7 @@ TEST(PolicyTest, DefaultIsPublicWithAllCapabilities) { } TEST(PolicyTest, PublicState) { - Policy p = Policy::PublicState(); + Policy p = Policy::CreatePublicState(); EXPECT_EQ(p.view, Policy::View::Public); EXPECT_TRUE(p.capabilities.can_evaluate_expressions); EXPECT_TRUE(p.capabilities.can_run_all_threads); @@ -37,7 +37,7 @@ TEST(PolicyTest, PublicState) { } TEST(PolicyTest, PrivateState) { - Policy p = Policy::PrivateState(); + Policy p = Policy::CreatePrivateState(); EXPECT_EQ(p.view, Policy::View::Private); EXPECT_TRUE(p.capabilities.can_evaluate_expressions); EXPECT_TRUE(p.capabilities.can_run_all_threads); @@ -48,7 +48,7 @@ TEST(PolicyTest, PrivateState) { } TEST(PolicyTest, PublicStateRunningExpression) { - Policy p = Policy::PublicStateRunningExpression(); + Policy p = Policy::CreatePublicStateRunningExpression(); EXPECT_EQ(p.view, Policy::View::Public); EXPECT_TRUE(p.capabilities.can_evaluate_expressions); EXPECT_TRUE(p.capabilities.can_run_all_threads); @@ -66,20 +66,25 @@ TEST(PolicyTest, StackDefaultIsPublicState) { } TEST(PolicyTest, StackPushPop) { - PolicyStack::Get().Push(Policy::PrivateState()); - EXPECT_EQ(PolicyStack::Get().Current().view, Policy::View::Private); - EXPECT_FALSE( - PolicyStack::Get().Current().capabilities.can_load_frame_providers); + { + PolicyStack::Guard guard = PolicyStack::Get().PushPrivateState(); + EXPECT_EQ(PolicyStack::Get().Current().view, Policy::View::Private); + EXPECT_FALSE( + PolicyStack::Get().Current().capabilities.can_load_frame_providers); - PolicyStack::Get().Push(Policy::PublicStateRunningExpression()); - EXPECT_EQ(PolicyStack::Get().Current().view, Policy::View::Public); - EXPECT_FALSE( - PolicyStack::Get().Current().capabilities.can_run_breakpoint_actions); + { + PolicyStack::Guard inner = + PolicyStack::Get().PushPublicStateRunningExpression(); + // PushPublicStateRunningExpression inherits from Current() and only + // toggles bp_actions; view stays Private. + EXPECT_EQ(PolicyStack::Get().Current().view, Policy::View::Private); + EXPECT_FALSE( + PolicyStack::Get().Current().capabilities.can_run_breakpoint_actions); + } - PolicyStack::Get().Pop(); - EXPECT_EQ(PolicyStack::Get().Current().view, Policy::View::Private); + EXPECT_EQ(PolicyStack::Get().Current().view, Policy::View::Private); + } - PolicyStack::Get().Pop(); EXPECT_EQ(PolicyStack::Get().Current().view, Policy::View::Public); } @@ -87,14 +92,16 @@ TEST(PolicyTest, GuardRAII) { EXPECT_EQ(PolicyStack::Get().Current().view, Policy::View::Public); { - PolicyStack::Guard guard(Policy::PrivateState()); + PolicyStack::Guard guard = PolicyStack::Get().PushPrivateState(); EXPECT_EQ(PolicyStack::Get().Current().view, Policy::View::Private); EXPECT_FALSE( PolicyStack::Get().Current().capabilities.can_load_frame_providers); { - PolicyStack::Guard inner(Policy::PublicStateRunningExpression()); - EXPECT_EQ(PolicyStack::Get().Current().view, Policy::View::Public); + PolicyStack::Guard inner = + PolicyStack::Get().PushPublicStateRunningExpression(); + // Inherits Private view from outer guard. + EXPECT_EQ(PolicyStack::Get().Current().view, Policy::View::Private); EXPECT_FALSE( PolicyStack::Get().Current().capabilities.can_run_breakpoint_actions); } @@ -106,7 +113,7 @@ TEST(PolicyTest, GuardRAII) { } TEST(PolicyTest, StackIsPerThread) { - PolicyStack::Get().Push(Policy::PrivateState()); + PolicyStack::Guard guard = PolicyStack::Get().PushPrivateState(); Policy::View other_thread_view; std::thread t([&other_thread_view]() { @@ -116,13 +123,11 @@ TEST(PolicyTest, StackIsPerThread) { EXPECT_EQ(PolicyStack::Get().Current().view, Policy::View::Private); EXPECT_EQ(other_thread_view, Policy::View::Public); - - PolicyStack::Get().Pop(); } TEST(PolicyTest, DumpPublicState) { StreamString s; - Policy::PublicState().Dump(s); + Policy::CreatePublicState().Dump(s); EXPECT_EQ(s.GetString(), "policy: view=public, capabilities={" "eval_expr=true run_all=true try_all=true " @@ -131,7 +136,7 @@ TEST(PolicyTest, DumpPublicState) { TEST(PolicyTest, DumpPrivateState) { StreamString s; - Policy::PrivateState().Dump(s); + Policy::CreatePrivateState().Dump(s); EXPECT_EQ(s.GetString(), "policy: view=private, capabilities={" "eval_expr=true run_all=true try_all=true " @@ -139,13 +144,60 @@ TEST(PolicyTest, DumpPrivateState) { } TEST(PolicyTest, DumpStack) { - PolicyStack::Get().Push(Policy::PrivateState()); + PolicyStack::Guard guard = PolicyStack::Get().PushPrivateState(); StreamString s; PolicyStack::Get().Dump(s); EXPECT_NE(s.GetString().find("depth=2"), std::string::npos); EXPECT_NE(s.GetString().find("[0] policy: view=public"), std::string::npos); EXPECT_NE(s.GetString().find("[1] policy: view=private"), std::string::npos); +} - PolicyStack::Get().Pop(); +TEST(PolicyTest, GuardSameThreadMove) { + // Move on the same thread is fine; the moved-into Guard still pops on + // destruction and the moved-from Guard becomes a no-op. + EXPECT_EQ(PolicyStack::Get().Current().view, Policy::View::Public); + { + PolicyStack::Guard outer = PolicyStack::Get().PushPrivateState(); + EXPECT_EQ(PolicyStack::Get().Current().view, Policy::View::Private); + + PolicyStack::Guard moved = std::move(outer); + EXPECT_EQ(PolicyStack::Get().Current().view, Policy::View::Private); + } + EXPECT_EQ(PolicyStack::Get().Current().view, Policy::View::Public); +} + +#if GTEST_HAS_DEATH_TEST +TEST(PolicyStackDeathTest, GuardDestroyedOnDifferentThread) { + PolicyStack::Guard outer = PolicyStack::Get().PushPrivateState(); + // The move into the closure happens here, on the constructing thread, so + // it doesn't trip the thread-affinity check. The closure (and thus the + // Guard) is destroyed on the worker thread once it returns, which is + // where the violation is detected. + EXPECT_DEATH( + { + std::thread t([guard = std::move(outer)]() mutable { (void)guard; }); + t.join(); + }, + "PolicyStack::Guard"); +} +#endif + +TEST(PolicyTest, PushInheritsFromCurrent) { + // Push* methods inherit from Current() rather than starting from a + // default Policy: stacking PushPublicStateRunningExpression on top of + // PushPrivateState must preserve the Private view. + PolicyStack::Guard outer = PolicyStack::Get().PushPrivateState(); + EXPECT_EQ(PolicyStack::Get().Current().view, Policy::View::Private); + + PolicyStack::Guard inner = + PolicyStack::Get().PushPublicStateRunningExpression(); + // Capability from inner push. + EXPECT_FALSE( + PolicyStack::Get().Current().capabilities.can_run_breakpoint_actions); + // View inherited from outer push (would be Public if Push reset state). + EXPECT_EQ(PolicyStack::Get().Current().view, Policy::View::Private); + // Capabilities from outer push also inherited. + EXPECT_FALSE( + PolicyStack::Get().Current().capabilities.can_load_frame_providers); } _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
