deprecate `folly::Subprocess(std::string, ...)`

Summary:
introducing `Subprocess::shellify` to get around this. Still thinking
about how to make sure people escape arguments passing into that function. I'd
like to have something along the lines of Phabricator's `csprintf` here.

Reviewed By: yfeldblum

Differential Revision: D3857827

fbshipit-source-id: 8afbc9f1c62c62e0fc91782e11b808145b370933

diff --git a/folly/Subprocess.cpp b/folly/Subprocess.cpp
index d7d28d9f4b150dd58375023bbf78ef85f4341105..d6883392c680054b54f7e93f101bce1d570a831d 100644 (file)
--- a/folly/Subprocess.cpp
+++ b/folly/Subprocess.cpp
@@ -182,17 +182,25 @@ Subprocess::Subprocess(
   if (options.usePath_) {
     throw std::invalid_argument("usePath() not allowed when running in shell");
   }
+
+  auto argv = Subprocess::shellify(cmd);
+  spawn(cloneStrings(argv), argv[0].c_str(), options, env);
+}
+
+/* static */ std::vector<std::string> Subprocess::shellify(
+    const std::string& cmd) {
+  std::vector<std::string> argv;
+
   const char* shell = getenv("SHELL");
   if (!shell) {
     shell = "/bin/sh";
   }
 
-  std::unique_ptr<const char*[]> argv(new const char*[4]);
-  argv[0] = shell;
-  argv[1] = "-c";
-  argv[2] = cmd.c_str();
-  argv[3] = nullptr;
-  spawn(std::move(argv), shell, options, env);
+  argv.push_back(shell);
+  argv.push_back("-c");
+  argv.push_back(cmd);
+
+  return argv;
 }
 
 Subprocess::~Subprocess() {

diff --git a/folly/Subprocess.h b/folly/Subprocess.h
index 8dabfc09af326047ae766ca1824e9e017f3a56cc..0300fd8efdeac65f711e2fb5c503ccf83535671d 100644 (file)
--- a/folly/Subprocess.h
+++ b/folly/Subprocess.h
@@ -456,11 +456,14 @@ class Subprocess {
    * The shell to use is taken from the environment variable $SHELL,
    * or /bin/sh if $SHELL is unset.
    */
+  FOLLY_DEPRECATED("Prefer not running in a shell or use `shellify`.")
   explicit Subprocess(
       const std::string& cmd,
       const Options& options = Options(),
       const std::vector<std::string>* env = nullptr);
 
+  static std::vector<std::string> shellify(const std::string& cmd);
+
   ////
   //// The methods below only manipulate the process state, and do not
   //// affect its communication pipes.

diff --git a/folly/test/SubprocessTest.cpp b/folly/test/SubprocessTest.cpp
index 66b1831957420556ca4c514deb3aaa919c65dcf7..419db0689d569dd36e207d4d5340c335d33c8644 100644 (file)
--- a/folly/test/SubprocessTest.cpp
+++ b/folly/test/SubprocessTest.cpp
@@ -531,3 +531,9 @@ TEST(CommunicateSubprocessTest, TakeOwnershipOfPipes) {
   buf[2] = 0;
   EXPECT_EQ("3\n", std::string(buf));
 }
+
+TEST(Subprocess, Shellify) {
+  auto argv = Subprocess::shellify("rm -rf /");
+  EXPECT_EQ(argv[1], "-c");
+  EXPECT_EQ(argv[2], "rm -rf /");
+}