From 9637519ede462c78b28ca7d2866429bba57a2996 Mon Sep 17 00:00:00 2001 From: Dominik Gabi Date: Thu, 15 Sep 2016 07:57:15 -0700 Subject: [PATCH] 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 --- folly/Shell.h | 9 ++------- folly/test/ShellTest.cpp | 1 + 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/folly/Shell.h b/folly/Shell.h index 56df3d27..c7aca9b7 100644 --- 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 std::vector shellify( const StringPiece format, Arguments&&... arguments) { - const char* shell = getenv("SHELL"); - if (!shell) { - shell = "/bin/sh"; - } auto command = sformat( format, shellQuote(to(std::forward(arguments)))...); - return {shell, "-c", command}; + return {"/bin/sh", "-c", command}; } } // folly diff --git a/folly/test/ShellTest.cpp b/folly/test/ShellTest.cpp index d129d5ba..00ddc043 100644 --- 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 /"); -- 2.34.1