This is an automated email from the ASF dual-hosted git repository.
bmahler pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/mesos.git
commit ca719052531904d5b04d3958e8185d88cc927fdb
Author: Benjamin Mahler <bmah...@apache.org>
AuthorDate: Wed Apr 1 19:52:37 2020 -0400
Moved documentation into os/shell.hpp interface.
The os/shell.hpp wrapper provides a single place to read the interface
and documentation for both POSIX and Windows, this makes it easier
for us to document the interface and OS differences that a caller
needs to know about in one place.
Review: https://reviews.apache.org/r/72304
---
3rdparty/stout/include/stout/os/posix/shell.hpp | 36 +------------
3rdparty/stout/include/stout/os/shell.hpp | 66 ++++++++++++++++++++++-
3rdparty/stout/include/stout/os/windows/shell.hpp | 24 +--------
3 files changed, 67 insertions(+), 59 deletions(-)
diff --git a/3rdparty/stout/include/stout/os/posix/shell.hpp
b/3rdparty/stout/include/stout/os/posix/shell.hpp
index 96dd280..9b47201 100644
--- a/3rdparty/stout/include/stout/os/posix/shell.hpp
+++ b/3rdparty/stout/include/stout/os/posix/shell.hpp
@@ -44,29 +44,7 @@ constexpr const char* arg1 = "-c";
} // namespace Shell {
-/**
- * Runs a shell command with optional arguments.
- *
- * This assumes that a successful execution will result in the exit code
- * for the command to be `EXIT_SUCCESS`; in this case, the contents
- * of the `Try` will be the contents of `stdout`.
- *
- * If the exit code is non-zero or the process was signaled, we will
- * return an appropriate error message; but *not* `stderr`.
- *
- * If the caller needs to examine the contents of `stderr` it should
- * be redirected to `stdout` (using, e.g., "2>&1 || true" in the command
- * string). The `|| true` is required to obtain a success exit
- * code in case of errors, and still obtain `stderr`, as piped to
- * `stdout`.
- *
- * @param fmt the formatting string that contains the command to execute
- * in the underlying shell.
- * @param t optional arguments for `fmt`.
- *
- * @return the output from running the specified command with the shell; or
- * an error message if the command's exit code is non-zero.
- */
+
template <typename... T>
Try<std::string> shell(const std::string& fmt, const T&... t)
{
@@ -116,18 +94,6 @@ Try<std::string> shell(const std::string& fmt, const T&...
t)
}
-// Executes a command by calling "/bin/sh -c <command>", and returns
-// after the command has been completed. Returns the exit code on success
-// and `None` on error (e.g., fork/exec/waitpid failed). This function
-// is async signal safe. We return an `Option<int>` instead of a `Try<int>`,
-// because although `Try` does not dynamically allocate, `Error` uses
-// `std::string`, which is not async signal safe.
-//
-// Note: Be cautious about shell injection
-// (https://en.wikipedia.org/wiki/Code_injection#Shell_injection)
-// when using this method and use proper validation and sanitization
-// on the `command`. For this reason in general `os::spawn` is
-// preferred if a shell is not required.
inline Option<int> system(const std::string& command)
{
pid_t pid = ::fork();
diff --git a/3rdparty/stout/include/stout/os/shell.hpp
b/3rdparty/stout/include/stout/os/shell.hpp
index 33cc8a6..2c378b7 100644
--- a/3rdparty/stout/include/stout/os/shell.hpp
+++ b/3rdparty/stout/include/stout/os/shell.hpp
@@ -13,7 +13,6 @@
#ifndef __STOUT_OS_SHELL_HPP__
#define __STOUT_OS_SHELL_HPP__
-
// For readability, we minimize the number of #ifdef blocks in the code by
// splitting platform specific system calls into separate directories.
#ifdef __WINDOWS__
@@ -22,5 +21,70 @@
#include <stout/os/posix/shell.hpp>
#endif // __WINDOWS__
+namespace os {
+
+// Executes a shell command (formatted in an sprintf manner) in a subprocess.
+//
+// Blocks until the subprocess terminates and returns the stdout
+// (but not stderr!) from running the specified command with the
+// shell; or an error message if the command's exit code is non-zero
+// or the process exited from a signal.
+//
+// POSIX: Performs a `popen(command.c_str(), "r")` and reads the
+// output until EOF or reading fails. Note that this is not an
+// async signal safe function.
+//
+// Windows: Forks and executes a "cmd /c <command>" subprocess.
+// TODO(bmahler): Rather than passing the command string to cmd /c,
+// this function is incorrectly quoting / escaping it as if it's
+// executing a program compatible with `CommandLineToArgvW`. cmd.exe
+// Does not use `CommandLineToArgvW` and has its own parsing, which
+// means we should not be adding extra quoting and the caller should
+// make sure that the command is correctly escaped. See MESOS-10093.
+//
+// Note: Be cautious about shell injection
+// (https://en.wikipedia.org/wiki/Code_injection#Shell_injection)
+// when using this method and use proper validation and sanitization
+// on the `command`. For this reason in general directly executing the
+// program using `os::spawn` is preferred if a shell is not required.
+//
+// TODO(bmahler): Embedding string formatting leads to a confusing
+// interface, why don't we let callers decide how they want to format
+// the command string?
+template <typename... T>
+Try<std::string> shell(const std::string& fmt, const T&... t);
+
+
+// Executes a shell command in a subprocess.
+//
+// Blocks until the subprocess terminates and returns the exit code of
+// the subprocess, or `None` if an error occurred (e.g., fork / exec /
+// waitpid or the Windows equivalents failed).
+//
+// POSIX: Performs a `fork()` / `execlp("sh", "-c", command, (char*)0)`
+// in an async signal safe manner. Note that this is not a pure wrapper
+// of the POSIX `system` call because the POSIX implementation ignores
+// SIGINT/SIGQUIT and blocks SIGCHLD while waiting for the child to
+// complete (which is not async-signal-safe).
+//
+// Windows: Forks and executes a "cmd /c <command>" subprocess.
+// TODO(bmahler): Rather than passing the command string to cmd /c,
+// this function is incorrectly quoting / escaping it as if it's
+// executing a program compatible with `CommandLineToArgvW`. cmd.exe
+// Does not use `CommandLineToArgvW` and has its own parsing, which
+// means we should not be adding extra quoting and the caller should
+// make sure that the command is correctly escaped. See MESOS-10093.
+//
+// Note: Be cautious about shell injection
+// (https://en.wikipedia.org/wiki/Code_injection#Shell_injection)
+// when using this method and use proper validation and sanitization
+// on the `command`. For this reason in general directly executing the
+// program using `os::spawn` is preferred if a shell is not required.
+//
+// Note that the return type is `Option<int>` rather than `Try<int>`
+// because `Error` uses an std::string which is not async signal safe.
+inline Option<int> system(const std::string& command);
+
+} // namespace os {
#endif // __STOUT_OS_SHELL_HPP__
diff --git a/3rdparty/stout/include/stout/os/windows/shell.hpp
b/3rdparty/stout/include/stout/os/windows/shell.hpp
index 3238fe1..587f061 100644
--- a/3rdparty/stout/include/stout/os/windows/shell.hpp
+++ b/3rdparty/stout/include/stout/os/windows/shell.hpp
@@ -52,20 +52,7 @@ constexpr const char* arg1 = "/c";
} // namespace Shell {
-// Runs a shell command (with `cmd.exe`) with optional arguments.
-//
-// This assumes that a successful execution will result in the exit
-// code for the command to be `0`; in this case, the contents of the
-// `Try` will be the contents of `stdout`.
-//
-// If the exit code is non-zero, we will return an appropriate error
-// message; but *not* `stderr`.
-//
-// If the caller needs to examine the contents of `stderr` it should
-// be redirected to `stdout` (using, e.g., `2>&1 || exit /b 0` in the
-// command string). The `|| exit /b 0` is required to obtain a success
-// exit code in case of errors, and still obtain `stderr`, as piped to
-// `stdout`.
+
template <typename... T>
Try<std::string> shell(const std::string& fmt, const T&... t)
{
@@ -176,15 +163,6 @@ Try<std::string> shell(const std::string& fmt, const T&...
t)
}
-// Executes a command by calling "cmd /c <command>", and returns
-// after the command has been completed. Returns the process exit
-// code on success and `None` on error.
-//
-// Note: Be cautious about shell injection
-// (https://en.wikipedia.org/wiki/Code_injection#Shell_injection)
-// when using this method and use proper validation and sanitization
-// on the `command`. For this reason in general `os::spawn` is
-// preferred if a shell is not required.
inline Option<int> system(const std::string& command)
{
return os::spawn(Shell::name, {Shell::arg0, Shell::arg1, command});
