[Host] Return the user's shell from GetDefaultShell

[JDevlieghere]

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.

[friss]

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]

Check pointer returned by getpwuid.

[JDevlieghere]

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]

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]

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.

lldb/trunk/lit/Host/Inputs/simple.c

+int main(int argc, char const *argv[]) { return 0; }

lldb/trunk/lit/Host/TestCustomShell.test

+# UNSUPPORTED: system-windows
+
+# RUN: %clang %S/Inputs/simple.c -g -o %t.out
+# RUN: SHELL=bogus %lldb %t.out -b -o 'run' 2>&1 | FileCheck %s --check-prefix ERROR
+# RUN: env -i %lldb %t.out -b -o 'run' 2>&1 | FileCheck %s
+
+# ERROR: error: shell expansion failed
+# CHECK-NOT: error: shell expansion failed

lldb/trunk/source/Host/posix/HostInfoPosix.cpp

 namespace {
 class PosixUserIDResolver : public UserIDResolver {
 protected:
   llvm::Optional<std::string> DoGetUserName(id_t uid) override;
   llvm::Optional<std::string> DoGetGroupName(id_t gid) override;
 };
 } // namespace
 
-llvm::Optional<std::string> PosixUserIDResolver::DoGetUserName(id_t uid) {
+struct PasswdEntry {
+  std::string username;
+  std::string shell;
+};
+
+static llvm::Optional<PasswdEntry> GetPassword(id_t uid) {
 #ifdef USE_GETPWUID
   // getpwuid_r is missing from android-9
-  // UserIDResolver provides some thread safety by making sure noone calls this
-  // function concurrently, but using getpwuid is ultimately not thread-safe as
-  // we don't know who else might be calling it.
-  struct passwd *user_info_ptr = ::getpwuid(uid);
-  if (user_info_ptr)
-    return std::string(user_info_ptr->pw_name);
+  // The caller should provide some thread safety by making sure no one calls
+  // this function concurrently, because using getpwuid is ultimately not
+  // thread-safe as we don't know who else might be calling it.
+  if (auto *user_info_ptr = ::getpwuid(uid))
+    return PasswdEntry{user_info_ptr->pw_name, user_info_ptr->pw_shell};
 #else
   struct passwd user_info;
   struct passwd *user_info_ptr = &user_info;
   char user_buffer[PATH_MAX];
   size_t user_buffer_size = sizeof(user_buffer);
   if (::getpwuid_r(uid, &user_info, user_buffer, user_buffer_size,
                    &user_info_ptr) == 0 &&
       user_info_ptr) {
-    return std::string(user_info_ptr->pw_name);
+    return PasswdEntry{user_info_ptr->pw_name, user_info_ptr->pw_shell};
   }
 #endif
   return llvm::None;
 }
 
+llvm::Optional<std::string> PosixUserIDResolver::DoGetUserName(id_t uid) {
+  if (llvm::Optional<PasswdEntry> password = GetPassword(uid))
+    return password->username;
+  return llvm::None;
+}
+
 llvm::Optional<std::string> PosixUserIDResolver::DoGetGroupName(id_t gid) {
 #ifndef __ANDROID__
   char group_buffer[PATH_MAX];
   size_t group_buffer_size = sizeof(group_buffer);
   struct group group_info;
   struct group *group_info_ptr = &group_info;
   // Try the threadsafe version first
   if (::getgrgid_r(gid, &group_info, group_buffer, group_buffer_size,

 uint32_t HostInfoPosix::GetUserID() { return getuid(); }
 
 uint32_t HostInfoPosix::GetGroupID() { return getgid(); }
 
 uint32_t HostInfoPosix::GetEffectiveUserID() { return geteuid(); }
 
 uint32_t HostInfoPosix::GetEffectiveGroupID() { return getegid(); }
 
-FileSpec HostInfoPosix::GetDefaultShell() { return FileSpec("/bin/sh"); }
+FileSpec HostInfoPosix::GetDefaultShell() {
+  if (const char *v = ::getenv("SHELL"))
+    return FileSpec(v);
+  if (llvm::Optional<PasswdEntry> password = GetPassword(::geteuid()))
+    return FileSpec(password->shell);
+  return FileSpec("/bin/sh");
+}
 
 bool HostInfoPosix::ComputeSupportExeDirectory(FileSpec &file_spec) {
   return ComputePathRelativeToLibrary(file_spec, "/bin");
 }
 
 bool HostInfoPosix::ComputeHeaderDirectory(FileSpec &file_spec) {
   FileSpec temp_file("/opt/local/include/lldb");
   file_spec.GetDirectory().SetCString(temp_file.GetPath().c_str());