debugserver: Propagate environment in launch-mode (pr35671)

[labath]

Summary

Make sure we propagate environment when starting debugserver with a pre-loaded
inferior. AFAIK, RNBRunLoopLaunchInferior is only called in pre-loaded inferior
scenario, so we can just pick up the debugserver environment instead of trying
to construct an envp from the (empty) context.

This makes debugserver pass an test added for an equivalent lldb-server fix.

[labath]

re-clang-format the patch

[clayborg]

We already have an option for this named "--forward-env". It might be better to fix this by adding the "--forward-env" argument to the debugserver launch during testing? When we launch through LLDB, it forwards all environment variables manually. Maybe we add a "--forward-env" option to lldb-server too? Not sure what the right thing to do here is. iOS, tvOS and watchOS won't be affected since we never launch directly and all env vars are manually set by LLDB. Jason, any comments on this one?

[labath]

In D41352#958549, @clayborg wrote:

We already have an option for this named "--forward-env". It might be better to fix this by adding the "--forward-env" argument to the debugserver launch during testing? When we launch through LLDB, it forwards all environment variables manually. Maybe we add a "--forward-env" option to lldb-server too? Not sure what the right thing to do here is. iOS, tvOS and watchOS won't be affected since we never launch directly and all env vars are manually set by LLDB. Jason, any comments on this one?

Ah, I haven't noticed that. I can definitely add the --forward-env to the test if you that's the behavior you want. However, I think that the default behavior of inheriting the environment for the "launch" mode makes more sense.

[clayborg]

I am ok either way, but I do want to make sure Jason gets input before we do anything. He might be out for a few weeks over the break, so there might be a delay.

[jasonmolenda]

I guess I don't have an opinion on this one. The correct way to pass environment variables to the inferior is through SBLaunchInfo::SetEnvironmentEntries or in cmd line lldb, process launch -v ENV=val. A test that assumes an environment variable set in lldb will end up in the inferior process isn't going to work when the process is launched on a remote target, is it?

Whether llgs or debugserver should be forwarding their environment variables on by default - it seems fragile to me. But maybe there's a use case that needs this behavior to be supported? I guess it's valid to test that it actually behaves that way, at least for processes launched on the local system.

(I apologize for not keeping up with lldb-commits the past week while this was work was being done -- I'm in the middle of a task where I've had to concentrate on that & pause reading emails etc for a bit. ;)

[labath]

In D41352#959051, @jasonmolenda wrote:

I guess I don't have an opinion on this one. The correct way to pass environment variables to the inferior is through SBLaunchInfo::SetEnvironmentEntries or in cmd line lldb, process launch -v ENV=val. A test that assumes an environment variable set in lldb will end up in the inferior process isn't going to work when the process is launched on a remote target, is it?

First a bit of background: We have an internal customer who uses lldb-server ... -- ./my_app --my_app_arguments + process connect ... combo for their debugging setup (mainly because this is compatible with their existing gdb setup). However, their app was failing to run because it was expecting some environment variables to be set, and lldb-server was launching it with a clean environment.

In this setup, it is not possible to set the environment variables via SBLaunchInfo, as lldb (client) is not responsible for launching the inferior -- as far as the client goes this situation is most similar to an "attach".

Whether llgs or debugserver should be forwarding their environment variables on by default - it seems fragile to me. But maybe there's a use case that needs this behavior to be supported? I guess it's valid to test that it actually behaves that way, at least for processes launched on the local system.

Note that I am only suggesting to pass the environment in the "pre-loaded inferior" mode (I'm not sure if it has an official name). I agree that the (regular) case where the inferior is launched under the control of the client via $A packets should remain the same, as in this case the client is able to setup the environment exactly as it wants to. However, in the pre-loaded mode we have just two options: launch with the environment inherited from the lldb-server and launch with an empty environment. Of these, I think the first one is much more useful.

The test we are speaking about is also only testing that the environment passing works in the pre-loaded mode. (Right now this test cannot be run remotely, but that's a separate issue which I plan to address in the future.)

(I apologize for not keeping up with lldb-commits the past week while this was work was being done -- I'm in the middle of a task where I've had to concentrate on that & pause reading emails etc for a bit. ;)

No worries, I am not in a rush with this.

[clayborg]

So the main question is what do we expect to happen by default. I kind of agree that if we launch the inferior directly when launching I would expect the environment to be passed along. Jason: since we always just launch debugserver for Xcode in the mode that doesn't expect environment variables to be passed along, I think changing the default behavior would be a good idea and it would only affect internal Apple customers. What do you think? We might need to add a "--no-forward-env" option in case anyone doesn't want this behavior just in case if we do switch the default.

[jasonmolenda]

Thanks for the background Pavel. I'm fine with changing the default behavior. I don't see this having any impact on users of debugserver, and if it does, we can add a flag like Greg suggests here.

[clayborg]

The existing code for the "--forward-env" does this:

case 'F':
  // Pass the current environment down to the process that gets launched
  {
    char **host_env = *_NSGetEnviron();
    char *env_entry;
    size_t i;
    for (i = 0; (env_entry = host_env[i]) != NULL; ++i)
      remote->Context().PushEnvironment(env_entry);
  }
  break;

Seems like your fix doesn't do the remote->Context().PushEnvironment(...). Not sure if this is needed or not.

[clayborg]

So we can also specify extra environment variable using the "--env" option with debugserver and your current fix will break that.

[clayborg]

Environment vars might be set by using --env, so those need to go into "inferior_envp" first. If we are launching, we will copy only the host environment vars that haven't been already set manually (they don't already exist in the inferior_envp).

[clayborg]

This is exposing a bug where if we have options like:

% debugserver --env USER=hello --forward-env -- /bin/ls -lAF

We would set USER to hello, then "--forward-env" would clobber the setting...

[labath]

New version: Make sure we respect variables set by --env and that they are not
overridden by --forward-env (the last part relies on the fact that in the
presence of multiply-defined environment variables, getenv() will pick the
first one).

[clayborg]

tools/debugserver/source/debugserver.cpp

1426 ↗

(On Diff #127681)

We need to check if the env var is already in the environment in remote->Context() first before pushing the new version. I would assume that if you exec a program with an env like:

FOO=bar
USER=me
FOO=baz

That FOO=baz will end up being the value that is used. If the user specifies things with --env, we will just overwrite it. We might add a PushEnvironmentIfNeeded() function to the Context() class that will make sure it isn't in the list first and push it only if needed.

[labath]

tools/debugserver/source/debugserver.cpp

1426 ↗

(On Diff #127681)

That isn't what happens. The execve() will just forward both definitions (you can see this by exec-ing /usr/bin/env, which will print both), but getenv() will return the first one. (at least on darwin and linux, but I expect others to behave the same).

I tried this out as while working on D41359, I got the impression that there is some confusion about what the code expected to happen when it called env.AppendArguments() -- e.g. should SBLaunchInfo::SetEnvironmentEntries(..., append=true) overwrite the existing entries or not? Based on these experiments, I replaced AppendArguments call with env.insert(...) (which does not overwrite), but it's possible some of these should actually by replaced by insert_or_assign..

That said, I agree that relying on this behavior here is dodgy. I'll see if I can remove it.

unittests/tools/lldb-server/tests/LLGSTest.cpp

35 ↗

(On Diff #127681)

This test shows that the --env below wins over this value from the parent environment (the test inferior will check it's own environment via getenv and report the result via exit status)

[labath]

Add PushEnvironmentIfNeeded to avoid creating duplicate entries in the environment.

[clayborg]

Thanks for all the revisions. Looks good.

lldb/trunk/tools/debugserver/source/RNBContext.h

public:
   void ClearArgv() { m_arg_vec.erase(m_arg_vec.begin(), m_arg_vec.end()); }
 
   size_t EnvironmentCount() const { return m_env_vec.size(); }
   const char *EnvironmentAtIndex(size_t index);
   void PushEnvironment(const char *arg) {
     if (arg)
       m_env_vec.push_back(arg);
   }
+  void PushEnvironmentIfNeeded(const char *arg);
   void ClearEnvironment() {
     m_env_vec.erase(m_env_vec.begin(), m_env_vec.end());
   }
   DNBError &LaunchStatus() { return m_launch_status; }
   const char *LaunchStatusAsString(std::string &s);
   nub_launch_flavor_t LaunchFlavor() const { return m_launch_flavor; }
   void SetLaunchFlavor(nub_launch_flavor_t flavor) { m_launch_flavor = flavor; }
 

lldb/trunk/tools/debugserver/source/RNBContext.cpp

 
 const char *RNBContext::EnvironmentAtIndex(size_t index) {
   if (index < m_env_vec.size())
     return m_env_vec[index].c_str();
   else
     return NULL;
 }
 
+static std::string GetEnvironmentKey(const std::string &env) {
+  std::string key = env.substr(0, env.find('='));
+  if (!key.empty() && key.back() == '=')
+    key.pop_back();
+  return key;
+}
+
+void RNBContext::PushEnvironmentIfNeeded(const char *arg) {
+  if (!arg)
+    return;
+  std::string arg_key = GetEnvironmentKey(arg);
+
+  for (const std::string &entry: m_env_vec) {
+    if (arg_key == GetEnvironmentKey(entry))
+      return;
+  }
+  m_env_vec.push_back(arg);
+}
+
 const char *RNBContext::ArgumentAtIndex(size_t index) {
   if (index < m_arg_vec.size())
     return m_arg_vec[index].c_str();
   else
     return NULL;
 }
 
 bool RNBContext::SetWorkingDirectory(const char *path) {

lldb/trunk/tools/debugserver/source/debugserver.cpp

 
 #if __GLIBC__
   optind = 0;
 #else
   optreset = 1;
   optind = 1;
 #endif
 
+  bool forward_env = false;
   while ((ch = getopt_long_only(argc, argv, short_options, g_long_options,
                                 &long_option_index)) != -1) {
     DNBLogDebug("option: ch == %c (0x%2.2x) --%s%c%s\n", ch, (uint8_t)ch,
                 g_long_options[long_option_index].name,
                 g_long_options[long_option_index].has_arg ? '=' : ' ',
                 optarg ? optarg : "");
     switch (ch) {
     case 0: // Any optional that auto set themselves will return 0
#endif
 
     case 'e':
       // Pass a single specified environment variable down to the process that
       // gets launched
       remote->Context().PushEnvironment(optarg);
       break;
 
     case 'F':
-      // Pass the current environment down to the process that gets launched
-      {
-        char **host_env = *_NSGetEnviron();
-        char *env_entry;
-        size_t i;
-        for (i = 0; (env_entry = host_env[i]) != NULL; ++i)
-          remote->Context().PushEnvironment(env_entry);
-      }
+      forward_env = true;
       break;
 
     case '2':
       // File descriptor passed to this process during fork/exec and is already
       // open and ready for communication.
       communication_fd = atoi(optarg);
       break;
     }
if (argc == 0 || g_lockdown_opt) {
         ctx.PushArgument(argv[i]);
       }
     }
   }
 
   if (start_mode == eRNBRunLoopModeExit)
     return -1;
 
+  if (forward_env || start_mode == eRNBRunLoopModeInferiorLaunching) {
+    // Pass the current environment down to the process that gets launched
+    // This happens automatically in the "launching" mode. For the rest, we
+    // only do that if the user explicitly requested this via --forward-env
+    // argument.
+    char **host_env = *_NSGetEnviron();
+    char *env_entry;
+    size_t i;
+    for (i = 0; (env_entry = host_env[i]) != NULL; ++i)
+      remote->Context().PushEnvironmentIfNeeded(env_entry);
+  }
+
   RNBRunLoopMode mode = start_mode;
   char err_str[1024] = {'\0'};
 
   while (mode != eRNBRunLoopModeExit) {
     switch (mode) {
     case eRNBRunLoopModeGetStartModeFromRemoteProtocol:
 #ifdef WITH_LOCKDOWN
       if (g_lockdown_opt) {

lldb/trunk/unittests/tools/lldb-server/CMakeLists.txt

 set(ALL_LLDB_TEST_EXECUTABLES)
 
 function(add_lldb_test_executable test_name)
   set(EXCLUDE_FROM_ALL ON)
   add_llvm_executable(${test_name} NO_INSTALL_RPATH ${ARGN})
   set(outdir ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR})
   set_output_directory(${test_name} BINARY_DIR ${outdir} LIBRARY_DIR ${outdir})
   list(APPEND ALL_LLDB_TEST_EXECUTABLES ${test_name})
   set(ALL_LLDB_TEST_EXECUTABLES ${ALL_LLDB_TEST_EXECUTABLES} PARENT_SCOPE)
 endfunction()
 
 add_lldb_test_executable(thread_inferior inferior/thread_inferior.cpp)
 add_lldb_test_executable(environment_check inferior/environment_check.cpp)
 
 if (CMAKE_SYSTEM_NAME MATCHES "Darwin")
-  add_definitions(-DLLDB_SERVER="$<TARGET_FILE:debugserver>")
+  add_definitions(-DLLDB_SERVER="$<TARGET_FILE:debugserver>" -DLLDB_SERVER_IS_DEBUGSERVER=1)
 else()
-  add_definitions(-DLLDB_SERVER="$<TARGET_FILE:lldb-server>")
+  add_definitions(-DLLDB_SERVER="$<TARGET_FILE:lldb-server>" -DLLDB_SERVER_IS_DEBUGSERVER=0)
 endif()
 
 add_definitions(
   -DLLDB_TEST_INFERIOR_PATH="${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR}"
   -DLLDB_TEST_INFERIOR_SUFFIX="${CMAKE_EXECUTABLE_SUFFIX}"
   )
 add_subdirectory(tests)

lldb/trunk/unittests/tools/lldb-server/tests/LLGSTest.cpp

 #include "lldb/Host/Host.h"
 #include "llvm/Testing/Support/Error.h"
 
 using namespace llgs_tests;
 using namespace lldb_private;
 using namespace llvm;
 
 TEST_F(TestBase, LaunchModePreservesEnvironment) {
-  if (TestClient::IsDebugServer()) {
-    GTEST_LOG_(WARNING) << "Test fails with debugserver: llvm.org/pr35671";
-    return;
-  }
-
   putenv(const_cast<char *>("LLDB_TEST_MAGIC_VARIABLE=LLDB_TEST_MAGIC_VALUE"));
 
   auto ClientOr = TestClient::launch(getLogFileName(),
                                      {getInferiorPath("environment_check")});
   ASSERT_THAT_EXPECTED(ClientOr, Succeeded());
   auto &Client = **ClientOr;
 
   ASSERT_THAT_ERROR(Client.ContinueAll(), Succeeded());
   ASSERT_THAT_EXPECTED(
       Client.GetLatestStopReplyAs<StopReplyExit>(),
       HasValue(testing::Property(&StopReply::getKind,
                                  WaitStatus{WaitStatus::Exit, 0})));
 }
+
+TEST_F(TestBase, DS_TEST(DebugserverEnv)) {
+  // Test that --env takes precedence over inherited environment variables.
+  putenv(const_cast<char *>("LLDB_TEST_MAGIC_VARIABLE=foobar"));
+
+  auto ClientOr = TestClient::launchCustom(getLogFileName(),
+      { "--env", "LLDB_TEST_MAGIC_VARIABLE=LLDB_TEST_MAGIC_VALUE" },
+                                     {getInferiorPath("environment_check")});
+  ASSERT_THAT_EXPECTED(ClientOr, Succeeded());
+  auto &Client = **ClientOr;
+
+  ASSERT_THAT_ERROR(Client.ContinueAll(), Succeeded());
+  ASSERT_THAT_EXPECTED(
+      Client.GetLatestStopReplyAs<StopReplyExit>(),
+      HasValue(testing::Property(&StopReply::getKind,
+                                 WaitStatus{WaitStatus::Exit, 0})));
+}

lldb/trunk/unittests/tools/lldb-server/tests/TestClient.h

 #include "lldb/Utility/ArchSpec.h"
 #include "lldb/Utility/Connection.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/FormatVariadic.h"
 #include <memory>
 #include <string>
 
+#if LLDB_SERVER_IS_DEBUGSERVER
+#define LLGS_TEST(x) DISABLED_ ## x
+#define DS_TEST(x) x
+#else
+#define LLGS_TEST(x) x
+#define DS_TEST(x) DISABLED_ ## x
+#endif
+
 namespace llgs_tests {
 class TestClient
     : public lldb_private::process_gdb_remote::GDBRemoteCommunicationClient {
 public:
-  static bool IsDebugServer();
-  static bool IsLldbServer();
+  static bool IsDebugServer() { return LLDB_SERVER_IS_DEBUGSERVER; }
+  static bool IsLldbServer() { return !IsDebugServer(); }
 
   /// Launches the server, connects it to the client and returns the client. If
   /// Log is non-empty, the server will write it's log to this file.
   static llvm::Expected<std::unique_ptr<TestClient>> launch(llvm::StringRef Log);
 
   /// Launches the server, while specifying the inferior on its command line.
   /// When the client connects, it already has a process ready.
   static llvm::Expected<std::unique_ptr<TestClient>>
   launch(llvm::StringRef Log, llvm::ArrayRef<llvm::StringRef> InferiorArgs);
 
+  /// Allows user to pass additional arguments to the server. Be careful when
+  /// using this for generic tests, as the two stubs have different
+  /// command-line interfaces.
+  static llvm::Expected<std::unique_ptr<TestClient>>
+  launchCustom(llvm::StringRef Log, llvm::ArrayRef<llvm::StringRef> ServerArgs, llvm::ArrayRef<llvm::StringRef> InferiorArgs);
+
+
   ~TestClient() override;
   llvm::Error SetInferior(llvm::ArrayRef<std::string> inferior_args);
   llvm::Error ListThreadsInStopReply();
   llvm::Error SetBreakpoint(unsigned long address);
   llvm::Error ContinueAll();
   llvm::Error ContinueThread(unsigned long thread_id);
   const ProcessInfo &GetProcessInfo();
   llvm::Optional<JThreadsInfo> GetJThreadsInfo();

lldb/trunk/unittests/tools/lldb-server/tests/TestClient.cpp

 #include <sstream>
 #include <string>
 
 using namespace lldb;
 using namespace lldb_private;
 using namespace llvm;
 
 namespace llgs_tests {
-bool TestClient::IsDebugServer() {
-  return sys::path::filename(LLDB_SERVER).contains("debugserver");
-}
-
-bool TestClient::IsLldbServer() { return !IsDebugServer(); }
 
 TestClient::TestClient(std::unique_ptr<Connection> Conn) {
   SetConnection(Conn.release());
 
   SendAck(); // Send this as a handshake.
 }
 
 TestClient::~TestClient() {
   if (!IsConnected())
     return;
 
   std::string response;
   // Debugserver (non-conformingly?) sends a reply to the k packet instead of
   // simply closing the connection.
   PacketResult result =
       IsDebugServer() ? PacketResult::Success : PacketResult::ErrorDisconnected;
   EXPECT_THAT_ERROR(SendMessage("k", response, result), Succeeded());
 }
 
 Expected<std::unique_ptr<TestClient>> TestClient::launch(StringRef Log) {
   return launch(Log, {});
 }
 
 Expected<std::unique_ptr<TestClient>> TestClient::launch(StringRef Log, ArrayRef<StringRef> InferiorArgs) {
+  return launchCustom(Log, {}, InferiorArgs);
+}
+
+Expected<std::unique_ptr<TestClient>> TestClient::launchCustom(StringRef Log, ArrayRef<StringRef> ServerArgs, ArrayRef<StringRef> InferiorArgs) {
   const ArchSpec &arch_spec = HostInfo::GetArchitecture();
   Args args;
   args.AppendArgument(LLDB_SERVER);
   if (IsLldbServer())
     args.AppendArgument("gdbserver");
   args.AppendArgument("--reverse-connect");
 
   if (!Log.empty()) {
     args.AppendArgument(("--log-file=" + Log).str());
     if (IsLldbServer())
       args.AppendArgument("--log-channels=gdb-remote packets");
     else
       args.AppendArgument("--log-flags=0x800000");
   }
 
   Status status;
   TCPSocket listen_socket(true, false);
   status = listen_socket.Listen("127.0.0.1:0", 5);
   if (status.Fail())
     return status.ToError();
 
   args.AppendArgument(
       ("localhost:" + Twine(listen_socket.GetLocalPortNumber())).str());
 
+  for (StringRef arg : ServerArgs)
+    args.AppendArgument(arg);
+
   if (!InferiorArgs.empty()) {
     args.AppendArgument("--");
     for (StringRef arg : InferiorArgs)
       args.AppendArgument(arg);
   }
 
   ProcessLaunchInfo Info;
   Info.SetArchitecture(arch_spec);