From: Tudor Bosman <tudorb@fb.com>
Date: Sat, 13 Apr 2013 06:49:32 +0000 (-0700)
Subject: Make Subprocess::spawn more robust
X-Git-Tag: v0.22.0~1006
X-Git-Url: http://demsky.eecs.uci.edu/git/?a=commitdiff_plain;h=e33bebd37a24287368fdebc0d085d7319083ba08;p=folly.git

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
---

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);