You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mesos.apache.org by bm...@apache.org on 2020/05/01 16:54:03 UTC
[mesos] branch master updated (db83570 -> ca71905)
This is an automated email from the ASF dual-hosted git repository.
bmahler pushed a change to branch master
in repository https://gitbox.apache.org/repos/asf/mesos.git.
from db83570 Added missing issues to the 1.7.3 CHANGELOG.
new cd66a51 Removed unecessary os::execlp wrapper.
new 9befeba Moved documentation into os/exec.hpp interface.
new ca71905 Moved documentation into os/shell.hpp interface.
The 3 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
3rdparty/stout/include/stout/os/exec.hpp | 91 +++++++++++++++++++++++
3rdparty/stout/include/stout/os/posix/exec.hpp | 20 -----
3rdparty/stout/include/stout/os/posix/shell.hpp | 36 +--------
3rdparty/stout/include/stout/os/shell.hpp | 66 +++++++++++++++-
3rdparty/stout/include/stout/os/windows/exec.hpp | 14 ----
3rdparty/stout/include/stout/os/windows/shell.hpp | 24 +-----
6 files changed, 158 insertions(+), 93 deletions(-)
[mesos] 01/03: Removed unecessary os::execlp wrapper.
Posted by bm...@apache.org.
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 cd66a51a183149e12c9cf5da8a0309213bcfaf72
Author: Benjamin Mahler <bm...@apache.org>
AuthorDate: Thu Apr 30 16:07:12 2020 -0400
Removed unecessary os::execlp wrapper.
This wrapper is deleted on windows and just directly calls through
to execlp on POSIX. The existing callsites don't prefix the call
with os:: so we can remove this wrapper for now.
Also important to note is that we probably want to remove the other
exec wrappers since on Windows they don't make much sense (requires
emulating via a subprocess, can't directly exec like on POSIX).
Review: https://reviews.apache.org/r/72456
---
3rdparty/stout/include/stout/os/posix/exec.hpp | 7 -------
3rdparty/stout/include/stout/os/windows/exec.hpp | 5 -----
2 files changed, 12 deletions(-)
diff --git a/3rdparty/stout/include/stout/os/posix/exec.hpp b/3rdparty/stout/include/stout/os/posix/exec.hpp
index db57130..dd0f42d 100644
--- a/3rdparty/stout/include/stout/os/posix/exec.hpp
+++ b/3rdparty/stout/include/stout/os/posix/exec.hpp
@@ -61,13 +61,6 @@ inline Option<int> spawn(
}
-template<typename... T>
-inline int execlp(const char* file, T... t)
-{
- return ::execlp(file, t...);
-}
-
-
inline int execvp(const char* file, char* const argv[])
{
return ::execvp(file, argv);
diff --git a/3rdparty/stout/include/stout/os/windows/exec.hpp b/3rdparty/stout/include/stout/os/windows/exec.hpp
index 89db8af..8c6f10b 100644
--- a/3rdparty/stout/include/stout/os/windows/exec.hpp
+++ b/3rdparty/stout/include/stout/os/windows/exec.hpp
@@ -442,11 +442,6 @@ inline Option<int> spawn(
}
-// TODO(bmahler): Why do we delete this on windows?
-template<typename... T>
-inline int execlp(const char* file, T... t) = delete;
-
-
// In order to emulate the semantics of `execvp`, `os::spawn` waits for the new
// process to exit, and returns its error code, which is propagated back to the
// parent via `exit` here.
[mesos] 02/03: Moved documentation into os/exec.hpp interface.
Posted by bm...@apache.org.
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 9befebac5f054657686c9a6490603afe9b7f8ce1
Author: Benjamin Mahler <bm...@apache.org>
AuthorDate: Thu Apr 30 16:08:59 2020 -0400
Moved documentation into os/exec.hpp interface.
The os/exec.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/72303/
---
3rdparty/stout/include/stout/os/exec.hpp | 91 ++++++++++++++++++++++++
3rdparty/stout/include/stout/os/posix/exec.hpp | 13 ----
3rdparty/stout/include/stout/os/windows/exec.hpp | 9 ---
3 files changed, 91 insertions(+), 22 deletions(-)
diff --git a/3rdparty/stout/include/stout/os/exec.hpp b/3rdparty/stout/include/stout/os/exec.hpp
index 4ad130b..686e7a1 100644
--- a/3rdparty/stout/include/stout/os/exec.hpp
+++ b/3rdparty/stout/include/stout/os/exec.hpp
@@ -21,4 +21,95 @@
#include <stout/os/posix/exec.hpp>
#endif // __WINDOWS__
+namespace os {
+
+// Forks a subprocess and executes the provided file with the provided
+// arguments.
+//
+// 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: 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.
+//
+// Windows: Note that on Windows, processes are started using
+// a string command line, and each process does its own parsing
+// of that command line into arguments. This function will quote
+// and escape the arguments compatible with any programs that
+// use `CommandLineToArgvW` (this is the most common way, and any
+// programs that use libc-style main with an arguments array will
+// use this under the covers). However, some programs, notably
+// cmd.exe have their own approach for parsing quotes / arguments
+// that are not compatible with CommandLineToArgvW, and therefore
+// should not be used with this function!
+//
+// TODO(bmahler): Add a windows only overload that takes a single
+// string command line (to support cmd.exe and others with non-
+// CommandLineToArgvW parsing). See MESOS-10093.
+inline Option<int> spawn(
+ const std::string& file,
+ const std::vector<std::string>& arguments);
+
+
+// This wrapper allows a caller to call `execvp` on both POSIX
+// and Windows systems.
+//
+// Windows: In order to emulate `execvp`, this function forks
+// another subprocess to execute with the provided arguments,
+// and once the subprocess terminates, the parent process will
+// in turn exit with the same exit code. Note that on Windows,
+// processes are started using a string command line, and each
+// process does its own parsing of that command line into
+// arguments. This function will quote and escape the arguments
+// compatible with any programs that use `CommandLineToArgvW`
+// (this is the most common way, and any programs that use
+// libc-style main with an arguments array will use this under
+// the covers). However, some programs, notably cmd.exe have
+// their own approach for parsing quotes / arguments that are
+// not compatible with CommandLineToArgvW, and therefore should
+// not be used with this function!
+//
+// TODO(bmahler): Probably we shouldn't provide this windows
+// emulation and should instead have the caller use windows
+// subprocess functions directly?
+inline int execvp(const char* file, char* const argv[]);
+
+
+// This function is a portable version of execvpe ('p' means searching
+// executable from PATH and 'e' means setting environments). We add
+// this function because it is not available on all POSIX systems.
+//
+// POSIX: This function is not thread safe. It is supposed to be used
+// only after fork (when there is only one thread). This function is
+// async signal safe.
+//
+// Windows: In order to emulate `execvpe`, this function forks
+// another subprocess to execute with the provided arguments,
+// and once the subprocess terminates, the parent process will
+// in turn exit with the same exit code. Note that on Windows,
+// processes are started using a string command line, and each
+// process does its own parsing of that command line into
+// arguments. This function will quote and escape the arguments
+// compatible with any programs that use `CommandLineToArgvW`
+// (this is the most common way, and any programs that use
+// libc-style main with an arguments array will use this under
+// the covers). However, some programs, notably cmd.exe have
+// their own approach for parsing quotes / arguments that are
+// not compatible with CommandLineToArgvW, and therefore should
+// not be used with this function!
+//
+// NOTE: This function can accept `Argv` and `Envp` constructs
+// via their implicit type conversions, but on Windows, it cannot
+// accept the os::raw forms.
+//
+// TODO(bmahler): Probably we shouldn't provide this windows
+// emulation and should instead have the caller use windows
+// subprocess functions directly?
+inline int execvpe(const char* file, char** argv, char** envp);
+
+} // namespace os {
+
#endif // __STOUT_OS_EXEC_HPP__
diff --git a/3rdparty/stout/include/stout/os/posix/exec.hpp b/3rdparty/stout/include/stout/os/posix/exec.hpp
index dd0f42d..3057b45 100644
--- a/3rdparty/stout/include/stout/os/posix/exec.hpp
+++ b/3rdparty/stout/include/stout/os/posix/exec.hpp
@@ -29,12 +29,6 @@
namespace os {
-// Executes a command by calling "<command> <arguments...>", 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.
inline Option<int> spawn(
const std::string& file,
const std::vector<std::string>& arguments)
@@ -67,13 +61,6 @@ inline int execvp(const char* file, char* const argv[])
}
-// This function is a portable version of execvpe ('p' means searching
-// executable from PATH and 'e' means setting environments). We add
-// this function because it is not available on all systems.
-//
-// NOTE: This function is not thread safe. It is supposed to be used
-// only after fork (when there is only one thread). This function is
-// async signal safe.
inline int execvpe(const char* file, char** argv, char** envp)
{
char** saved = os::raw::environment();
diff --git a/3rdparty/stout/include/stout/os/windows/exec.hpp b/3rdparty/stout/include/stout/os/windows/exec.hpp
index 8c6f10b..21e1d15 100644
--- a/3rdparty/stout/include/stout/os/windows/exec.hpp
+++ b/3rdparty/stout/include/stout/os/windows/exec.hpp
@@ -409,9 +409,6 @@ inline Try<ProcessData> create_process(
} // namespace windows {
-// Executes a command by calling "<command> <arguments...>", and
-// returns after the command has been completed. Returns the process exit
-// code on success and `None` on error.
inline Option<int> spawn(
const std::string& command,
const std::vector<std::string>& arguments,
@@ -442,9 +439,6 @@ inline Option<int> spawn(
}
-// In order to emulate the semantics of `execvp`, `os::spawn` waits for the new
-// process to exit, and returns its error code, which is propagated back to the
-// parent via `exit` here.
inline int execvp(
const std::string& file,
const std::vector<std::string>& argv)
@@ -454,9 +448,6 @@ inline int execvp(
}
-// NOTE: This function can accept `Argv` and `Envp` constructs through their
-// explicit type conversions, but unlike the POSIX implementations, it cannot
-// accept the raw forms.
inline int execvpe(
const std::string& file,
const std::vector<std::string>& argv,
[mesos] 03/03: Moved documentation into os/shell.hpp interface.
Posted by bm...@apache.org.
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 <bm...@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});