Commit 9cbe6594 authored by Adam Simpkins's avatar Adam Simpkins Committed by Sara Golemon

Update Subprocess to throw if exec() fails

Summary:
Add a new SubprocessSpawnError, and change the Subprocess constructor to
throw this if the child process encounters an error before calling
execve().  Error information is passed back to the parent process over a
pipe.

Previosly in this case the Subprocess constructor would fail, and
clients would simply get a return code of 126 or 127 when waiting on the
process.  There was no way to distinguish this from a successful
execve() followed by the child process exiting with status 127.

Test Plan:
Added tests to check the exception behavior, and also to check for file
descriptor leaks in the parent process.

Reviewed By: tudorb@fb.com

FB internal diff: D776273
parent 94ddd29d
...@@ -104,7 +104,7 @@ class ScopeGuardImpl : public ScopeGuardImplBase { ...@@ -104,7 +104,7 @@ class ScopeGuardImpl : public ScopeGuardImplBase {
} }
} }
private: private:
void* operator new(size_t) = delete; void* operator new(size_t) = delete;
void execute() noexcept { function_(); } void execute() noexcept { function_(); }
......
...@@ -33,6 +33,7 @@ ...@@ -33,6 +33,7 @@
#include "folly/Conv.h" #include "folly/Conv.h"
#include "folly/Exception.h" #include "folly/Exception.h"
#include "folly/FileUtil.h"
#include "folly/ScopeGuard.h" #include "folly/ScopeGuard.h"
#include "folly/String.h" #include "folly/String.h"
#include "folly/io/Cursor.h" #include "folly/io/Cursor.h"
...@@ -94,6 +95,16 @@ CalledProcessError::CalledProcessError(ProcessReturnCode rc) ...@@ -94,6 +95,16 @@ CalledProcessError::CalledProcessError(ProcessReturnCode rc)
what_(returnCode_.str()) { what_(returnCode_.str()) {
} }
SubprocessSpawnError::SubprocessSpawnError(const char* executable,
int errCode,
int errnoValue)
: errnoValue_(errnoValue),
what_(to<std::string>(errCode == kExecFailure ?
"failed to execute " :
"error preparing to execute ",
executable, ": ", errnoStr(errnoValue))) {
}
namespace { namespace {
// Copy pointers to the given strings in a format suitable for posix_spawn // Copy pointers to the given strings in a format suitable for posix_spawn
...@@ -170,12 +181,29 @@ Subprocess::Subprocess( ...@@ -170,12 +181,29 @@ Subprocess::Subprocess(
Subprocess::~Subprocess() { Subprocess::~Subprocess() {
CHECK_NE(returnCode_.state(), ProcessReturnCode::RUNNING) CHECK_NE(returnCode_.state(), ProcessReturnCode::RUNNING)
<< "Subprocess destroyed without reaping child"; << "Subprocess destroyed without reaping child";
closeAll();
} }
namespace { namespace {
void closeChecked(int fd) { void closeChecked(int fd) {
checkUnixError(::close(fd), "close"); checkUnixError(::close(fd), "close");
} }
struct ChildErrorInfo {
int errCode;
int errnoValue;
};
void childError(int errFd, int errCode, int errnoValue) FOLLY_NORETURN;
void childError(int errFd, int errCode, int errnoValue) {
ChildErrorInfo info = {errCode, errnoValue};
// Write the error information over the pipe to our parent process.
// We can't really do anything else if this write call fails.
writeNoInt(errFd, &info, sizeof(info));
// exit
_exit(errCode);
}
} // namespace } // namespace
void Subprocess::closeAll() { void Subprocess::closeAll() {
...@@ -208,24 +236,79 @@ void Subprocess::spawn( ...@@ -208,24 +236,79 @@ void Subprocess::spawn(
// Make a copy, we'll mutate options // Make a copy, we'll mutate options
Options options(optionsIn); Options options(optionsIn);
// On error, close all of the pipes_
auto pipesGuard = makeGuard([&] {
for (auto& p : this->pipes_) {
CHECK_ERR(::close(p.parentFd));
}
});
// Create a pipe to use to receive error information from the child,
// in case it fails before calling exec()
int errFds[2];
int r = ::pipe(errFds);
checkUnixError(r, "pipe");
SCOPE_EXIT {
CHECK_ERR(::close(errFds[0]));
if (errFds[1] >= 0) {
CHECK_ERR(::close(errFds[1]));
}
};
// Ask the child to close the read end of the error pipe.
options.fdActions_[errFds[0]] = CLOSE;
// Set the close-on-exec flag on the write side of the pipe.
// This way the pipe will be closed automatically in the child if execve()
// succeeds. If the exec fails the child can write error information to the
// pipe.
r = fcntl(errFds[1], F_SETFD, FD_CLOEXEC);
checkUnixError(r, "set FD_CLOEXEC");
// Perform the actual work of setting up pipes then forking and
// executing the child.
spawnInternal(std::move(argv), executable, options, env, errFds[1]);
// After spawnInternal() returns the child is alive. We have to be very
// careful about throwing after this point. We are inside the constructor,
// so if we throw the Subprocess object will have never existed, and the
// destructor will never be called.
//
// We should only throw if we got an error via the errFd, and we know the
// child has exited and can be immediately waited for. In all other cases,
// we have no way of cleaning up the child.
// Close writable side of the errFd pipe in the parent process
CHECK_ERR(::close(errFds[1]));
errFds[1] = -1;
// Read from the errFd pipe, to tell if the child ran into any errors before
// calling exec()
readChildErrorPipe(errFds[0], executable);
// We have fully succeeded now, so release the guard on pipes_
pipesGuard.dismiss();
}
void Subprocess::spawnInternal(
std::unique_ptr<const char*[]> argv,
const char* executable,
Options& options,
const std::vector<std::string>* env,
int errFd) {
// Parent work, pre-fork: create pipes // Parent work, pre-fork: create pipes
std::vector<int> childFds; std::vector<int> childFds;
// Close all of the childFds as we leave this scope
// If we throw, don't leak file descriptors SCOPE_EXIT {
auto guard = makeGuard([&] {
// These are only pipes, closing them shouldn't fail // These are only pipes, closing them shouldn't fail
for (int cfd : childFds) { for (int cfd : childFds) {
CHECK_ERR(::close(cfd)); CHECK_ERR(::close(cfd));
} }
for (auto& p : this->pipes_) { };
CHECK_ERR(::close(p.parentFd));
}
});
int r;
for (auto& p : options.fdActions_) { for (auto& p : options.fdActions_) {
if (p.second == PIPE_IN || p.second == PIPE_OUT) { if (p.second == PIPE_IN || p.second == PIPE_OUT) {
int fds[2]; int fds[2];
int r = ::pipe(fds); r = ::pipe(fds);
checkUnixError(r, "pipe"); checkUnixError(r, "pipe");
PipeInfo pinfo; PipeInfo pinfo;
pinfo.direction = p.second; pinfo.direction = p.second;
...@@ -277,72 +360,62 @@ void Subprocess::spawn( ...@@ -277,72 +360,62 @@ void Subprocess::spawn(
// //
// The parent also unblocks all signals as soon as vfork() returns. // The parent also unblocks all signals as soon as vfork() returns.
sigset_t allBlocked; sigset_t allBlocked;
int r = ::sigfillset(&allBlocked); r = ::sigfillset(&allBlocked);
checkUnixError(r, "sigfillset"); checkUnixError(r, "sigfillset");
sigset_t oldSignals; sigset_t oldSignals;
r = pthread_sigmask(SIG_SETMASK, &allBlocked, &oldSignals); r = pthread_sigmask(SIG_SETMASK, &allBlocked, &oldSignals);
checkPosixError(r, "pthread_sigmask"); checkPosixError(r, "pthread_sigmask");
SCOPE_EXIT {
// Restore signal mask
r = pthread_sigmask(SIG_SETMASK, &oldSignals, nullptr);
CHECK_EQ(r, 0) << "pthread_sigmask: " << errnoStr(r); // shouldn't fail
};
pid_t pid = vfork(); pid_t pid = vfork();
if (pid == 0) { if (pid == 0) {
// While all signals are blocked, we must reset their int errnoValue = prepareChild(options, &oldSignals);
// dispositions to default. if (errnoValue != 0) {
for (int sig = 1; sig < NSIG; ++sig) { childError(errFd, kChildFailure, errnoValue);
::signal(sig, SIG_DFL);
} }
// Unblock signals; restore signal mask.
int r = pthread_sigmask(SIG_SETMASK, &oldSignals, nullptr);
if (r != 0) _exit(kChildFailure);
runChild(executable, argVec, envVec, options);
// This should never return, but there's nothing else we can do here.
_exit(kExecFailure);
}
// In parent. We want to restore the signal mask even if vfork fails,
// so we'll save errno here, restore the signal mask, and only then
// throw.
int savedErrno = errno;
// Restore signal mask; do this even if vfork fails!
r = pthread_sigmask(SIG_SETMASK, &oldSignals, nullptr);
CHECK_EQ(r, 0) << "pthread_sigmask: " << errnoStr(r); // shouldn't fail
checkUnixError(pid, savedErrno, "vfork");
// Child is alive. We can't throw any more, as we can't figure out
// what to do with the child.
guard.dismiss();
pid_ = pid;
returnCode_ = ProcessReturnCode(RV_RUNNING);
// Parent work, post-fork: close child's ends of pipes; closing them errnoValue = runChild(executable, argVec, envVec, options);
// shouldn't fail. // If we get here, exec() failed.
for (int f : childFds) { childError(errFd, kExecFailure, errnoValue);
CHECK_ERR(::close(f));
} }
} // In parent. Make sure vfork() succeeded.
checkUnixError(pid, errno, "vfork");
namespace { // Child is alive. We have to be very careful about throwing after this
// point. We are inside the constructor, so if we throw the Subprocess
// Checked version of close() to use in the child: _exit(126) on error // object will have never existed, and the destructor will never be called.
void childClose(int fd) { //
int r = ::close(fd); // We should only throw if we got an error via the errFd, and we know the
if (r == -1) _exit(kChildFailure); // child has exited and can be immediately waited for. In all other cases,
} // we have no way of cleaning up the child.
pid_ = pid;
// Checked version of dup2() to use in the child: _exit(126) on error returnCode_ = ProcessReturnCode(RV_RUNNING);
void childDup2(int oldfd, int newfd) {
int r = ::dup2(oldfd, newfd);
if (r == -1) _exit(kChildFailure);
} }
} // namespace int Subprocess::prepareChild(const Options& options,
const sigset_t* sigmask) const {
// While all signals are blocked, we must reset their
// dispositions to default.
for (int sig = 1; sig < NSIG; ++sig) {
::signal(sig, SIG_DFL);
}
// Unblock signals; restore signal mask.
int r = pthread_sigmask(SIG_SETMASK, sigmask, nullptr);
if (r != 0) {
return r; // pthread_sigmask() returns an errno value
}
void Subprocess::runChild(const char* executable,
char** argv, char** env,
const Options& options) const {
// Close parent's ends of all pipes // Close parent's ends of all pipes
for (auto& p : pipes_) { for (auto& p : pipes_) {
childClose(p.parentFd); r = ::close(p.parentFd);
if (r == -1) {
return errno;
}
} }
// Close all fds that we're supposed to close. // Close all fds that we're supposed to close.
...@@ -352,7 +425,10 @@ void Subprocess::runChild(const char* executable, ...@@ -352,7 +425,10 @@ void Subprocess::runChild(const char* executable,
if (p.second == CLOSE) { if (p.second == CLOSE) {
::close(p.first); ::close(p.first);
} else { } else {
childDup2(p.second, p.first); r = ::dup2(p.second, p.first);
if (r == -1) {
return errno;
}
} }
} }
...@@ -369,12 +445,18 @@ void Subprocess::runChild(const char* executable, ...@@ -369,12 +445,18 @@ void Subprocess::runChild(const char* executable,
// Opt to receive signal on parent death, if requested // Opt to receive signal on parent death, if requested
if (options.parentDeathSignal_ != 0) { if (options.parentDeathSignal_ != 0) {
int r = prctl(PR_SET_PDEATHSIG, options.parentDeathSignal_, 0, 0, 0); r = prctl(PR_SET_PDEATHSIG, options.parentDeathSignal_, 0, 0, 0);
if (r == -1) { if (r == -1) {
_exit(kChildFailure); return errno;
} }
} }
return 0;
}
int Subprocess::runChild(const char* executable,
char** argv, char** env,
const Options& options) const {
// Now, finally, exec. // Now, finally, exec.
int r; int r;
if (options.usePath_) { if (options.usePath_) {
...@@ -382,6 +464,36 @@ void Subprocess::runChild(const char* executable, ...@@ -382,6 +464,36 @@ void Subprocess::runChild(const char* executable,
} else { } else {
::execve(executable, argv, env); ::execve(executable, argv, env);
} }
return errno;
}
void Subprocess::readChildErrorPipe(int pfd, const char* executable) {
ChildErrorInfo info;
auto rc = readNoInt(pfd, &info, sizeof(info));
if (rc == 0) {
// No data means the child executed successfully, and the pipe
// was closed due to the close-on-exec flag being set.
return;
} else if (rc != sizeof(ChildErrorInfo)) {
// An error occurred trying to read from the pipe, or we got a partial read.
// Neither of these cases should really occur in practice.
//
// We can't get any error data from the child in this case, and we don't
// know if it is successfully running or not. All we can do is to return
// normally, as if the child executed successfully. If something bad
// happened the caller should at least get a non-normal exit status from
// the child.
LOG(ERROR) << "unexpected error trying to read from child error pipe " <<
"rc=" << rc << ", errno=" << errno;
return;
}
// We got error data from the child. The child should exit immediately in
// this case, so wait on it to clean up.
wait();
// Throw to signal the error
throw SubprocessSpawnError(executable, info.errCode, info.errnoValue);
} }
ProcessReturnCode Subprocess::poll() { ProcessReturnCode Subprocess::poll() {
......
...@@ -144,10 +144,15 @@ class ProcessReturnCode { ...@@ -144,10 +144,15 @@ class ProcessReturnCode {
int rawStatus_; int rawStatus_;
}; };
/**
* Base exception thrown by the Subprocess methods.
*/
class SubprocessError : public std::exception {};
/** /**
* Exception thrown by *Checked methods of Subprocess. * Exception thrown by *Checked methods of Subprocess.
*/ */
class CalledProcessError : public std::exception { class CalledProcessError : public SubprocessError {
public: public:
explicit CalledProcessError(ProcessReturnCode rc); explicit CalledProcessError(ProcessReturnCode rc);
~CalledProcessError() throw() { } ~CalledProcessError() throw() { }
...@@ -158,6 +163,21 @@ class CalledProcessError : public std::exception { ...@@ -158,6 +163,21 @@ class CalledProcessError : public std::exception {
std::string what_; std::string what_;
}; };
/**
* Exception thrown if the subprocess cannot be started.
*/
class SubprocessSpawnError : public SubprocessError {
public:
SubprocessSpawnError(const char* executable, int errCode, int errnoValue);
~SubprocessSpawnError() throw() {}
const char* what() const throw() FOLLY_OVERRIDE { return what_.c_str(); }
int errnoValue() const { return errnoValue_; }
private:
int errnoValue_;
std::string what_;
};
/** /**
* Subprocess. * Subprocess.
*/ */
...@@ -457,16 +477,34 @@ class Subprocess : private boost::noncopyable { ...@@ -457,16 +477,34 @@ class Subprocess : private boost::noncopyable {
static const int RV_RUNNING = ProcessReturnCode::RV_RUNNING; static const int RV_RUNNING = ProcessReturnCode::RV_RUNNING;
static const int RV_NOT_STARTED = ProcessReturnCode::RV_NOT_STARTED; static const int RV_NOT_STARTED = ProcessReturnCode::RV_NOT_STARTED;
// spawn() sets up a pipe to read errors from the child,
// then calls spawnInternal() to do the bulk of the work. Once
// spawnInternal() returns it reads the error pipe to see if the child
// encountered any errors.
void spawn( void spawn(
std::unique_ptr<const char*[]> argv, std::unique_ptr<const char*[]> argv,
const char* executable, const char* executable,
const Options& options, const Options& options,
const std::vector<std::string>* env); const std::vector<std::string>* env);
void spawnInternal(
std::unique_ptr<const char*[]> argv,
const char* executable,
Options& options,
const std::vector<std::string>* env,
int errFd);
// Action to run in child. // Actions to run in child.
// Note that this runs after vfork(), so tread lightly. // Note that this runs after vfork(), so tread lightly.
void runChild(const char* executable, char** argv, char** env, // Returns 0 on success, or an errno value on failure.
const Options& options) const; int prepareChild(const Options& options, const sigset_t* sigmask) const;
int runChild(const char* executable, char** argv, char** env,
const Options& options) const;
/**
* Read from the error pipe, and throw SubprocessSpawnError if the child
* failed before calling exec().
*/
void readChildErrorPipe(int pfd, const char* executable);
/** /**
* Close all file descriptors. * Close all file descriptors.
......
...@@ -17,10 +17,14 @@ ...@@ -17,10 +17,14 @@
#include "folly/Subprocess.h" #include "folly/Subprocess.h"
#include <unistd.h> #include <unistd.h>
#include <sys/types.h>
#include <dirent.h>
#include <boost/container/flat_set.hpp>
#include <glog/logging.h> #include <glog/logging.h>
#include <gtest/gtest.h> #include <gtest/gtest.h>
#include "folly/Exception.h"
#include "folly/Format.h" #include "folly/Format.h"
#include "folly/experimental/Gen.h" #include "folly/experimental/Gen.h"
#include "folly/experimental/FileGen.h" #include "folly/experimental/FileGen.h"
...@@ -49,9 +53,27 @@ TEST(SimpleSubprocessTest, ExitsWithErrorChecked) { ...@@ -49,9 +53,27 @@ TEST(SimpleSubprocessTest, ExitsWithErrorChecked) {
EXPECT_THROW(proc.waitChecked(), CalledProcessError); EXPECT_THROW(proc.waitChecked(), CalledProcessError);
} }
#define EXPECT_SPAWN_ERROR(err, errMsg, cmd, ...) \
do { \
try { \
Subprocess proc(std::vector<std::string>{ (cmd), ## __VA_ARGS__ }); \
ADD_FAILURE() << "expected an error when running " << (cmd); \
} catch (const SubprocessSpawnError& ex) { \
EXPECT_EQ((err), ex.errnoValue()); \
if (StringPiece(ex.what()).find(errMsg) == StringPiece::npos) { \
ADD_FAILURE() << "failed to find \"" << (errMsg) << \
"\" in exception: \"" << ex.what() << "\""; \
} \
} \
} while (0)
TEST(SimpleSubprocessTest, ExecFails) { TEST(SimpleSubprocessTest, ExecFails) {
Subprocess proc(std::vector<std::string>{ "/no/such/file" }); EXPECT_SPAWN_ERROR(ENOENT, "failed to execute /no/such/file:",
EXPECT_EQ(127, proc.wait().exitStatus()); "/no/such/file");
EXPECT_SPAWN_ERROR(EACCES, "failed to execute /etc/passwd:",
"/etc/passwd");
EXPECT_SPAWN_ERROR(ENOTDIR, "failed to execute /etc/passwd/not/a/file:",
"/etc/passwd/not/a/file");
} }
TEST(SimpleSubprocessTest, ShellExitsSuccesssfully) { TEST(SimpleSubprocessTest, ShellExitsSuccesssfully) {
...@@ -64,6 +86,69 @@ TEST(SimpleSubprocessTest, ShellExitsWithError) { ...@@ -64,6 +86,69 @@ TEST(SimpleSubprocessTest, ShellExitsWithError) {
EXPECT_EQ(1, proc.wait().exitStatus()); EXPECT_EQ(1, proc.wait().exitStatus());
} }
namespace {
boost::container::flat_set<int> getOpenFds() {
auto pid = getpid();
auto dirname = to<std::string>("/proc/", pid, "/fd");
boost::container::flat_set<int> fds;
for (fs::directory_iterator it(dirname);
it != fs::directory_iterator();
++it) {
int fd = to<int>(it->path().filename().native());
fds.insert(fd);
}
return fds;
}
template<class Runnable>
void checkFdLeak(const Runnable& r) {
// Get the currently open fds. Check that they are the same both before and
// after calling the specified function. We read the open fds from /proc.
// (If we wanted to work even on systems that don't have /proc, we could
// perhaps create and immediately close a socket both before and after
// running the function, and make sure we got the same fd number both times.)
auto fdsBefore = getOpenFds();
r();
auto fdsAfter = getOpenFds();
EXPECT_EQ(fdsAfter.size(), fdsBefore.size());
}
}
// Make sure Subprocess doesn't leak any file descriptors
TEST(SimpleSubprocessTest, FdLeakTest) {
// Normal execution
checkFdLeak([] {
Subprocess proc("true");
EXPECT_EQ(0, proc.wait().exitStatus());
});
// Normal execution with pipes
checkFdLeak([] {
Subprocess proc("echo foo; echo bar >&2",
Subprocess::pipeStdout() | Subprocess::pipeStderr());
auto p = proc.communicate(Subprocess::readStdout() |
Subprocess::readStderr());
EXPECT_EQ("foo\n", p.first);
EXPECT_EQ("bar\n", p.second);
proc.waitChecked();
});
// Test where the exec call fails()
checkFdLeak([] {
EXPECT_SPAWN_ERROR(ENOENT, "failed to execute", "/no/such/file");
});
// Test where the exec call fails() with pipes
checkFdLeak([] {
try {
Subprocess proc(std::vector<std::string>({"/no/such/file"}),
Subprocess::pipeStdout().stderr(Subprocess::PIPE));
ADD_FAILURE() << "expected an error when running /no/such/file";
} catch (const SubprocessSpawnError& ex) {
EXPECT_EQ(ENOENT, ex.errnoValue());
}
});
}
TEST(ParentDeathSubprocessTest, ParentDeathSignal) { TEST(ParentDeathSubprocessTest, ParentDeathSignal) {
// Find out where we are. // Find out where we are.
static constexpr size_t pathLength = 2048; static constexpr size_t pathLength = 2048;
...@@ -144,4 +229,3 @@ TEST(CommunicateSubprocessTest, Duplex) { ...@@ -144,4 +229,3 @@ TEST(CommunicateSubprocessTest, Duplex) {
EXPECT_EQ(std::string::npos, p.first.find_first_not_of('X')); EXPECT_EQ(std::string::npos, p.first.find_first_not_of('X'));
proc.waitChecked(); proc.waitChecked();
} }
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment