From e33bebd37a24287368fdebc0d085d7319083ba08 Mon Sep 17 00:00:00 2001
From: Tudor Bosman <tudorb@fb.com>
Date: Fri, 12 Apr 2013 23:49:32 -0700
Subject: [PATCH] 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
---
folly/Subprocess.cpp | 46 ++++++++++++++++++++++-------------
folly/test/SubprocessTest.cpp | 7 +++++-
2 files changed, 35 insertions(+), 18 deletions(-)
diff --git a/folly/Subprocess.cpp b/folly/Subprocess.cpp
index 8421bd9e..86134025 100644
--- a/folly/Subprocess.cpp
+++ b/folly/Subprocess.cpp
@@ -39,6 +39,9 @@
extern char** environ;
+constexpr int kExecFailure = 127;
+constexpr int kChildFailure = 126;
+
namespace folly {
ProcessReturnCode::State ProcessReturnCode::state() const {
@@ -207,6 +210,18 @@ void Subprocess::spawn(
// Parent work, pre-fork: create pipes
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_) {
if (p.second == PIPE_IN || p.second == PIPE_OUT) {
int fds[2];
@@ -277,11 +292,11 @@ void Subprocess::spawn(
}
// Unblock signals; restore signal mask.
int r = pthread_sigmask(SIG_SETMASK, &oldSignals, nullptr);
- if (r != 0) abort();
+ if (r != 0) _exit(kChildFailure);
runChild(executable, argVec, envVec, options);
// 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,
// so we'll save errno here, restore the signal mask, and only then
@@ -289,35 +304,35 @@ void Subprocess::spawn(
int savedErrno = errno;
// 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);
+ CHECK_EQ(r, 0) << "pthread_sigmask: " << errnoStr(r); // shouldn't fail
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;
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) {
- closeChecked(f);
+ CHECK_ERR(::close(f));
}
-
- checkPosixError(r, "pthread_sigmask");
}
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) {
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) {
int r = ::dup2(oldfd, newfd);
- if (r == -1) abort();
+ if (r == -1) _exit(kChildFailure);
}
} // namespace
@@ -356,7 +371,7 @@ void Subprocess::runChild(const char* executable,
if (options.parentDeathSignal_ != 0) {
int r = prctl(PR_SET_PDEATHSIG, options.parentDeathSignal_, 0, 0, 0);
if (r == -1) {
- abort();
+ _exit(kChildFailure);
}
}
@@ -367,9 +382,6 @@ void Subprocess::runChild(const char* executable,
} else {
::execve(executable, argv, env);
}
-
- // If we're here, something's wrong.
- abort();
}
ProcessReturnCode Subprocess::poll() {
diff --git a/folly/test/SubprocessTest.cpp b/folly/test/SubprocessTest.cpp
index 5d3fd3a8..bcb163a3 100644
--- a/folly/test/SubprocessTest.cpp
+++ b/folly/test/SubprocessTest.cpp
@@ -49,6 +49,11 @@ TEST(SimpleSubprocessTest, ExitsWithErrorChecked) {
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) {
Subprocess proc("true");
EXPECT_EQ(0, proc.wait().exitStatus());
@@ -64,7 +69,7 @@ TEST(ParentDeathSubprocessTest, ParentDeathSignal) {
static constexpr size_t pathLength = 2048;
char buf[pathLength];
int r = readlink("/proc/self/exe", buf, pathLength);
- CHECK_ERR(r >= 0);
+ CHECK_ERR(r);
buf[r] = '\0';
fs::path helper(buf);
--
2.34.1