Replace covert pipe with SIGCHLD handler

For background, see
#2444 (comment).

In short, when running subprocesses that share the terminal, ninja
intentionally leaves a pipe open before exec() so that it can use EOF
from that pipe to detect when the subprocess has exited.

That mechanism is problematic: If the subprocess ends up spawning
background processes (e.g. sccache), those would also inherit the pipe
by default. In that case, ninja may not detect process termination until
all background processes have quitted.

This patch makes it so that, instead of propagating a pipe file
descriptor to the subprocess without its knowledge, ninja relies on the
SIGCHLD signal together with waitpid(WNOHANG) to detect termination of
console subprocesses.

After this patch, console-sharing subprocesses do no longer have an
associated pipe.

Fixes #2444

src/subprocess-posix.cc

@@ -16,6 +16,7 @@
 
 #include <sys/select.h>
 #include <assert.h>
+#include <atomic>
 #include <errno.h>
 #include <fcntl.h>
 #include <unistd.h>
@@ -36,6 +37,8 @@ extern char** environ;
 
 using namespace std;
 
+static ExitStatus ParseExitStatus(int status);
+
 Subprocess::Subprocess(bool use_console) : fd_(-1), pid_(-1),
                                            use_console_(use_console) {
 }
@@ -49,26 +52,32 @@ Subprocess::~Subprocess() {
 }
 
 bool Subprocess::Start(SubprocessSet* set, const string& command) {
-  int output_pipe[2];
-  if (pipe(output_pipe) < 0)
-    Fatal("pipe: %s", strerror(errno));
-  fd_ = output_pipe[0];
+  int output_pipe[2] = {-1, -1};
+  if (use_console_) {
+    fd_ = -1;
+  } else {
+    if (pipe(output_pipe) < 0)
+      Fatal("pipe: %s", strerror(errno));
+    fd_ = output_pipe[0];
 #if !defined(USE_PPOLL)
-  // If available, we use ppoll in DoWork(); otherwise we use pselect
-  // and so must avoid overly-large FDs.
-  if (fd_ >= static_cast<int>(FD_SETSIZE))
-    Fatal("pipe: %s", strerror(EMFILE));
+    // If available, we use ppoll in DoWork(); otherwise we use pselect
+    // and so must avoid overly-large FDs.
+    if (fd_ >= static_cast<int>(FD_SETSIZE))
+      Fatal("pipe: %s", strerror(EMFILE));
 #endif  // !USE_PPOLL
-  SetCloseOnExec(fd_);
+    SetCloseOnExec(fd_);
+  }
 
   posix_spawn_file_actions_t action;
   int err = posix_spawn_file_actions_init(&action);
   if (err != 0)
     Fatal("posix_spawn_file_actions_init: %s", strerror(err));
 
-  err = posix_spawn_file_actions_addclose(&action, output_pipe[0]);
-  if (err != 0)
-    Fatal("posix_spawn_file_actions_addclose: %s", strerror(err));
+  if (!use_console_) {
+    err = posix_spawn_file_actions_addclose(&action, output_pipe[0]);
+    if (err != 0)
+      Fatal("posix_spawn_file_actions_addclose: %s", strerror(err));
+  }
 
   posix_spawnattr_t attr;
   err = posix_spawnattr_init(&attr);
@@ -106,9 +115,8 @@ bool Subprocess::Start(SubprocessSet* set, const string& command) {
     err = posix_spawn_file_actions_addclose(&action, output_pipe[1]);
     if (err != 0)
       Fatal("posix_spawn_file_actions_addclose: %s", strerror(err));
-    // In the console case, output_pipe is still inherited by the child and
-    // closed when the subprocess finishes, which then notifies ninja.
   }
+
 #ifdef POSIX_SPAWN_USEVFORK
   flags |= POSIX_SPAWN_USEVFORK;
 #endif
@@ -130,7 +138,8 @@ bool Subprocess::Start(SubprocessSet* set, const string& command) {
   if (err != 0)
     Fatal("posix_spawn_file_actions_destroy: %s", strerror(err));
 
-  close(output_pipe[1]);
+  if (!use_console_)
+    close(output_pipe[1]);
   return true;
 }
 
@@ -147,13 +156,30 @@ void Subprocess::OnPipeReady() {
   }
 }
 
-ExitStatus Subprocess::Finish() {
+
+bool Subprocess::TryFinish(int options) {
   assert(pid_ != -1);
-  int status;
-  if (waitpid(pid_, &status, 0) < 0)
-    Fatal("waitpid(%d): %s", pid_, strerror(errno));
+  int status, ret;
+  while ((ret = waitpid(pid_, &status, options)) < 0) {
+    if (errno != EINTR)
+      Fatal("waitpid(%d): %s", pid_, strerror(errno));
+  }
+  if (ret == 0)
+    return false; // Subprocess is alive (WNOHANG-only).
   pid_ = -1;
+  exit_status_ = ParseExitStatus(status);
+  return true; // Subprocess has terminated.
+}
+
+ExitStatus Subprocess::Finish() {
+  if (pid_ != -1) {
+    TryFinish(0);
+    assert(pid_ == -1);
+  }
+  return exit_status_;
+}
 
+static ExitStatus ParseExitStatus(int status) {
 #ifdef _AIX
   if (WIFEXITED(status) && WEXITSTATUS(status) & 0x80) {
     // Map the shell's exit code used for signal failure (128 + signal) to the
@@ -177,19 +203,28 @@ ExitStatus Subprocess::Finish() {
 }
 
 bool Subprocess::Done() const {
-  return fd_ == -1;
+  // Console subprocesses share console with ninja, and we consider them done
+  // when they exit.
+  // For other processes, we consider them done when we have consumed all their
+  // output and closed their associated pipe.
+  return (use_console_ && pid_ == -1) || (!use_console_ && fd_ == -1);
 }
 
 const string& Subprocess::GetOutput() const {
   return buf_;
 }
 
-int SubprocessSet::interrupted_;
+volatile int SubprocessSet::interrupted_;
+volatile sig_atomic_t SubprocessSet::s_sigchld_received;
 
 void SubprocessSet::SetInterruptedFlag(int signum) {
   interrupted_ = signum;
 }
 
+void SubprocessSet::SigChldHandler(int signo, siginfo_t* info, void* context) {
+  s_sigchld_received = 1;
+}
+
 void SubprocessSet::HandlePendingInterruption() {
   sigset_t pending;
   sigemptyset(&pending);
@@ -206,11 +241,14 @@ void SubprocessSet::HandlePendingInterruption() {
 }
 
 SubprocessSet::SubprocessSet() {
+  // Block all these signals.
+  // Their handlers will only be enabled during ppoll/pselect().
   sigset_t set;
   sigemptyset(&set);
   sigaddset(&set, SIGINT);
   sigaddset(&set, SIGTERM);
   sigaddset(&set, SIGHUP);
+  sigaddset(&set, SIGCHLD);
   if (sigprocmask(SIG_BLOCK, &set, &old_mask_) < 0)
     Fatal("sigprocmask: %s", strerror(errno));
 
@@ -223,6 +261,26 @@ SubprocessSet::SubprocessSet() {
     Fatal("sigaction: %s", strerror(errno));
   if (sigaction(SIGHUP, &act, &old_hup_act_) < 0)
     Fatal("sigaction: %s", strerror(errno));
+
+  memset(&act, 0, sizeof(act));
+  act.sa_flags = SA_SIGINFO | SA_NOCLDSTOP;
+  act.sa_sigaction = SigChldHandler;
+  if (sigaction(SIGCHLD, &act, &old_chld_act_) < 0)
+    Fatal("sigaction: %s", strerror(errno));
+}
+
+void SubprocessSet::CheckConsoleProcessTerminated() {
+  if (!s_sigchld_received)
+    return;
+  for (vector<Subprocess*>::iterator i = running_.begin();
+       i != running_.end(); ) {
+    if ((*i)->use_console_ && (*i)->TryFinish(WNOHANG)) {
+      finished_.push(*i);
+      i = running_.erase(i);
+    } else {
+      ++i;
+    }
+  }
 }
 
 SubprocessSet::~SubprocessSet() {
@@ -234,6 +292,8 @@ SubprocessSet::~SubprocessSet() {
     Fatal("sigaction: %s", strerror(errno));
   if (sigaction(SIGHUP, &old_hup_act_, 0) < 0)
     Fatal("sigaction: %s", strerror(errno));
+  if (sigaction(SIGCHLD, &old_chld_act_, 0) < 0)
+    Fatal("sigaction: %s", strerror(errno));
   if (sigprocmask(SIG_SETMASK, &old_mask_, 0) < 0)
     Fatal("sigprocmask: %s", strerror(errno));
 }
@@ -262,17 +322,30 @@ bool SubprocessSet::DoWork() {
     fds.push_back(pfd);
     ++nfds;
   }
+  if (nfds == 0) {
+    // Add a dummy entry to prevent using an empty pollfd vector.
+    // ppoll() allows to do this by setting fd < 0.
+    pollfd pfd = { -1, 0, 0 };
+    fds.push_back(pfd);
+    ++nfds;
+  }
 
   interrupted_ = 0;
+  s_sigchld_received = 0;
+  std::atomic_signal_fence(std::memory_order_release);
   int ret = ppoll(&fds.front(), nfds, NULL, &old_mask_);
   if (ret == -1) {
     if (errno != EINTR) {
       perror("ninja: ppoll");
       return false;
     }
+    CheckConsoleProcessTerminated();
     return IsInterrupted();
   }
 
+  // ppoll/pselect prioritizes file descriptor events over a signal delivery.
+  // However, if the user is trying to quit ninja, we should react as fast as
+  // possible.
   HandlePendingInterruption();
   if (IsInterrupted())
     return true;
@@ -315,15 +388,20 @@ bool SubprocessSet::DoWork() {
   }
 
   interrupted_ = 0;
-  int ret = pselect(nfds, &set, 0, 0, 0, &old_mask_);
+  s_sigchld_received = 0;
+  int ret = pselect(nfds, (nfds > 0 ? &set : nullptr), 0, 0, 0, &old_mask_);
   if (ret == -1) {
     if (errno != EINTR) {
       perror("ninja: pselect");
       return false;
     }
+    CheckConsoleProcessTerminated();
     return IsInterrupted();
   }
 
+  // ppoll/pselect prioritizes file descriptor events over a signal delivery.
+  // However, if the user is trying to quit ninja, we should react as fast as
+  // possible.
   HandlePendingInterruption();
   if (IsInterrupted())
     return true;

src/subprocess.h

@@ -68,8 +68,27 @@ struct Subprocess {
   char overlapped_buf_[4 << 10];
   bool is_reading_;
 #else
+  /// The file descriptor that will be used in ppoll/pselect() for this process,
+  /// if any. Otherwise -1.
+  /// In non-console mode, this is the read-side of a pipe that was created
+  /// specifically for this subprocess. The write-side of the pipe is given to
+  /// the subprocess as stdout.
+  /// In console mode no pipe is created: fd_ is -1, and process termination is
+  /// detected using the SIGCHLD signal and waitpid(WNOHANG).
   int fd_;
+  /// PID of the subprocess. Set to -1 when the subprocess is reaped.
   pid_t pid_;
+  /// In POSIX platforms it is necessary to use waitpid(WNOHANG) to know whether
+  /// a certain subprocess has finished. This is done for terminal subprocesses.
+  /// However, this also causes the subprocess to be reaped before Finish() is
+  /// called, so we need to store the ExitStatus so that a later Finish()
+  /// invocation can return it.
+  ExitStatus exit_status_;
+
+  /// Call waitpid() on the subprocess with the provided options and update the
+  /// pid_ and exit_status_ fields.
+  /// Return a boolean indicating whether the subprocess has indeed terminated.
+  bool TryFinish(int options);
 #endif
   bool use_console_;
 
@@ -96,16 +115,24 @@ struct SubprocessSet {
   static HANDLE ioport_;
 #else
   static void SetInterruptedFlag(int signum);
-  static void HandlePendingInterruption();
+  static void SigChldHandler(int signo, siginfo_t* info, void* context);
+
   /// Store the signal number that causes the interruption.
   /// 0 if not interruption.
-  static int interrupted_;
-
+  static volatile int interrupted_;
+  /// Whether ninja should quit. Set on SIGINT, SIGTERM or SIGHUP reception.
   static bool IsInterrupted() { return interrupted_ != 0; }
+  static void HandlePendingInterruption();
+
+  /// Initialized to 0 before ppoll/pselect().
+  /// Filled to 1 by SIGCHLD handler when a child process terminates.
+  static volatile sig_atomic_t s_sigchld_received;
+  void CheckConsoleProcessTerminated();
 
   struct sigaction old_int_act_;
   struct sigaction old_term_act_;
   struct sigaction old_hup_act_;
+  struct sigaction old_chld_act_;
   sigset_t old_mask_;
 #endif
 };