ignore `$SHELL` in `shellify`

Summary:
Why `$SHELL` is a bad idea:

- getenv() is not thread safe. (In practice it should work if other threads aren't calling setenv(), but still seems undesirable if we can avoid it.)
- It seems confusing for the program to have different behavior for different developers.
- Other shells besides /bin/sh may have different quoting behaviors. For instance, I don't think csh allows unescaped newlines inside single quotes.
- SHELL might be set to other non-shell-like programs in some cases. (Say, if this gets run from inside a git or mercurial hook it might end up being set to the restricted git or hg shell that only lets you run specific commands.)
Anyway, this isn't related to your diff, so nothing needs to be done for now,
but I would vote for changing this to always use /bin/sh in a future diff.

Both the C system() call and python's subprocess module appear to
always use /bin/sh and ignore $SHELL.

Reviewed By: simpkins

Differential Revision: D3867047

fbshipit-source-id: dab0e6afbe1c20ff13d9a52f212df95af425dd77

diff --git a/folly/Shell.h b/folly/Shell.h
index 56df3d278498dbd994bd2fe03f21fbe79a1beb2d..c7aca9b79194fc78f6ca81eaa954d8e58e1b3c27 100644 (file)
--- a/folly/Shell.h
+++ b/folly/Shell.h
@@ -51,8 +51,7 @@ std::string shellQuote(StringPiece argument) {
   * Create argument array for `Subprocess()` for a process running in a
   * shell.
   *
-  * The shell to use is taken from the environment variable $SHELL,
-  * or /bin/sh if $SHELL is unset.
+  * The shell to use is always going to be `/bin/sh`.
   *
   * The format string should always be a string literal to protect against
   * shell injections. Arguments will automatically be escaped with `'`.
@@ -63,14 +62,10 @@ template <typename... Arguments>
 std::vector<std::string> shellify(
     const StringPiece format,
     Arguments&&... arguments) {
-  const char* shell = getenv("SHELL");
-  if (!shell) {
-    shell = "/bin/sh";
-  }
   auto command = sformat(
       format,
       shellQuote(to<std::string>(std::forward<Arguments>(arguments)))...);
-  return {shell, "-c", command};
+  return {"/bin/sh", "-c", command};
 }
 
 } // folly

diff --git a/folly/test/ShellTest.cpp b/folly/test/ShellTest.cpp
index d129d5ba01fa02678f032d51717b0900241e36c1..00ddc043a5bab35686b3026db02f127e054b5cb2 100644 (file)
--- a/folly/test/ShellTest.cpp
+++ b/folly/test/ShellTest.cpp
@@ -29,6 +29,7 @@ TEST(Shell, ShellQuote) {
 
 TEST(Shell, Shellify) {
   auto command = shellify("rm -rf /");
+  EXPECT_EQ(command[0], "/bin/sh");
   EXPECT_EQ(command[1], "-c");
   EXPECT_EQ(command[2], "rm -rf /");