From c7be3b8248e8eeb3bec0cbaac8d6026cd64f8975 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alicia=20Boya=20Garc=C3=ADa?= Date: Fri, 10 Jan 2025 03:49:13 +0100 Subject: [PATCH] Replace covert pipe with self-pipe SIGCHLD handler For background, see https://github.com/ninja-build/ninja/issues/2444#issuecomment-2577999973. 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 https://github.com/ninja-build/ninja/issues/2444 --- src/subprocess-posix.cc | 125 +++++++++++++++++++++++++++++++++++++--- src/subprocess.h | 1 + 2 files changed, 119 insertions(+), 7 deletions(-) diff --git a/src/subprocess-posix.cc b/src/subprocess-posix.cc index 8e785406c9..91dfac2c84 100644 --- a/src/subprocess-posix.cc +++ b/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 @@ -123,14 +126,22 @@ bool Subprocess::Start(SubprocessSet* set, const string& command) { 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. + // Calling this is safe as SIGCHLD is blocked except during ppoll/pselect(). + CloseFileWhenPidExits(pid_, output_pipe[1]); + } 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 +161,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,12 +218,102 @@ 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(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() { + // 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 +326,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 +343,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)); } diff --git a/src/subprocess.h b/src/subprocess.h index 9e3d2ee98f..77edb122fe 100644 --- a/src/subprocess.h +++ b/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 };