[lldb] Respect LaunchInfo::SetExecutable in ProcessLauncherPosixFork

[labath]

Using argv[0] for this was incorrect. I'm ignoring LaunchInfo::SetArg0, as that's what darwin and windows launchers do (they use the first element of the args vector instead).

I picked up the funny unit test re-exec method from the llvm unit tests.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Pavel Labath (labath)

Changes

Using argv[0] for this was incorrect. I'm ignoring LaunchInfo::SetArg0, as that's what darwin and windows launchers do (they use the first element of the args vector instead).

I picked up the funny unit test re-exec method from the llvm unit tests.

---

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

2 Files Affected:

• (modified) lldb/source/Host/posix/ProcessLauncherPosixFork.cpp (+6-2)
• (modified) lldb/unittests/Host/HostTest.cpp (+42-1)

diff --git a/lldb/source/Host/posix/ProcessLauncherPosixFork.cpp b/lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
index 3e956290c3055..8c6d503fc7fe2 100644
--- a/lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
+++ b/lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
@@ -94,6 +94,7 @@ struct ForkLaunchInfo {
   bool debug;
   bool disable_aslr;
   std::string wd;
+  std::string executable;
   const char **argv;
   Environment::Envp envp;
   std::vector<ForkFileAction> actions;
@@ -194,7 +195,8 @@ struct ForkLaunchInfo {
   }
 
   // Execute.  We should never return...
-  execve(info.argv[0], const_cast<char *const *>(info.argv), info.envp);
+  execve(info.executable.c_str(), const_cast<char *const *>(info.argv),
+         info.envp);
 
 #if defined(__linux__)
   if (errno == ETXTBSY) {
@@ -207,7 +209,8 @@ struct ForkLaunchInfo {
     // Since this state should clear up quickly, wait a while and then give it
     // one more go.
     usleep(50000);
-    execve(info.argv[0], const_cast<char *const *>(info.argv), info.envp);
+    execve(info.executable.c_str(), const_cast<char *const *>(info.argv),
+           info.envp);
   }
 #endif
 
@@ -236,6 +239,7 @@ ForkLaunchInfo::ForkLaunchInfo(const ProcessLaunchInfo &info)
       debug(info.GetFlags().Test(eLaunchFlagDebug)),
       disable_aslr(info.GetFlags().Test(eLaunchFlagDisableASLR)),
       wd(info.GetWorkingDirectory().GetPath()),
+      executable(info.GetExecutableFile().GetPath()),
       argv(info.GetArguments().GetConstArgumentVector()),
       envp(info.GetEnvironment().getEnvp()), actions(MakeForkActions(info)) {}
 
diff --git a/lldb/unittests/Host/HostTest.cpp b/lldb/unittests/Host/HostTest.cpp
index a1d8a3b7f485a..ed1df6de001ea 100644
--- a/lldb/unittests/Host/HostTest.cpp
+++ b/lldb/unittests/Host/HostTest.cpp
@@ -7,12 +7,24 @@
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Host/Host.h"
+#include "TestingSupport/SubsystemRAII.h"
+#include "lldb/Host/FileSystem.h"
+#include "lldb/Host/ProcessLaunchInfo.h"
 #include "lldb/Utility/ProcessInfo.h"
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Testing/Support/Error.h"
 #include "gtest/gtest.h"
+#include <future>
 
 using namespace lldb_private;
 using namespace llvm;
 
+// From TestMain.cpp.
+extern const char *TestMainArgv0;
+
+static cl::opt<uint64_t> test_arg("test-arg");
+
 TEST(Host, WaitStatusFormat) {
   EXPECT_EQ("W01", formatv("{0:g}", WaitStatus{WaitStatus::Exit, 1}).str());
   EXPECT_EQ("X02", formatv("{0:g}", WaitStatus{WaitStatus::Signal, 2}).str());
@@ -45,4 +57,33 @@ TEST(Host, ProcessInstanceInfoCumulativeSystemTimeIsValid) {
   EXPECT_TRUE(info.CumulativeSystemTimeIsValid());
   info.SetCumulativeSystemTime(ProcessInstanceInfo::timespec{1, 0});
   EXPECT_TRUE(info.CumulativeSystemTimeIsValid());
-}
\ No newline at end of file
+}
+
+TEST(Host, LaunchProcessSetsArgv0) {
+  SubsystemRAII<FileSystem> subsystems;
+
+  static constexpr StringLiteral TestArgv0 = "HelloArgv0";
+  if (test_arg != 0) {
+    // In subprocess
+    if (TestMainArgv0 != TestArgv0) {
+      errs() << formatv("Got '{0}' for argv[0]\n", TestMainArgv0);
+      exit(1);
+    }
+    exit(0);
+  }
+
+  ProcessLaunchInfo info;
+  info.SetExecutableFile(
+      FileSpec(llvm::sys::fs::getMainExecutable(TestMainArgv0, &test_arg)),
+      /*add_exe_file_as_first_arg=*/false);
+  info.GetArguments().AppendArgument("HelloArgv0");
+  info.GetArguments().AppendArgument(
+      "--gtest_filter=Host.LaunchProcessSetsArgv0");
+  info.GetArguments().AppendArgument("--test-arg=47");
+  std::promise<int> exit_status;
+  info.SetMonitorProcessCallback([&](lldb::pid_t pid, int signal, int status) {
+    exit_status.set_value(status);
+  });
+  ASSERT_THAT_ERROR(Host::LaunchProcess(info).takeError(), Succeeded());
+  ASSERT_THAT(exit_status.get_future().get(), 0);
+}

[labath]

@yuvald-sweet-security

[JDevlieghere]

This is consistent with the rest of the file, but any reason these are std::strings and not FileSpecs?

[labath]

They are accessed from the forked process (line 34 in this file), so they need to be async-signal safe (nothing really guarantees std::string is that, but it's pretty hard to write a c_str implementation that isn't).

[JDevlieghere]

Makes sense, thanks for explaining!

[yuvald-sweet-security]

@yuvald-sweet-security

Thanks. This patch means we're still ignoring the arg0 set by SetArg0, right? and just using whatever was in the standard GetArguments vector. Maybe it's worth updating the comment I pointed at (

llvm-project/lldb/include/lldb/Utility/ProcessInfo.h

Lines 122 to 125 in b231f6f

	std::string m_arg0; // argv[0] if supported. If empty, then use m_executable.
	// Not all process plug-ins support specifying an argv[0] that differs from
	// the resolved platform executable (which is in m_executable)
	Args m_arguments; // All program arguments except argv[0]

) so that this is less confusing?

[labath]

@yuvald-sweet-security

Thanks. This patch means we're still ignoring the arg0 set by SetArg0, right? and just using whatever was in the standard GetArguments vector. Maybe it's worth updating the comment I pointed at (

llvm-project/lldb/include/lldb/Utility/ProcessInfo.h

Lines 122 to 125 in b231f6f

	std::string m_arg0; // argv[0] if supported. If empty, then use m_executable.
	// Not all process plug-ins support specifying an argv[0] that differs from
	// the resolved platform executable (which is in m_executable)
	Args m_arguments; // All program arguments except argv[0]

) so that this is less confusing?

Yeah, what can I say.. welcome to lldb? The tricky part there is what to say. I believe the comments there are actually match how certain parts of the code use this structure. In particular, I think it matches this code here. It's just that the ProcessLauncher code does something different (but with this patch, at least it does so consistently :/)

[DavidSpickett]

/cherry-pick 39e7efe

[llvmbot]

/pull-request #134079