[lldb] Guard stale SBProcess target access#192842
Conversation
|
@llvm/pr-subscribers-flang-fir-hlfir @llvm/pr-subscribers-lldb Author: Henry (sfu2) ChangesDescription
Reproduction
The new regression test Fix Add a nullable Full diff: https://github.com/llvm/llvm-project/pull/192842.diff 4 Files Affected:
diff --git a/lldb/include/lldb/Target/Process.h b/lldb/include/lldb/Target/Process.h
index 19b5ae3041826..ca53f4633f4d5 100644
--- a/lldb/include/lldb/Target/Process.h
+++ b/lldb/include/lldb/Target/Process.h
@@ -1244,6 +1244,11 @@ class Process : public std::enable_shared_from_this<Process>,
/// Get the target object pointer for this module.
///
+ /// \return
+ /// A Target object pointer to the target that owns this
+ /// module.
+ lldb::TargetSP GetTargetSP() const { return m_target_wp.lock(); }
+
/// \return
/// A Target object pointer to the target that owns this
/// module.
diff --git a/lldb/source/API/SBProcess.cpp b/lldb/source/API/SBProcess.cpp
index 14ce236b4f1b5..ca8d2100f4075 100644
--- a/lldb/source/API/SBProcess.cpp
+++ b/lldb/source/API/SBProcess.cpp
@@ -241,12 +241,15 @@ SBTarget SBProcess::GetTarget() const {
LLDB_INSTRUMENT_VA(this);
SBTarget sb_target;
- TargetSP target_sp;
ProcessSP process_sp(GetSP());
- if (process_sp) {
- target_sp = process_sp->GetTarget().shared_from_this();
- sb_target.SetSP(target_sp);
- }
+ if (!process_sp)
+ return sb_target;
+
+ TargetSP target_sp = process_sp->GetTargetSP();
+ if (!target_sp)
+ return sb_target;
+
+ sb_target.SetSP(target_sp);
return sb_target;
}
@@ -1274,8 +1277,14 @@ lldb::SBError SBProcess::SaveCore(SBSaveCoreOptions &options) {
return error;
}
- std::lock_guard<std::recursive_mutex> guard(
- process_sp->GetTarget().GetAPIMutex());
+ TargetSP target_sp = process_sp->GetTargetSP();
+ if (!target_sp) {
+ error = Status::FromErrorString(
+ "SBProcess is invalid because its target has been deleted");
+ return error;
+ }
+
+ std::lock_guard<std::recursive_mutex> guard(target_sp->GetAPIMutex());
if (process_sp->GetState() != eStateStopped) {
error = Status::FromErrorString("the process is not stopped");
diff --git a/lldb/unittests/API/CMakeLists.txt b/lldb/unittests/API/CMakeLists.txt
index b86054fb353f7..19a6302021d8e 100644
--- a/lldb/unittests/API/CMakeLists.txt
+++ b/lldb/unittests/API/CMakeLists.txt
@@ -3,6 +3,7 @@ add_lldb_unittest(APITests
SBLineEntryTest.cpp
SBMutexTest.cpp
SBBreakpointClearConditionTest.cpp
+ SBProcessDeleteTargetTest.cpp
SBAPITEST
diff --git a/lldb/unittests/API/SBProcessDeleteTargetTest.cpp b/lldb/unittests/API/SBProcessDeleteTargetTest.cpp
new file mode 100644
index 0000000000000..078d5aa53ed6f
--- /dev/null
+++ b/lldb/unittests/API/SBProcessDeleteTargetTest.cpp
@@ -0,0 +1,101 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// Use the umbrella header for -Wdocumentation.
+#include "lldb/API/LLDB.h"
+
+#include "TestingSupport/SubsystemRAII.h"
+#include "gtest/gtest.h"
+#include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+namespace {
+
+class SBProcessDeleteTargetTest : public testing::Test {
+protected:
+ void SetUp() override {
+ debugger = SBDebugger::Create(/*source_init_files=*/false);
+ }
+
+ void TearDown() override { SBDebugger::Destroy(debugger); }
+
+ static bool DebuggerSupportsLLVMTarget(llvm::StringRef target) {
+ SBStructuredData data = SBDebugger::GetBuildConfiguration()
+ .GetValueForKey("targets")
+ .GetValueForKey("value");
+ for (size_t i = 0; i < data.GetSize(); ++i) {
+ char buf[100] = {0};
+ size_t size = data.GetItemAtIndex(i).GetStringValue(buf, sizeof(buf));
+ if (llvm::StringRef(buf, size) == target)
+ return true;
+ }
+ return false;
+ }
+
+ static std::string GetCoreFixturePath(llvm::StringRef name) {
+ llvm::SmallString<256> path(__FILE__);
+ llvm::sys::path::remove_filename(path);
+ llvm::sys::path::append(path, "..", "..", "test", "API");
+ llvm::sys::path::append(path, "tools", "lldb-dap", "coreFile", name);
+ return std::string(path.str());
+ }
+
+ void LoadCore(SBTarget &target, SBProcess &process) {
+ std::string binary_path = GetCoreFixturePath("linux-x86_64.out");
+ std::string core_path = GetCoreFixturePath("linux-x86_64.core");
+ ASSERT_TRUE(llvm::sys::fs::exists(binary_path));
+ ASSERT_TRUE(llvm::sys::fs::exists(core_path));
+
+ SBError error;
+ target = debugger.CreateTarget(binary_path.c_str(), /*target_triple=*/"",
+ /*platform_name=*/"",
+ /*add_dependent_modules=*/false, error);
+ EXPECT_TRUE(target);
+ EXPECT_TRUE(error.Success()) << error.GetCString();
+ debugger.SetSelectedTarget(target);
+
+ process = target.LoadCore(core_path.c_str());
+ EXPECT_TRUE(process);
+ }
+
+ SubsystemRAII<lldb::SBDebugger> subsystems;
+ SBDebugger debugger;
+};
+
+} // namespace
+
+TEST_F(SBProcessDeleteTargetTest, GetTargetAndSaveCoreFailSafelyAfterDelete) {
+ if (!DebuggerSupportsLLVMTarget("X86"))
+ GTEST_SKIP() << "X86 target is not enabled";
+
+ SBTarget target;
+ SBProcess process;
+ LoadCore(target, process);
+ ASSERT_TRUE(target.IsValid());
+ ASSERT_TRUE(process.IsValid());
+ ASSERT_EQ(process.GetState(), eStateStopped);
+
+ SBSaveCoreOptions options;
+ options.SetProcess(process);
+
+ ASSERT_TRUE(debugger.DeleteTarget(target));
+ ASSERT_FALSE(target.IsValid());
+ EXPECT_FALSE(process.IsValid());
+
+ EXPECT_FALSE(process.GetTarget().IsValid());
+
+ SBError error = process.SaveCore(options);
+ EXPECT_TRUE(error.Fail());
+ EXPECT_STREQ("SBProcess is invalid because its target has been deleted",
+ error.GetCString());
+}
|
|
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
sfu2
force-pushed
the
lldb-stale-sbprocess-target
branch
from
6c49089 to
1459b97
Compare
SBSaveCoreOptions can retain a ProcessSP after DeleteTarget destroys the Target, leaving Process with a weak_ptr<Target> whose pointee has already been destroyed. Stale SBProcess handles then reach through Process::GetTarget() and crash while dereferencing that expired target relationship. Add a nullable Process::GetTargetSP() helper for SB API callers, use it to make SBProcess::GetTarget() return an invalid target and make SBProcess::SaveCore() fail with an error when the target has already been deleted. Add an API regression test that keeps a core-backed process alive through SBSaveCoreOptions, deletes the target, and verifies both paths fail safely instead of crashing.
sfu2
force-pushed
the
lldb-stale-sbprocess-target
branch
from
1459b97 to
14bf8a7
Compare
|
@jimingham My understanding was that a Process always has a Target. I would expect that deleting the target causes the process to be delated as well. Can you confirm whether that was the intended design? |
|
We did not intend that holding onto an SBProcess should keep its SBTarget alive. The SBProcess and the SBTarget both hold weak pointers to their lldb_private counterpart. An SBProcess whose weak pointer "lock's" successfully should always have a valid TargetSP. If we wanted to do the error-handling more completely, when we find we have an invalid Target in SBProcess::GetTarget, we should assert that the Process WP locks to an empty shared pointer. If the process is also invalid, then it's correct to hand out an empty SBTarget. Anything else would indicate a programming error. |
jimingham
left a comment
jimingham
left a comment
There was a problem hiding this comment.
This looks right.
For extra credit, you might check the case where you have a valid SBProcess (i.e. its process_wp locks to a valid sp) but it doesn't have a valid Target.
Having an SBProcess whose underlying lldb_private Process & Target have gone away is a thing that can happen, but having an lldb_private Process whose Target has gone away should not be possible.
|
Basically, this patch fixes the stale retained-ProcessSP case after DeleteTarget. I don’t currently see a normal path for a live non-finalized lldb_private::Process to lose its Target though. |
|
The SBProcess holds a ProcessWP, so it doesn't keep the process alive. The Target holds a ProcessSP, but it resets that in Finalize and in the Destructor. The situation you are protecting against is that the Target has been destroyed (so that Process's TargetWP wouldn't resolve to a valid SP) but the ProcessWP in SBProcess still resolves to a valid ProcessSP. That can only happen if something else must have been holding onto a ProcessSP for the process in question as well as the Target. The ProcessSP might be still alive but it has been finalized and isn't really useable at that stage, so I don't think we're doing any good handing out SBProcess objects with actual but invalid Processes. I wonder if a more complete fix here might be to have SBProcess::GetSP() also return an empty shared pointer if the ProcessSP that you get back from locking the weak pointer had been Finalized (that's what Process::IsValid() checks). That would mean that in the only cases where you might hand out a valid SBProcess it would always have a Target to hand out as well. |
|
Sure. The fix was updated so the stale retained-ProcessSP case after DeleteTarget is handled centrally in SBProcess::GetSP(). In a325beb, the regression was tightened to expect stale finalized processes to behave as invalid SBProcesss; |
jimingham
left a comment
jimingham
left a comment
There was a problem hiding this comment.
LGTM
Thanks for cleaning this up.
sfu2
requested review from
DeinAlptraum,
aaupov,
ayermolo,
lamb-j,
lanza,
maksfb,
paschalis-mpeis,
rafaelauler,
vangthao95,
yavtuk,
yota9 and
yozhu
as code owners
llvmbot
added
clang-tidy
clang:driver
|
I am terribly sorry for the noise — I accidentally clicked GitHub's sync/update button on my fork branch, which temporarily pulled a large unrelated upstream diff into this PR and caused unrelated code-owner/subscriber review requests. I have force-pushed the branch back to the intended LLDB-only changes. The remaining diff should now be limited to the SBProcess stale-target fix and regression test. I do not have permission to remove the unrelated reviewers/subscriber requests or labels. Could someone with maintainer permission please kindly clear the accidental unrelated metadata if needed? I could also open a new PR referencing this one if that's better. Sincere apologies again for the noise to everyone. |
Fix a typo in comment. Co-authored-by: Sergei Barannikov <barannikov88@gmail.com>
| TargetSP target_sp = process_sp->GetTargetSP(); | ||
| assert(target_sp && "valid SBProcess should have a target"); | ||
| if (!target_sp) { | ||
| error = Status::FromErrorString("SBProcess is invalid"); |
There was a problem hiding this comment.
Is there a typo in the error string? We're in SBProcess method and process_sp is valid at this point (checked at the beginning of this function).
There was a problem hiding this comment.
The original wording was meant at the SB API level: this handle is no longer usable as an SBProcess, even though the local process_sp is still non-null. But I agree that reads as ambiguous here. I'll fix it in the next commit.
|
|
||
| TargetSP target_sp = process_sp->GetTargetSP(); | ||
| assert(target_sp && "valid SBProcess should have a target"); | ||
| if (!target_sp) |
There was a problem hiding this comment.
I guess same applies here (but error reporting is missing?).
I feel a bit uncomfortable about reporting a user error while it is actually reports a violation of an internal contract, if I'm reading what @jimingham wrote correctly. The bug will be there forever because no one will find it unless they use a debug build of lldb. Maybe this should be lldbassert in the end. I'd like someone on the lldb team confirm or refute my reasoning (I'm new to lldb).
Most people just abandon the PR and create a new one, because now you spam everyone until they have the chance to unsubscribe manually. (Which I will now do.) |
9 participants
Description
SBSaveCoreOptionscan retain aProcessSPafterDeleteTargetdestroys theTarget, leavingProcesswith aweak_ptr<Target>whose pointee has already been destroyed. StaleSBProcesshandles then reach throughProcess::GetTarget()and crash while dereferencing that expired target relationship.Reproduction
SBDebugger.SBTargetandSBProcess.SBSaveCoreOptions. This retains the underlyingProcessSPeven after the target is deleted.SBDebugger::DeleteTarget(target). At this point the publicSBTargetis invalid, but the retainedProcessmay still outlive it.process.GetTarget().Pre-fix, this can crash because
SBProcess::GetTarget()reachesProcess::GetTarget(), which dereferences an expired weak target relationship after theTargethas already been destroyed.process.SaveCore(options).Pre-fix, this can also crash because
SBProcess::SaveCore()takes the target API mutex throughprocess_sp->GetTarget().GetAPIMutex(), which dereferences the same expired target relationship.The new regression test
SBProcessDeleteTargetTest.GetTargetAndSaveCoreFailSafelyAfterDeleteexercises this sequence with a core-backedSBProcessretained throughSBSaveCoreOptions. It reuses the checked-in core-file fixtureslldb/test/API/tools/lldb-dap/coreFile/linux-x86_64.outandlldb/test/API/tools/lldb-dap/coreFile/linux-x86_64.core, resolving them directly from the source tree at runtime.Fix
Add a nullable
Process::GetTargetSP()helper for SB API callers, use it to makeSBProcess::GetTarget()return an invalid target and makeSBProcess::SaveCore()fail with an error when the target has already been deleted. Add an API regression test that keeps a core-backed process alive throughSBSaveCoreOptions, deletes the target, and verifies both paths fail safely instead of crashing.