Commit e33bebd3 authored by Tudor Bosman's avatar Tudor Bosman Committed by Jordan DeLong

Make Subprocess::spawn more robust

Summary:
We can't throw after the process is created, because we don't know what to do
with it (and the Subprocess object goes up in smoke, so we can't rely on the
caller to clean up, either).  So don't throw.

If we throw before the process is created, make sure we clean up.

Test Plan: subprocess_test

Reviewed By: delong.j@fb.com

FB internal diff: D774722
parent 570ec333
...@@ -39,6 +39,9 @@ ...@@ -39,6 +39,9 @@
extern char** environ; extern char** environ;
constexpr int kExecFailure = 127;
constexpr int kChildFailure = 126;
namespace folly { namespace folly {
ProcessReturnCode::State ProcessReturnCode::state() const { ProcessReturnCode::State ProcessReturnCode::state() const {
...@@ -207,6 +210,18 @@ void Subprocess::spawn( ...@@ -207,6 +210,18 @@ void Subprocess::spawn(
// Parent work, pre-fork: create pipes // Parent work, pre-fork: create pipes
std::vector<int> childFds; std::vector<int> childFds;
// If we throw, don't leak file descriptors
auto guard = makeGuard([&] {
// 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));
}
});
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];
...@@ -277,11 +292,11 @@ void Subprocess::spawn( ...@@ -277,11 +292,11 @@ void Subprocess::spawn(
} }
// Unblock signals; restore signal mask. // Unblock signals; restore signal mask.
int r = pthread_sigmask(SIG_SETMASK, &oldSignals, nullptr); int r = pthread_sigmask(SIG_SETMASK, &oldSignals, nullptr);
if (r != 0) abort(); if (r != 0) _exit(kChildFailure);
runChild(executable, argVec, envVec, options); runChild(executable, argVec, envVec, options);
// This should never return, but there's nothing else we can do here. // This should never return, but there's nothing else we can do here.
abort(); _exit(kExecFailure);
} }
// In parent. We want to restore the signal mask even if vfork fails, // 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 // so we'll save errno here, restore the signal mask, and only then
...@@ -289,35 +304,35 @@ void Subprocess::spawn( ...@@ -289,35 +304,35 @@ void Subprocess::spawn(
int savedErrno = errno; int savedErrno = errno;
// Restore signal mask; do this even if vfork fails! // Restore signal mask; do this even if vfork fails!
// We only check for errors from pthread_sigmask after we recorded state
// that the child is alive, so we know to reap it.
r = pthread_sigmask(SIG_SETMASK, &oldSignals, nullptr); r = pthread_sigmask(SIG_SETMASK, &oldSignals, nullptr);
CHECK_EQ(r, 0) << "pthread_sigmask: " << errnoStr(r); // shouldn't fail
checkUnixError(pid, savedErrno, "vfork"); checkUnixError(pid, savedErrno, "vfork");
// Child is alive // 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; pid_ = pid;
returnCode_ = ProcessReturnCode(RV_RUNNING); returnCode_ = ProcessReturnCode(RV_RUNNING);
// Parent work, post-fork: close child's ends of pipes // Parent work, post-fork: close child's ends of pipes; closing them
// shouldn't fail.
for (int f : childFds) { for (int f : childFds) {
closeChecked(f); CHECK_ERR(::close(f));
} }
checkPosixError(r, "pthread_sigmask");
} }
namespace { namespace {
// Checked version of close() to use in the child: abort() on error // Checked version of close() to use in the child: _exit(126) on error
void childClose(int fd) { void childClose(int fd) {
int r = ::close(fd); int r = ::close(fd);
if (r == -1) abort(); if (r == -1) _exit(kChildFailure);
} }
// Checked version of dup2() to use in the child: abort() on error // Checked version of dup2() to use in the child: _exit(126) on error
void childDup2(int oldfd, int newfd) { void childDup2(int oldfd, int newfd) {
int r = ::dup2(oldfd, newfd); int r = ::dup2(oldfd, newfd);
if (r == -1) abort(); if (r == -1) _exit(kChildFailure);
} }
} // namespace } // namespace
...@@ -356,7 +371,7 @@ void Subprocess::runChild(const char* executable, ...@@ -356,7 +371,7 @@ void Subprocess::runChild(const char* executable,
if (options.parentDeathSignal_ != 0) { if (options.parentDeathSignal_ != 0) {
int r = prctl(PR_SET_PDEATHSIG, options.parentDeathSignal_, 0, 0, 0); int r = prctl(PR_SET_PDEATHSIG, options.parentDeathSignal_, 0, 0, 0);
if (r == -1) { if (r == -1) {
abort(); _exit(kChildFailure);
} }
} }
...@@ -367,9 +382,6 @@ void Subprocess::runChild(const char* executable, ...@@ -367,9 +382,6 @@ void Subprocess::runChild(const char* executable,
} else { } else {
::execve(executable, argv, env); ::execve(executable, argv, env);
} }
// If we're here, something's wrong.
abort();
} }
ProcessReturnCode Subprocess::poll() { ProcessReturnCode Subprocess::poll() {
......
...@@ -49,6 +49,11 @@ TEST(SimpleSubprocessTest, ExitsWithErrorChecked) { ...@@ -49,6 +49,11 @@ TEST(SimpleSubprocessTest, ExitsWithErrorChecked) {
EXPECT_THROW(proc.waitChecked(), CalledProcessError); EXPECT_THROW(proc.waitChecked(), CalledProcessError);
} }
TEST(SimpleSubprocessTest, ExecFails) {
Subprocess proc(std::vector<std::string>{ "/no/such/file" });
EXPECT_EQ(127, proc.wait().exitStatus());
}
TEST(SimpleSubprocessTest, ShellExitsSuccesssfully) { TEST(SimpleSubprocessTest, ShellExitsSuccesssfully) {
Subprocess proc("true"); Subprocess proc("true");
EXPECT_EQ(0, proc.wait().exitStatus()); EXPECT_EQ(0, proc.wait().exitStatus());
...@@ -64,7 +69,7 @@ TEST(ParentDeathSubprocessTest, ParentDeathSignal) { ...@@ -64,7 +69,7 @@ TEST(ParentDeathSubprocessTest, ParentDeathSignal) {
static constexpr size_t pathLength = 2048; static constexpr size_t pathLength = 2048;
char buf[pathLength]; char buf[pathLength];
int r = readlink("/proc/self/exe", buf, pathLength); int r = readlink("/proc/self/exe", buf, pathLength);
CHECK_ERR(r >= 0); CHECK_ERR(r);
buf[r] = '\0'; buf[r] = '\0';
fs::path helper(buf); fs::path helper(buf);
......
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