From: Philipp Stephani Date: Sun, 10 Jan 2021 15:31:12 +0000 (+0100) Subject: Fix deadlock when receiving SIGCHLD during 'pselect'. X-Git-Tag: emacs-28.0.90~4248 X-Git-Url: http://git.eshelyaron.com/gitweb/?a=commitdiff_plain;h=8f0ce42d3eb9b212424a4a25a376287ffc94a73e;p=emacs.git 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 dac7d0440fa..474c87089e0 100644 --- 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 57097cfa052..dad36426a09 100644 --- 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