From: Adam Simpkins Date: Tue, 16 Apr 2013 00:58:47 +0000 (-0700) Subject: Update Subprocess to throw if exec() fails X-Git-Tag: v0.22.0~971 X-Git-Url: http://demsky.eecs.uci.edu/git/?a=commitdiff_plain;h=9cbe659479b0271da0376b0ec50a21723d484e25;p=folly.git 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 --- diff --git a/folly/ScopeGuard.h b/folly/ScopeGuard.h index f6f2e2c3..a3050a84 100644 --- a/folly/ScopeGuard.h +++ b/folly/ScopeGuard.h @@ -104,7 +104,7 @@ class ScopeGuardImpl : public ScopeGuardImplBase { } } -private: + private: void* operator new(size_t) = delete; void execute() noexcept { function_(); } diff --git a/folly/Subprocess.cpp b/folly/Subprocess.cpp index 86134025..11026c29 100644 --- a/folly/Subprocess.cpp +++ b/folly/Subprocess.cpp @@ -33,6 +33,7 @@ #include "folly/Conv.h" #include "folly/Exception.h" +#include "folly/FileUtil.h" #include "folly/ScopeGuard.h" #include "folly/String.h" #include "folly/io/Cursor.h" @@ -94,6 +95,16 @@ CalledProcessError::CalledProcessError(ProcessReturnCode rc) what_(returnCode_.str()) { } +SubprocessSpawnError::SubprocessSpawnError(const char* executable, + int errCode, + int errnoValue) + : errnoValue_(errnoValue), + what_(to(errCode == kExecFailure ? + "failed to execute " : + "error preparing to execute ", + executable, ": ", errnoStr(errnoValue))) { +} + namespace { // Copy pointers to the given strings in a format suitable for posix_spawn @@ -170,12 +181,29 @@ Subprocess::Subprocess( Subprocess::~Subprocess() { CHECK_NE(returnCode_.state(), ProcessReturnCode::RUNNING) << "Subprocess destroyed without reaping child"; + closeAll(); } namespace { void closeChecked(int fd) { 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 void Subprocess::closeAll() { @@ -208,24 +236,79 @@ void Subprocess::spawn( // Make a copy, we'll mutate options 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 argv, + const char* executable, + Options& options, + const std::vector* env, + int errFd) { // Parent work, pre-fork: create pipes std::vector childFds; - - // If we throw, don't leak file descriptors - auto guard = makeGuard([&] { + // Close all of the childFds as we leave this scope + SCOPE_EXIT { // These are only pipes, closing them shouldn't fail for (int cfd : childFds) { CHECK_ERR(::close(cfd)); } - for (auto& p : this->pipes_) { - CHECK_ERR(::close(p.parentFd)); - } - }); + }; + int r; for (auto& p : options.fdActions_) { if (p.second == PIPE_IN || p.second == PIPE_OUT) { int fds[2]; - int r = ::pipe(fds); + r = ::pipe(fds); checkUnixError(r, "pipe"); PipeInfo pinfo; pinfo.direction = p.second; @@ -277,72 +360,62 @@ void Subprocess::spawn( // // The parent also unblocks all signals as soon as vfork() returns. sigset_t allBlocked; - int r = ::sigfillset(&allBlocked); + r = ::sigfillset(&allBlocked); checkUnixError(r, "sigfillset"); sigset_t oldSignals; + r = pthread_sigmask(SIG_SETMASK, &allBlocked, &oldSignals); 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(); if (pid == 0) { - // While all signals are blocked, we must reset their - // dispositions to default. - for (int sig = 1; sig < NSIG; ++sig) { - ::signal(sig, SIG_DFL); + int errnoValue = prepareChild(options, &oldSignals); + if (errnoValue != 0) { + childError(errFd, kChildFailure, errnoValue); } - // 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 - // shouldn't fail. - for (int f : childFds) { - CHECK_ERR(::close(f)); + errnoValue = runChild(executable, argVec, envVec, options); + // If we get here, exec() failed. + childError(errFd, kExecFailure, errnoValue); } -} + // In parent. Make sure vfork() succeeded. + checkUnixError(pid, errno, "vfork"); -namespace { - -// Checked version of close() to use in the child: _exit(126) on error -void childClose(int fd) { - int r = ::close(fd); - if (r == -1) _exit(kChildFailure); -} - -// Checked version of dup2() to use in the child: _exit(126) on error -void childDup2(int oldfd, int newfd) { - int r = ::dup2(oldfd, newfd); - if (r == -1) _exit(kChildFailure); + // 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. + pid_ = pid; + returnCode_ = ProcessReturnCode(RV_RUNNING); } -} // 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 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. @@ -352,7 +425,10 @@ void Subprocess::runChild(const char* executable, if (p.second == CLOSE) { ::close(p.first); } 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, // Opt to receive signal on parent death, if requested 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) { - _exit(kChildFailure); + return errno; } } + return 0; +} + +int Subprocess::runChild(const char* executable, + char** argv, char** env, + const Options& options) const { // Now, finally, exec. int r; if (options.usePath_) { @@ -382,6 +464,36 @@ void Subprocess::runChild(const char* executable, } else { ::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() { diff --git a/folly/Subprocess.h b/folly/Subprocess.h index 5c53a319..58c9c9c8 100644 --- a/folly/Subprocess.h +++ b/folly/Subprocess.h @@ -144,10 +144,15 @@ class ProcessReturnCode { int rawStatus_; }; +/** + * Base exception thrown by the Subprocess methods. + */ +class SubprocessError : public std::exception {}; + /** * Exception thrown by *Checked methods of Subprocess. */ -class CalledProcessError : public std::exception { +class CalledProcessError : public SubprocessError { public: explicit CalledProcessError(ProcessReturnCode rc); ~CalledProcessError() throw() { } @@ -158,6 +163,21 @@ class CalledProcessError : public std::exception { 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. */ @@ -457,16 +477,34 @@ class Subprocess : private boost::noncopyable { static const int RV_RUNNING = ProcessReturnCode::RV_RUNNING; 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( std::unique_ptr argv, const char* executable, const Options& options, const std::vector* env); + void spawnInternal( + std::unique_ptr argv, + const char* executable, + Options& options, + const std::vector* env, + int errFd); - // Action to run in child. + // Actions to run in child. // Note that this runs after vfork(), so tread lightly. - void runChild(const char* executable, char** argv, char** env, - const Options& options) const; + // Returns 0 on success, or an errno value on failure. + 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. diff --git a/folly/test/SubprocessTest.cpp b/folly/test/SubprocessTest.cpp index 17c145d1..112e9aae 100644 --- a/folly/test/SubprocessTest.cpp +++ b/folly/test/SubprocessTest.cpp @@ -17,10 +17,14 @@ #include "folly/Subprocess.h" #include +#include +#include +#include #include #include +#include "folly/Exception.h" #include "folly/Format.h" #include "folly/experimental/Gen.h" #include "folly/experimental/FileGen.h" @@ -49,9 +53,27 @@ TEST(SimpleSubprocessTest, ExitsWithErrorChecked) { EXPECT_THROW(proc.waitChecked(), CalledProcessError); } +#define EXPECT_SPAWN_ERROR(err, errMsg, cmd, ...) \ + do { \ + try { \ + Subprocess proc(std::vector{ (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) { - Subprocess proc(std::vector{ "/no/such/file" }); - EXPECT_EQ(127, proc.wait().exitStatus()); + EXPECT_SPAWN_ERROR(ENOENT, "failed to execute /no/such/file:", + "/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) { @@ -64,6 +86,69 @@ TEST(SimpleSubprocessTest, ShellExitsWithError) { EXPECT_EQ(1, proc.wait().exitStatus()); } +namespace { +boost::container::flat_set getOpenFds() { + auto pid = getpid(); + auto dirname = to("/proc/", pid, "/fd"); + + boost::container::flat_set fds; + for (fs::directory_iterator it(dirname); + it != fs::directory_iterator(); + ++it) { + int fd = to(it->path().filename().native()); + fds.insert(fd); + } + return fds; +} + +template +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({"/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) { // Find out where we are. static constexpr size_t pathLength = 2048; @@ -144,4 +229,3 @@ TEST(CommunicateSubprocessTest, Duplex) { EXPECT_EQ(std::string::npos, p.first.find_first_not_of('X')); proc.waitChecked(); } -