Page MenuHomePhabricator

debugserver: Propagate environment in launch-mode (pr35671)
ClosedPublic

Authored by labath on Dec 18 2017, 7:25 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.Dec 18 2017, 7:25 AM
labath updated this revision to Diff 127358.Dec 18 2017, 7:26 AM

re-clang-format the patch

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?

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.

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.

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

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.

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 accepted this revision.Dec 19 2017, 10:20 AM

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.

This revision is now accepted and ready to land.Dec 19 2017, 10:20 AM

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 requested changes to this revision.Dec 19 2017, 10:36 AM

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

This revision now requires changes to proceed.Dec 19 2017, 10:36 AM

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).

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 updated this revision to Diff 127681.Dec 20 2017, 4:17 AM

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 added inline comments.Dec 20 2017, 10:44 AM
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 added inline comments.Dec 21 2017, 3:43 AM
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 updated this revision to Diff 127898.Dec 21 2017, 8:49 AM

Add PushEnvironmentIfNeeded to avoid creating duplicate entries in the environment.

clayborg accepted this revision.Dec 21 2017, 8:50 AM

Thanks for all the revisions. Looks good.

This revision is now accepted and ready to land.Dec 21 2017, 8:50 AM
This revision was automatically updated to reflect the committed changes.