formatting support for `Subprocess::shellify`

Summary:
Adding formatting support with automatic escaping of parameters. Will
subsequentyl try and make it harder for people to use non-statically determined
format strings for this.

Reviewed By: simpkins

Differential Revision: D3866174

fbshipit-source-id: 80bb5886386692643876116fbf338ca0d6986e6a

folly/Makefile.am

@@ -313,6 +313,7 @@ nobase_follyinclude_HEADERS = \
 	RWSpinLock.h \
 	ScopeGuard.h \
 	SharedMutex.h \
+	Shell.h \
 	Singleton.h \
 	Singleton-inl.h \
 	SingletonThreadLocal.h \

folly/Shell.h

+/*
+ * Copyright 2016 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+/**
+ * `Shell` provides a collection of functions to use with `Subprocess` that make
+ * it easier to safely run processes in a unix shell.
+ *
+ * Note: use this rarely and carefully. By default you should use `Subprocess`
+ * with a vector of arguments.
+ */
+
+#pragma once
+
+#include <string>
+#include <vector>
+
+#include <folly/Format.h>
+#include <folly/Range.h>
+
+namespace folly {
+
+/**
+ * Quotes an argument to make it suitable for use as shell command arguments.
+ */
+std::string shellQuote(StringPiece argument) {
+  std::string quoted = "'";
+  for (auto c : argument) {
+    if (c == '\'') {
+      quoted += "'\\''";
+    } else {
+      quoted += c;
+    }
+  }
+  return quoted + "'";
+}
+
+/**
+  * 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 format string should always be a string literal to protect against
+  * shell injections. Arguments will automatically be escaped with `'`.
+  *
+  * TODO(dominik): find a way to ensure statically determined format strings.
+  */
+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};
+}
+
+} // folly

folly/Subprocess.cpp

@@ -37,6 +37,7 @@
 #include <folly/Conv.h>
 #include <folly/Exception.h>
 #include <folly/ScopeGuard.h>
+#include <folly/Shell.h>
 #include <folly/String.h>
 #include <folly/io/Cursor.h>
 #include <folly/portability/Environment.h>
@@ -183,26 +184,10 @@ Subprocess::Subprocess(
     throw std::invalid_argument("usePath() not allowed when running in shell");
   }
 
-  auto argv = Subprocess::shellify(cmd);
+  auto argv = 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";
-  }
-
-  argv.push_back(shell);
-  argv.push_back("-c");
-  argv.push_back(cmd);
-
-  return argv;
-}
-
 Subprocess::~Subprocess() {
   CHECK_NE(returnCode_.state(), ProcessReturnCode::RUNNING)
     << "Subprocess destroyed without reaping child";

folly/Subprocess.h

@@ -462,8 +462,6 @@ class Subprocess {
       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.

folly/test/ShellTest.cpp

+/*
+ * Copyright 2016 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <glog/logging.h>
+#include <gtest/gtest.h>
+
+#include <folly/Shell.h>
+
+using namespace folly;
+
+TEST(Shell, ShellQuote) {
+  EXPECT_EQ(shellQuote("a"), "'a'");
+  EXPECT_EQ(shellQuote("a'b"), "'a'\\''b'");
+  EXPECT_EQ(shellQuote("a\"b"), "'a\"b'");
+}
+
+TEST(Shell, Shellify) {
+  auto command = shellify("rm -rf /");
+  EXPECT_EQ(command[1], "-c");
+  EXPECT_EQ(command[2], "rm -rf /");
+
+  command = shellify("rm -rf {}", "someFile.txt");
+  EXPECT_EQ(command[2], "rm -rf 'someFile.txt'");
+
+  command = shellify("rm -rf {}", 5);
+  EXPECT_EQ(command[2], "rm -rf '5'");
+
+  command = shellify("ls {}", "blah'; rm -rf /");
+  EXPECT_EQ(command[2], "ls 'blah'\\''; rm -rf /'");
+}

folly/test/SubprocessTest.cpp

@@ -531,9 +531,3 @@ 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 /");
-}