From e69e359c24070c3b4ea2cd25f87b554e33b2a132 Mon Sep 17 00:00:00 2001 From: Dominik Gabi Date: Thu, 15 Sep 2016 07:57:14 -0700 Subject: [PATCH] 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 | 1 + folly/Shell.h | 76 +++++++++++++++++++++++++++++++++++ folly/Subprocess.cpp | 19 +-------- folly/Subprocess.h | 2 - folly/test/ShellTest.cpp | 43 ++++++++++++++++++++ folly/test/SubprocessTest.cpp | 6 --- 6 files changed, 122 insertions(+), 25 deletions(-) create mode 100644 folly/Shell.h create mode 100644 folly/test/ShellTest.cpp diff --git a/folly/Makefile.am b/folly/Makefile.am index f9c059bb..5a5f66ab 100644 --- a/folly/Makefile.am +++ b/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 \ diff --git a/folly/Shell.h b/folly/Shell.h new file mode 100644 index 00000000..56df3d27 --- /dev/null +++ b/folly/Shell.h @@ -0,0 +1,76 @@ +/* + * 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 +#include + +#include +#include + +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 +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}; +} + +} // folly diff --git a/folly/Subprocess.cpp b/folly/Subprocess.cpp index d6883392..912b54f2 100644 --- a/folly/Subprocess.cpp +++ b/folly/Subprocess.cpp @@ -37,6 +37,7 @@ #include #include #include +#include #include #include #include @@ -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 Subprocess::shellify( - const std::string& cmd) { - std::vector 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"; diff --git a/folly/Subprocess.h b/folly/Subprocess.h index 0300fd8e..a3b9f877 100644 --- a/folly/Subprocess.h +++ b/folly/Subprocess.h @@ -462,8 +462,6 @@ class Subprocess { const Options& options = Options(), const std::vector* env = nullptr); - static std::vector 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/ShellTest.cpp b/folly/test/ShellTest.cpp new file mode 100644 index 00000000..d129d5ba --- /dev/null +++ b/folly/test/ShellTest.cpp @@ -0,0 +1,43 @@ +/* + * 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 +#include + +#include + +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 /'"); +} diff --git a/folly/test/SubprocessTest.cpp b/folly/test/SubprocessTest.cpp index 419db068..66b18319 100644 --- a/folly/test/SubprocessTest.cpp +++ b/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 /"); -} -- 2.34.1