Don't let call-process be a zombie factory.

Fixing this bug required some cleanup of the signal-handling code.
As a side effect, this change also fixes a longstanding rare race
condition whereby Emacs could mistakenly kill unrelated processes,
and it fixes a bug where a second C-g does not kill a recalcitrant
synchronous process in GNU/Linux and similar platforms.
The patch should also fix the last vestiges of Bug#9488,
a bug which has mostly been fixed on the trunk by other changes.
* callproc.c, process.h (synch_process_alive, synch_process_death)
(synch_process_termsig, sync_process_retcode):
Remove.  All uses removed, to simplify analysis and so that
less consing is done inside critical sections.
* callproc.c (call_process_exited): Remove.  All uses replaced
with !synch_process_pid.
* callproc.c (synch_process_pid, synch_process_fd): New static vars.
These take the role of what used to be in unwind-protect arg.
All uses changed.
(block_child_signal, unblock_child_signal):
New functions, to avoid races that could kill innocent-victim processes.
(call_process_kill, call_process_cleanup, Fcall_process): Use them.
(call_process_kill): Record killed processes as deleted, so that
zombies do not clutter up the system.  Do this inside a critical
section, to avoid a race that would allow the clutter.
(call_process_cleanup): Fix code so that the second C-g works again
on common platforms such as GNU/Linux.
(Fcall_process): Create the child process in a critical section,
to fix a race condition.  If creating an asynchronous process,
record it as deleted so that zombies do not clutter up the system.
Do unwind-protect for WINDOWSNT too, as that's simpler in the
light of these changes.  Omit unnecessary call to emacs_close
before failure, as the unwind-protect code does that.
* callproc.c (call_process_cleanup):
* w32proc.c (waitpid): Simplify now that synch_process_alive is gone.
* process.c (record_deleted_pid): New function, containing
code refactored out of Fdelete_process.
(Fdelete_process): Use it.
(process_status_retrieved): Remove.  All callers changed to use
child_status_change.
(record_child_status_change): Remove, folding its contents into ...
(handle_child_signal): ... this signal handler.  Now, this
function is purely a handler for SIGCHLD, and is not called after
a synchronous waitpid returns; the synchronous code is moved to
wait_for_termination.  There is no need to worry about reaping
more than one child now.
* sysdep.c (get_child_status, child_status_changed): New functions.
(wait_for_termination): Now takes int * status and bool
interruptible arguments, too.  Do not record child status change;
that's now the caller's responsibility.  All callers changed.
Reimplement in terms of get_child_status.
(wait_for_termination_1, interruptible_wait_for_termination):
Remove.  All callers changed to use wait_for_termination.
* syswait.h: Include <stdbool.h>, for bool.
(record_child_status_change, interruptible_wait_for_termination):
Remove decls.
(record_deleted_pid, child_status_changed): New decls.
(wait_for_termination): Adjust to API changes noted above.

Fixes: debbugs:12980

1 files changed, 52 insertions, 28 deletions

diff --git a/src/sysdep.c b/src/sysdep.c
index 1a3834f0379..7068a4f0977 100644
--- a/src/sysdep.c
+++ b/src/sysdep.c
@@ -266,45 +266,71 @@ init_baud_rate (int fd)
 
 #ifndef MSDOS
 
-static void
-wait_for_termination_1 (pid_t pid, int interruptible)
+/* Wait for the subprocess with process id CHILD to terminate or change status.
+   CHILD must be a child process that has not been reaped.
+   If STATUS is non-null, store the waitpid-style exit status into *STATUS
+   and tell wait_reading_process_output that it needs to look around.
+   Use waitpid-style OPTIONS when waiting.
+   If INTERRUPTIBLE, this function is interruptible by a signal.
+
+   Return CHILD if successful, 0 if no status is available;
+   the latter is possible only when options & NOHANG.  */
+static pid_t
+get_child_status (pid_t child, int *status, int options, bool interruptible)
 {
-  while (1)
+  pid_t pid;
+
+  /* Invoke waitpid only with a known process ID; do not invoke
+     waitpid with a nonpositive argument.  Otherwise, Emacs might
+     reap an unwanted process by mistake.  For example, invoking
+     waitpid (-1, ...) can mess up glib by reaping glib's subprocesses,
+     so that another thread running glib won't find them.  */
+  eassert (0 < child);
+
+  while ((pid = waitpid (child, status, options)) < 0)
     {
-      int status;
-      int wait_result = waitpid (pid, &status, 0);
-      if (wait_result < 0)
-        {
-          if (errno != EINTR)
-            break;
-        }
-      else
-        {
-          record_child_status_change (wait_result, status);
-          break;
-        }
+      /* CHILD must be a child process that has not been reaped, and
+         STATUS and OPTIONS must be valid.  */
+      eassert (errno == EINTR);
 
       /* Note: the MS-Windows emulation of waitpid calls QUIT
          internally.  */
       if (interruptible)
         QUIT;
     }
-}
 
-/* Wait for subprocess with process id `pid' to terminate and
-   make sure it will get eliminated (not remain forever as a zombie) */
+  /* If successful and status is requested, tell wait_reading_process_output
+     that it needs to wake up and look around.  */
+  if (pid && status && input_available_clear_time)
+    *input_available_clear_time = make_emacs_time (0, 0);
 
+  return pid;
+}
+
+/* Wait for the subprocess with process id CHILD to terminate.
+   CHILD must be a child process that has not been reaped.
+   If STATUS is non-null, store the waitpid-style exit status into *STATUS
+   and tell wait_reading_process_output that it needs to look around.
+   If INTERRUPTIBLE, this function is interruptible by a signal.  */
 void
-wait_for_termination (pid_t pid)
+wait_for_termination (pid_t child, int *status, bool interruptible)
 {
-  wait_for_termination_1 (pid, 0);
+  get_child_status (child, status, 0, interruptible);
 }
 
-/* Like the above, but allow keyboard interruption. */
-void
-interruptible_wait_for_termination (pid_t pid)
+/* Report whether the subprocess with process id CHILD has changed status.
+   Termination counts as a change of status.
+   CHILD must be a child process that has not been reaped.
+   If STATUS is non-null, store the waitpid-style exit status into *STATUS
+   and tell wait_reading_process_output that it needs to look around.
+   Use waitpid-style OPTIONS to check status, but do not wait.
+
+   Return CHILD if successful, 0 if no status is available because
+   the process's state has not changed.  */
+pid_t
+child_status_changed (pid_t child, int *status, int options)
 {
-  wait_for_termination_1 (pid, 1);
+  return get_child_status (child, status, WNOHANG | options, 0);
 }
 
 /*
@@ -454,6 +480,7 @@ sys_subshell (void)
   char oldwd[MAXPATHLEN+1]; /* Fixed length is safe on MSDOS.  */
 #endif
   pid_t pid;
+  int status;
   struct save_signal saved_handlers[5];
   Lisp_Object dir;
   unsigned char *volatile str_volatile = 0;
@@ -491,7 +518,6 @@ sys_subshell (void)
 #ifdef DOS_NT
   pid = 0;
   save_signal_handlers (saved_handlers);
-  synch_process_alive = 1;
 #else
   pid = vfork ();
   if (pid == -1)
@@ -560,14 +586,12 @@ sys_subshell (void)
   /* Do this now if we did not do it before.  */
 #ifndef MSDOS
   save_signal_handlers (saved_handlers);
-  synch_process_alive = 1;
 #endif
 
 #ifndef DOS_NT
-  wait_for_termination (pid);
+  wait_for_termination (pid, &status, 0);
 #endif
   restore_signal_handlers (saved_handlers);
-  synch_process_alive = 0;
 }
 
 static void