fix some of the warning/errors clang 3.1 reports
[folly.git] / folly / Subprocess.cpp
index 034165c374c8bfedaff53d7d339cf103bf9e336c..da69e5368e4649f1da07317e9b432077b9b6ffdb 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright 2012 Facebook, Inc.
+ * Copyright 2013 Facebook, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -33,7 +33,7 @@
 #include "folly/Conv.h"
 #include "folly/ScopeGuard.h"
 #include "folly/String.h"
-#include "folly/experimental/io/Cursor.h"
+#include "folly/io/Cursor.h"
 
 extern char** environ;
 
@@ -128,6 +128,11 @@ void checkUnixError(ssize_t ret, const char* msg) {
     throwSystemError(msg);
   }
 }
+void checkUnixError(ssize_t ret, int savedErrno, const char* msg) {
+  if (ret == -1) {
+    throwSystemError(savedErrno, msg);
+  }
+}
 
 // Check a wait() status, throw on non-successful
 void checkStatus(ProcessReturnCode returnCode) {
@@ -191,22 +196,8 @@ Subprocess::Subprocess(
 }
 
 Subprocess::~Subprocess() {
-  if (returnCode_.state() == ProcessReturnCode::RUNNING) {
-    LOG(ERROR) << "Subprocess destroyed without reaping; killing child.";
-    try {
-      kill();
-      wait();
-    } catch (...) {
-      LOG(FATAL) << "Killing child failed, terminating: "
-                 << exceptionStr(std::current_exception());
-    }
-  }
-  try {
-    closeAll();
-  } catch (...) {
-    LOG(FATAL) << "close failed, terminating: "
-               << exceptionStr(std::current_exception());
-  }
+  CHECK_NE(returnCode_.state(), ProcessReturnCode::RUNNING)
+    << "Subprocess destroyed without reaping child";
 }
 
 namespace {
@@ -288,15 +279,53 @@ void Subprocess::spawn(
     envVec = environ;
   }
 
+  // Block all signals around vfork; see http://ewontfix.com/7/.
+  //
+  // As the child may run in the same address space as the parent until
+  // the actual execve() system call, any (custom) signal handlers that
+  // the parent has might alter parent's memory if invoked in the child,
+  // with undefined results.  So we block all signals in the parent before
+  // vfork(), which will cause them to be blocked in the child as well (we
+  // rely on the fact that Linux, just like all sane implementations, only
+  // clones the calling thread).  Then, in the child, we reset all signals
+  // to their default dispositions (while still blocked), and unblock them
+  // (so the exec()ed process inherits the parent's signal mask)
+  //
+  // The parent also unblocks all signals as soon as vfork() returns.
+  sigset_t allBlocked;
+  int r = ::sigfillset(&allBlocked);
+  checkUnixError(r, "sigfillset");
+  sigset_t oldSignals;
+  r = pthread_sigmask(SIG_SETMASK, &allBlocked, &oldSignals);
+  checkPosixError(r, "pthread_sigmask");
+
   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);
+    }
+    // Unblock signals; restore signal mask.
+    int r = pthread_sigmask(SIG_SETMASK, &oldSignals, nullptr);
+    if (r != 0) abort();
+
     runChild(executable, argVec, envVec, options);
     // This should never return, but there's nothing else we can do here.
     abort();
   }
+  // 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;
 
-  // In parent
-  checkUnixError(pid, "vfork");
+  // 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);
+  checkUnixError(pid, savedErrno, "vfork");
+
+  // Child is alive
   pid_ = pid;
   returnCode_ = ProcessReturnCode(RV_RUNNING);
 
@@ -304,6 +333,8 @@ void Subprocess::spawn(
   for (int f : childFds) {
     closeChecked(f);
   }
+
+  checkPosixError(r, "pthread_sigmask");
 }
 
 namespace {
@@ -389,8 +420,12 @@ ProcessReturnCode Subprocess::wait() {
   returnCode_.enforce(ProcessReturnCode::RUNNING);
   DCHECK_GT(pid_, 0);
   int status;
-  pid_t found = ::waitpid(pid_, &status, 0);
+  pid_t found;
+  do {
+    found = ::waitpid(pid_, &status, 0);
+  } while (found == -1 && errno == EINTR);
   checkUnixError(found, "waitpid");
+  DCHECK_EQ(found, pid_);
   returnCode_ = ProcessReturnCode(status);
   return returnCode_;
 }
@@ -407,12 +442,6 @@ void Subprocess::sendSignal(int signal) {
 }
 
 namespace {
-void setNonBlocking(int fd) {
-  int flags = ::fcntl(fd, F_GETFL);
-  checkUnixError(flags, "fcntl");
-  int r = ::fcntl(fd, F_SETFL, flags | O_NONBLOCK);
-  checkUnixError(r, "fcntl");
-}
 
 std::pair<const uint8_t*, size_t> queueFront(const IOBufQueue& queue) {
   auto* p = queue.front();
@@ -482,7 +511,7 @@ bool discardRead(int fd) {
 }  // namespace
 
 std::pair<std::string, std::string> Subprocess::communicate(
-    int flags,
+    const CommunicateFlags& flags,
     StringPiece data) {
   IOBufQueue dataQueue;
   dataQueue.wrapBuffer(data.data(), data.size());
@@ -505,14 +534,14 @@ std::pair<std::string, std::string> Subprocess::communicate(
 }
 
 std::pair<IOBufQueue, IOBufQueue> Subprocess::communicateIOBuf(
-    int flags,
+    const CommunicateFlags& flags,
     IOBufQueue data) {
   std::pair<IOBufQueue, IOBufQueue> out;
 
-  auto readCallback = [&, flags] (int pfd, int cfd) {
-    if (cfd == 1 && (flags & READ_STDOUT)) {
+  auto readCallback = [&] (int pfd, int cfd) -> bool {
+    if (cfd == 1 && flags.readStdout_) {
       return handleRead(pfd, out.first);
-    } else if (cfd == 2 && (flags & READ_STDERR)) {
+    } else if (cfd == 2 && flags.readStderr_) {
       return handleRead(pfd, out.second);
     } else {
       // Don't close the file descriptor, the child might not like SIGPIPE,
@@ -521,8 +550,8 @@ std::pair<IOBufQueue, IOBufQueue> Subprocess::communicateIOBuf(
     }
   };
 
-  auto writeCallback = [&, flags] (int pfd, int cfd) {
-    if (cfd == 0 && (flags & WRITE_STDIN)) {
+  auto writeCallback = [&] (int pfd, int cfd) -> bool {
+    if (cfd == 0 && flags.writeStdin_) {
       return handleWrite(pfd, data);
     } else {
       // If we don't want to write to this fd, just close it.