Fix deadlock when receiving SIGCHLD during 'pselect'.

If we receive and handle a SIGCHLD signal for a process while waiting
for that process, 'pselect' might never return.  Instead, we have to
explicitly 'pselect' that the process status has changed.  We do this
by writing to a pipe in the SIGCHLD handler and having
'wait_reading_process_output' select on it.

* src/process.c (child_signal_init): New helper function to create a
pipe for SIGCHLD notifications.
(child_signal_read, child_signal_notify): New helper functions to
read from/write to the child signal pipe.
(create_process): Initialize the child signal pipe on first use.
(handle_child_signal): Notify waiters that a process status has
changed.
(wait_reading_process_output): Make sure that we also catch
SIGCHLD/process status changes.

* test/src/process-tests.el
(process-tests/fd-setsize-no-crash/make-process): Remove workaround,
which is no longer needed.

diff --git a/src/process.c b/src/process.c
index dac7d0440faf49fcc0aa0ae2ff9172a5d5db35c8..474c87089e092b2783ee52c16dd46c994a8763c0 100644 (file)
--- a/src/process.c
+++ b/src/process.c
@@ -283,6 +283,16 @@ static int max_desc;
    the file descriptor of a socket that is already bound.  */
 static int external_sock_fd;
 
+/* File descriptor that becomes readable when we receive SIGCHLD.  */
+static int child_signal_read_fd = -1;
+/* The write end thereof.  The SIGCHLD handler writes to this file
+   descriptor to notify `wait_reading_process_output' of process
+   status changes.  */
+static int child_signal_write_fd = -1;
+static void child_signal_init (void);
+static void child_signal_read (int, void *);
+static void child_signal_notify (void);
+
 /* Indexed by descriptor, gives the process (if any) for that descriptor.  */
 static Lisp_Object chan_process[FD_SETSIZE];
 static void wait_for_socket_fds (Lisp_Object, char const *);
@@ -2060,6 +2070,10 @@ create_process (Lisp_Object process, char **new_argv, Lisp_Object current_dir)
   Lisp_Object lisp_pty_name = Qnil;
   sigset_t oldset;
 
+  /* Ensure that the SIGCHLD handler can notify
+     `wait_reading_process_output'.  */
+  child_signal_init ();
+
   inchannel = outchannel = -1;
 
   if (p->pty_flag)
@@ -5395,6 +5409,14 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
          check_write = true;
        }
 
+      /* We have to be informed when we receive a SIGCHLD signal for
+        an asynchronous process.  Otherwise this might deadlock if we
+        receive a SIGCHLD during `pselect'.  */
+      int child_fd = child_signal_read_fd;
+      eassert (0 <= child_fd);
+      eassert (child_fd < FD_SETSIZE);
+      FD_SET (child_fd, &Available);
+
       /* If frame size has changed or the window is newly mapped,
         redisplay now, before we start to wait.  There is a race
         condition here; if a SIGIO arrives between now and the select
@@ -7114,7 +7136,70 @@ process has been transmitted to the serial port.  */)
    subprocesses which the main thread should not reap.  For example,
    if the main thread attempted to reap an already-reaped child, it
    might inadvertently reap a GTK-created process that happened to
-   have the same process ID.  */
+   have the same process ID.
+
+   To avoid a deadlock when receiving SIGCHLD while
+   `wait_reading_process_output' is in `pselect', the SIGCHLD handler
+   will notify the `pselect' using a pipe.  */
+
+/* Set up `child_signal_read_fd' and `child_signal_write_fd'.  */
+
+static void
+child_signal_init (void)
+{
+  /* Either both are initialized, or both are uninitialized.  */
+  eassert ((child_signal_read_fd < 0) == (child_signal_write_fd < 0));
+
+  if (0 <= child_signal_read_fd)
+    return; /* already done */
+
+  int fds[2];
+  if (emacs_pipe (fds) < 0)
+    report_file_error ("Creating pipe for child signal", Qnil);
+  if (FD_SETSIZE <= fds[0])
+    {
+      /* Since we need to `pselect' on the read end, it has to fit
+        into an `fd_set'.  */
+      emacs_close (fds[0]);
+      emacs_close (fds[1]);
+      report_file_errno ("Creating pipe for child signal", Qnil,
+                        EMFILE);
+    }
+
+  /* We leave the file descriptors open until the Emacs process
+     exits.  */
+  eassert (0 <= fds[0]);
+  eassert (0 <= fds[1]);
+  add_read_fd (fds[0], child_signal_read, NULL);
+  fd_callback_info[fds[0]].flags &= ~KEYBOARD_FD;
+  child_signal_read_fd = fds[0];
+  child_signal_write_fd = fds[1];
+}
+
+/* Consume a process status change.  */
+
+static void
+child_signal_read (int fd, void *data)
+{
+  eassert (0 <= fd);
+  eassert (fd == child_signal_read_fd);
+  char dummy;
+  if (emacs_read (fd, &dummy, 1) < 0)
+    emacs_perror ("reading from child signal FD");
+}
+
+/* Notify `wait_reading_process_output' of a process status
+   change.  */
+
+static void
+child_signal_notify (void)
+{
+  int fd = child_signal_write_fd;
+  eassert (0 <= fd);
+  char dummy = 0;
+  if (emacs_write (fd, &dummy, 1) != 1)
+    emacs_perror ("writing to child signal FD");
+}
 
 /* LIB_CHILD_HANDLER is a SIGCHLD handler that Emacs calls while doing
    its own SIGCHLD handling.  On POSIXish systems, glib needs this to
@@ -7152,6 +7237,7 @@ static void
 handle_child_signal (int sig)
 {
   Lisp_Object tail, proc;
+  bool changed = false;
 
   /* Find the process that signaled us, and record its status.  */
 
@@ -7174,6 +7260,7 @@ handle_child_signal (int sig)
          eassert (ok);
          if (child_status_changed (deleted_pid, 0, 0))
            {
+             changed = true;
              if (STRINGP (XCDR (head)))
                unlink (SSDATA (XCDR (head)));
              XSETCAR (tail, Qnil);
@@ -7191,6 +7278,7 @@ handle_child_signal (int sig)
          && child_status_changed (p->pid, &status, WUNTRACED | WCONTINUED))
        {
          /* Change the status of the process that was found.  */
+         changed = true;
          p->tick = ++process_tick;
          p->raw_status = status;
          p->raw_status_new = 1;
@@ -7210,6 +7298,10 @@ handle_child_signal (int sig)
        }
     }
 
+  if (changed)
+    /* Wake up `wait_reading_process_output'.  */
+    child_signal_notify ();
+
   lib_child_handler (sig);
 #ifdef NS_IMPL_GNUSTEP
   /* NSTask in GNUstep sets its child handler each time it is called.

diff --git a/test/src/process-tests.el b/test/src/process-tests.el
index 57097cfa05263b033e50fb54e43ef4c0dbf513a8..dad36426a09d2122ae555878f0f82cda8278f8c3 100644 (file)
--- a/test/src/process-tests.el
+++ b/test/src/process-tests.el
@@ -576,11 +576,6 @@ FD_SETSIZE file descriptors (Bug#24325)."
                 (should (memq (process-status process) '(run exit)))
                 (when (process-live-p process)
                   (process-send-eof process))
-                ;; FIXME: This `sleep-for' shouldn't be needed.  It
-                ;; indicates a bug in Emacs; perhaps SIGCHLD is
-                ;; received in parallel with `accept-process-output',
-                ;; causing the latter to hang.
-                (sleep-for 0.1)
                 (while (accept-process-output process))
                 (should (eq (process-status process) 'exit))
                 ;; If there's an error between fork and exec, Emacs