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

1424

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

1424

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

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.

tools/debugserver/source/debugserver.cpp

RNBRunLoopMode RNBRunLoopLaunchInferior(RNBRemote *remote,
   size_t inferior_argc = ctx.ArgumentCount();
   // Initialize inferior_argv with inferior_argc + 1 NULLs
   std::vector<const char *> inferior_argv(inferior_argc + 1, NULL);
 
   size_t i;
   for (i = 0; i < inferior_argc; i++)
     inferior_argv[i] = ctx.ArgumentAtIndex(i);
 
-  // Pass the environment array the same way:
-
-  size_t inferior_envc = ctx.EnvironmentCount();
-  // Initialize inferior_argv with inferior_argc + 1 NULLs
-  std::vector<const char *> inferior_envp(inferior_envc + 1, NULL);
-
-  for (i = 0; i < inferior_envc; i++)
-    inferior_envp[i] = ctx.EnvironmentAtIndex(i);
-
   // Our launch type hasn't been set to anything concrete, so we need to
   // figure our how we are going to launch automatically.
 
   nub_launch_flavor_t launch_flavor = g_launch_flavor;
   if (launch_flavor == eLaunchFlavorDefault) {
     // Our default launch method is posix spawn
     launch_flavor = eLaunchFlavorPosixSpawn;
 
#endif
 
   char launch_err_str[PATH_MAX];
   launch_err_str[0] = '\0';
   const char *cwd =
       (ctx.GetWorkingDirPath() != NULL ? ctx.GetWorkingDirPath()
                                        : ctx.GetWorkingDirectory());
   const char *process_event = ctx.GetProcessEvent();
   nub_process_t pid = DNBProcessLaunch(
-      resolved_path, &inferior_argv[0], &inferior_envp[0], cwd, stdin_path,
-      stdout_path, stderr_path, no_stdio, launch_flavor, g_disable_aslr,
-      process_event, launch_err_str, sizeof(launch_err_str));
+      resolved_path, &inferior_argv[0],
+      const_cast<const char **>(*_NSGetEnviron()), cwd, stdin_path, stdout_path,
+      stderr_path, no_stdio, launch_flavor, g_disable_aslr, process_event,
+      launch_err_str, sizeof(launch_err_str));
 
   g_pid = pid;
 
   if (pid == INVALID_NUB_PROCESS && strlen(launch_err_str) > 0) {
     DNBLogThreaded("%s DNBProcessLaunch() returned error: '%s'", __FUNCTION__,
                    launch_err_str);
     ctx.LaunchStatus().SetError(-1, DNBError::Generic);
     ctx.LaunchStatus().SetErrorString(launch_err_str);
#endif
   char err_str[1024] = {'\0'};
 
   while (mode != eRNBRunLoopModeExit) {
     switch (mode) {
     case eRNBRunLoopModeGetStartModeFromRemoteProtocol:
 #ifdef WITH_LOCKDOWN
       if (g_lockdown_opt) {
         if (!remote->Comm().IsConnected()) {
           if (remote->Comm().ConnectToService() != rnb_success) {
             RNBLogSTDERR(
                 "Failed to get connection from a remote gdb process.\n");
             mode = eRNBRunLoopModeExit;
           } else if (g_applist_opt != 0) {
             // List all applications we are able to see
             std::string applist_plist;
             if (ListApplications(applist_plist, false, false) == 0) {
               DNBLogDebug("Task list: %s", applist_plist.c_str());

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})));
 }