Page MenuHomePhabricator

[Host] Return the user's shell from GetDefaultShell
ClosedPublic

Authored by JDevlieghere on Oct 1 2019, 8:08 PM.

Details

Summary

LLDB handles shell expansion by running lldb-argdumper under a shell. Currently, this is always /bin/sh on POSIX. This potentially leads to different behavior between lldb and the user's current shell. Here's an example of different expansions between shells:

$ /bin/bash -c 'echo -config={Options:[{key:foo_key,value:foo_value}]}'
-config={Options:[key:foo_key]} -config={Options:[value:foo_value]}
$ /bin/zsh -c 'echo -config={Options:[{key:foo_key,value:foo_value}]}'
zsh:1: no matches found: -config={Options:[key:foo_key]}
$ /bin/sh -c 'echo -config={Options:[{key:foo_key,value:foo_value}]}'
-config={Options:[key:foo_key]} -config={Options:[value:foo_value]}
$ /bin/fish -c 'echo -config={Options:[{key:foo_key,value:foo_value}]}'
-config=Options:[key:foo_key] -config=Options:[value:foo_value]

To reduce surprises, this patch returns the user's current shell. It first looks at the SHELL environment variable. If that isn't set, it'll ask for the user's default shell. Only if that fails, we'll fallback to /bin/sh, which should always be available.

Diff Detail

Repository
rL LLVM

Event Timeline

JDevlieghere created this revision.Oct 1 2019, 8:08 PM
JDevlieghere edited the summary of this revision. (Show Details)
friss added inline comments.Oct 1 2019, 8:15 PM
lldb/lit/Host/TestCustomShell.test
5 ↗(On Diff #222745)

Is there a reliable way to check that the expansion we get in lldb matches the one in the shell? For example, could we have the program dump its arguments once without lldb and match them against the lldb output?

I guess the zsh example that errors out in your description makes this hard?

lldb/source/Host/posix/HostInfoPosix.cpp
119 ↗(On Diff #222745)

the dereference seems a little careless. getpwuid can fail and return NULL.

JDevlieghere marked an inline comment as done.

Check pointer returned by getpwuid.

JDevlieghere added inline comments.Oct 1 2019, 11:08 PM
lldb/lit/Host/TestCustomShell.test
5 ↗(On Diff #222745)

You could run lldb-argdumper under lldb and compare the output to running it under different shells. Do you think it's reasonable to assume that at least /bin/bash and /bin/zsh are available?

JDevlieghere marked an inline comment as done.Oct 1 2019, 11:08 PM
JDevlieghere added inline comments.
lldb/lit/Host/TestCustomShell.test
5 ↗(On Diff #222745)

Assuming we can find something that expands differently without throwing an error in bash and zsh.

labath added inline comments.Oct 2 2019, 1:23 AM
lldb/lit/Host/TestCustomShell.test
5 ↗(On Diff #222745)

I don't think you can assume zsh is universally available. If you wanted to do something like, the safest option would be to write a mock shell, which implements some dumb substitution (s/FOO/BAR/) and otherwise forwards to the real shell. But, I wouldn't say that is required, as that is not even testing what this patch is changing. A simpler but still interesting test might be to unset the SHELL variable to ensure that the getpwuid path is at least executed (as one can't really check it's result reasonably).

lldb/source/Host/posix/HostInfoPosix.cpp
119 ↗(On Diff #222745)

Also, getpwuid is not thread safe. There's already code in this very file which chooses between getpwuid and getpwuid_r, but it is hidden inside PosixUserIDResolver::DoGetUserName. If you factor that out into a helper function, you'll be able to always use the best available API.

JDevlieghere marked 4 inline comments as done.
This revision was not accepted when it landed; it landed in state Needs Review.Oct 3 2019, 11:28 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 3 2019, 11:28 AM