Skip to content

[lldb] Don't crash when attaching to pid and no binaries found #100287

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

jasonmolenda
Copy link
Collaborator

There is a narrow window during process launch on macOS where lldb can attach and no binaries will be seen as loaded in the process (none reported by libdyld SPI). A year ago I made changes to set the new-binary-loaded breakpoint correctly despite this. But we've seen a crash when this combination is seen, where CommandObjectProcessAttach::DoExecute assumed there was at least one binary registered in the Target. Fix that.

Also fix two FileSpec API uses from when we didn't have a GetPath() method that returned a std::string, and was copying the filepaths into fixed length buffers. All of this code was from ~14 years ago when we didn't have that API.

rdar://131631627

There is a narrow window during process launch on macOS where
lldb can attach and no binaries will be seen as loaded in the
process (none reported by libdyld SPI).  A year ago I made
changes to set the new-binary-loaded breakpoint correctly despite
this.  But we've seen a crash when this combination is seen, where
CommandObjectProcessAttach::DoExecute assumed there was at least
one binary registered in the Target.  Fix that.

Also fix two FileSpec API uses from when we didn't have a GetPath()
method that returned a std::string, and was copying the filepaths
into fixed length buffers.  All of this code was from ~14 years ago
when we didn't have that API.

rdar://131631627
@llvmbot
Copy link
Member

llvmbot commented Jul 24, 2024

@llvm/pr-subscribers-lldb

Author: Jason Molenda (jasonmolenda)

Changes

There is a narrow window during process launch on macOS where lldb can attach and no binaries will be seen as loaded in the process (none reported by libdyld SPI). A year ago I made changes to set the new-binary-loaded breakpoint correctly despite this. But we've seen a crash when this combination is seen, where CommandObjectProcessAttach::DoExecute assumed there was at least one binary registered in the Target. Fix that.

Also fix two FileSpec API uses from when we didn't have a GetPath() method that returned a std::string, and was copying the filepaths into fixed length buffers. All of this code was from ~14 years ago when we didn't have that API.

rdar://131631627


Full diff: https://github.com/llvm/llvm-project/pull/100287.diff

1 Files Affected:

  • (modified) lldb/source/Commands/CommandObjectProcess.cpp (+8-10)
diff --git a/lldb/source/Commands/CommandObjectProcess.cpp b/lldb/source/Commands/CommandObjectProcess.cpp
index 50695af556939..e605abdb3c771 100644
--- a/lldb/source/Commands/CommandObjectProcess.cpp
+++ b/lldb/source/Commands/CommandObjectProcess.cpp
@@ -369,25 +369,23 @@ class CommandObjectProcessAttach : public CommandObjectProcessLaunchOrAttach {
 
     // Okay, we're done.  Last step is to warn if the executable module has
     // changed:
-    char new_path[PATH_MAX];
     ModuleSP new_exec_module_sp(target->GetExecutableModule());
     if (!old_exec_module_sp) {
       // We might not have a module if we attached to a raw pid...
       if (new_exec_module_sp) {
-        new_exec_module_sp->GetFileSpec().GetPath(new_path, PATH_MAX);
-        result.AppendMessageWithFormat("Executable module set to \"%s\".\n",
-                                       new_path);
+        result.AppendMessageWithFormat(
+            "Executable binary set to \"%s\".\n",
+            new_exec_module_sp->GetFileSpec().GetPath().c_str());
       }
+    } else if (!new_exec_module_sp) {
+      result.AppendWarningWithFormat("No executable binary.");
     } else if (old_exec_module_sp->GetFileSpec() !=
                new_exec_module_sp->GetFileSpec()) {
-      char old_path[PATH_MAX];
-
-      old_exec_module_sp->GetFileSpec().GetPath(old_path, PATH_MAX);
-      new_exec_module_sp->GetFileSpec().GetPath(new_path, PATH_MAX);
 
       result.AppendWarningWithFormat(
-          "Executable module changed from \"%s\" to \"%s\".\n", old_path,
-          new_path);
+          "Executable binary changed from \"%s\" to \"%s\".\n",
+          old_exec_module_sp->GetFileSpec().GetPath().c_str(),
+          new_exec_module_sp->GetFileSpec().GetPath().c_str());
     }
 
     if (!old_arch_spec.IsValid()) {

Copy link
Collaborator

@jimingham jimingham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jasonmolenda jasonmolenda merged commit 56535a0 into llvm:main Jul 24, 2024
8 checks passed
@jasonmolenda jasonmolenda deleted the dont-crash-if-attach-has-no-binaries-loaded branch July 24, 2024 00:50
yuxuanchen1997 pushed a commit that referenced this pull request Jul 25, 2024
Summary:
There is a narrow window during process launch on macOS where lldb can
attach and no binaries will be seen as loaded in the process (none
reported by libdyld SPI). A year ago I made changes to set the
new-binary-loaded breakpoint correctly despite this. But we've seen a
crash when this combination is seen, where
CommandObjectProcessAttach::DoExecute assumed there was at least one
binary registered in the Target. Fix that.

Also fix two FileSpec API uses from when we didn't have a GetPath()
method that returned a std::string, and was copying the filepaths into
fixed length buffers. All of this code was from ~14 years ago when we
didn't have that API.

rdar://131631627

Test Plan: 

Reviewers: 

Subscribers: 

Tasks: 

Tags: 


Differential Revision: https://phabricator.intern.facebook.com/D60250573
@AZero13
Copy link
Contributor

AZero13 commented Aug 2, 2024

Should this be backported to 19.x?

jasonmolenda added a commit to jasonmolenda/llvm-project that referenced this pull request Aug 27, 2024
…100287)

There is a narrow window during process launch on macOS where lldb can
attach and no binaries will be seen as loaded in the process (none
reported by libdyld SPI). A year ago I made changes to set the
new-binary-loaded breakpoint correctly despite this. But we've seen a
crash when this combination is seen, where
CommandObjectProcessAttach::DoExecute assumed there was at least one
binary registered in the Target. Fix that.

Also fix two FileSpec API uses from when we didn't have a GetPath()
method that returned a std::string, and was copying the filepaths into
fixed length buffers. All of this code was from ~14 years ago when we
didn't have that API.

rdar://131631627
(cherry picked from commit 56535a0)
jasonmolenda added a commit to swiftlang/llvm-project that referenced this pull request Aug 27, 2024
…found-dont-crash

[lldb] Don't crash when attaching to pid and no binaries found (llvm#100287)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants