Replace covert pipe with self-pipe SIGCHLD handler

For background, see
ninja-build#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 the pipe file
descriptor to the subprocess without its knowledge, ninja keeps both
ends of the pipe to itself, and uses a SIGCHLD handler to close the
write end of the pipe when the subprocess has truly exited.

During testing I found Subprocess::Finish() lacked EINTR retrying, which
made ninja crash prematurely. This patch also fixes that.

Fixes ninja-build#2444

Author: ntrrgc

src/subprocess-posix.cc

@@ -36,6 +36,8 @@ extern char** environ;
 
 using namespace std;
 
+static void CloseFileWhenPidExits(pid_t pid, int fd);
+
 Subprocess::Subprocess(bool use_console) : fd_(-1), pid_(-1),
                                            use_console_(use_console) {
 }
@@ -103,12 +105,13 @@ bool Subprocess::Start(SubprocessSet* set, const string& command) {
     err = posix_spawn_file_actions_adddup2(&action, output_pipe[1], 2);
     if (err != 0)
       Fatal("posix_spawn_file_actions_adddup2: %s", strerror(err));
-    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.
   }
+  err = posix_spawn_file_actions_addclose(&action, output_pipe[1]);
+  if (err != 0)
+    Fatal("posix_spawn_file_actions_addclose: %s", strerror(err));
+
 #ifdef POSIX_SPAWN_USEVFORK
   flags |= POSIX_SPAWN_USEVFORK;
 #endif
@@ -117,20 +120,42 @@ bool Subprocess::Start(SubprocessSet* set, const string& command) {
   if (err != 0)
     Fatal("posix_spawnattr_setflags: %s", strerror(err));
 
+  if (use_console_) {
+    // Block SIGCHLD to prevent a race where the new subprocess exits before we
+    // register its PID.
+    sigset_t signals;
+    sigemptyset(&signals);
+    sigaddset(&signals, SIGCHLD);
+    if (sigprocmask(SIG_BLOCK, &signals, NULL) < 0)
+      Fatal("sigprocmask: %s", strerror(errno));
+  }
+
   const char* spawned_args[] = { "/bin/sh", "-c", command.c_str(), NULL };
   err = posix_spawn(&pid_, "/bin/sh", &action, &attr,
         const_cast<char**>(spawned_args), environ);
   if (err != 0)
     Fatal("posix_spawn: %s", strerror(err));
 
+  if (use_console_) {
+    // Shared console case: We keep the write-side of the pipe just so that we
+    // can close it on SIGCHLD reception.
+    CloseFileWhenPidExits(pid_, output_pipe[1]);
+    sigset_t signals;
+    sigemptyset(&signals);
+    sigaddset(&signals, SIGCHLD);
+    if (sigprocmask(SIG_UNBLOCK, &signals, NULL) < 0)
+      Fatal("sigprocmask: %s", strerror(errno));
+  } else {
+    // Not shared console: Only the subprocess needs the write-end of the pipe.
+    close(output_pipe[1]);
+  }
+
   err = posix_spawnattr_destroy(&attr);
   if (err != 0)
     Fatal("posix_spawnattr_destroy: %s", strerror(err));
   err = posix_spawn_file_actions_destroy(&action);
   if (err != 0)
     Fatal("posix_spawn_file_actions_destroy: %s", strerror(err));
-
-  close(output_pipe[1]);
   return true;
 }
 
@@ -150,8 +175,10 @@ void Subprocess::OnPipeReady() {
 ExitStatus Subprocess::Finish() {
   assert(pid_ != -1);
   int status;
-  if (waitpid(pid_, &status, 0) < 0)
-    Fatal("waitpid(%d): %s", pid_, strerror(errno));
+  while (waitpid(pid_, &status, 0) < 0) {
+    if (errno != EINTR)
+      Fatal("waitpid(%d): %s", pid_, strerror(errno));
+  }
   pid_ = -1;
 
 #ifdef _AIX
@@ -205,6 +232,93 @@ void SubprocessSet::HandlePendingInterruption() {
     interrupted_ = SIGHUP;
 }
 
+// SIGCHLD handling:
+
+struct PidFdEntry;
+typedef unsigned int IxEntry;
+// PidFdList is used to store the PID and file descriptor pairs of the
+// subprocesses being run by ninja that share a terminal.
+// On SIGCHLD the PID of the signal is matched with one of the entries and the
+// associated file descriptor closed. Then the entry is freed for reuse.
+// Normally we would use something like std::unordered_map<>, but that involves
+// memory allocations which are not reentrant.
+struct PidFdList {
+  PidFdEntry* entries; // Entry with index 0 is reserved as NULL and not used.
+  size_t capacity; // Number of PidFdEntry allocated for pid_fds.
+  IxEntry ix_head_used;
+  IxEntry ix_head_free;
+};
+
+// A PidFdEntry may be used or free.
+struct PidFdEntry {
+  pid_t pid; // -1 if this is a free entry
+  int fd;
+  // ix_next of a used entry points to another used entry, ix_next of free entry
+  // points to another free entry.
+  IxEntry ix_next;
+};
+
+static PidFdList PID_FD_LIST = { nullptr, 0, 0, 1 };
+
+static void initializePidFdEntries(PidFdEntry* entries, IxEntry ix_start, size_t capacity) {
+  for (unsigned int i = ix_start; i < capacity; i++) {
+    entries[i].pid = -1;
+    entries[i].fd = -1;
+    entries[i].ix_next = i + 1;
+  }
+  entries[capacity - 1].ix_next = 0; // end of the free list.
+}
+
+// Must only be called with SIGCHLD blocked.
+static void CloseFileWhenPidExits(pid_t pid, int fd) {
+  PidFdList* list = &PID_FD_LIST;
+  if (!list->entries) {
+    // First use, allocate the list.
+    list->capacity = 16;
+    list->entries = new PidFdEntry[list->capacity];
+    initializePidFdEntries(list->entries, 1, list->capacity);
+  } else if (!list->ix_head_free) {
+    // Expand the list to get more free entries.
+    size_t old_capacity = list->capacity;
+    list->capacity *= 2;
+    list->entries = static_cast<PidFdEntry*>(realloc(list->entries,
+      sizeof(PidFdEntry) * list->capacity));
+    initializePidFdEntries(list->entries, old_capacity, list->capacity);
+    list->ix_head_free = old_capacity;
+  }
+  // Replace the head free entry and make it the new used head.
+  IxEntry ix_entry = list->ix_head_free;
+  PidFdEntry* entry = &list->entries[ix_entry];
+  IxEntry ix_next_free = entry->ix_next;
+  entry->pid = pid;
+  entry->fd = fd;
+  entry->ix_next = list->ix_head_used;
+  list->ix_head_used = ix_entry;
+  list->ix_head_free = ix_next_free;
+}
+
+static void SigChldHandler(int signo, siginfo_t* info, void* context) {
+  int pid = info->si_pid;
+
+  PidFdList* list = &PID_FD_LIST;
+  if (!list->entries)
+    return;
+  for (IxEntry* ix = &list->ix_head_used; *ix; ix = &list->entries[*ix].ix_next) {
+    PidFdEntry* entry = &list->entries[*ix];
+    if (entry->pid == pid) {
+      // Found a match. Close the pipe and remove the entry.
+      close(entry->fd);
+      IxEntry ix_next_used = entry->ix_next;
+      entry->fd = -1;
+      entry->pid = -1;
+      entry->ix_next = list->ix_head_free;
+      list->ix_head_free = *ix;
+      *ix = ix_next_used;
+      break;
+    }
+  }
+}
+
 SubprocessSet::SubprocessSet() {
   sigset_t set;
   sigemptyset(&set);
@@ -223,6 +337,12 @@ 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));
 }
 
 SubprocessSet::~SubprocessSet() {
@@ -234,6 +354,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));
 }

src/subprocess.h

@@ -106,6 +106,7 @@ struct SubprocessSet {
   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
 };