SIGCHLD in vfork child context

SIGCHLD in vfork child context

[YAMAMOTO Mitsuharu]

Recently Emacs on Darwin has changed to use vfork rather than fork for
performance reasons (Bug#26397).  The latest Mac port contains a
similar change, but I received a report telling that this causes
occasional hangs.

It seems that calling waitpid from the SIGCHLD handler results in
ECHILD in the vfork child context, and in this case, Emacs aborts and
then hangs in such a context for some reason on macOS.

src/sysdep.c:
   385	  while (true)
   386	    {
   387	      /* Note: the MS-Windows emulation of waitpid calls maybe_quit
   388		 internally.  */
   389	      if (interruptible)
   390		maybe_quit ();
   391	
   392	      pid = waitpid (child, status, options);
   393	      if (0 <= pid)
   394		break;
   395	
   396	      /* Check that CHILD is a child process that has not been reaped,
   397		 and that STATUS and OPTIONS are valid.  Otherwise abort,
   398		 as continuing after this internal error could cause Emacs to
   399		 become confused and kill innocent-victim processes.  */
   400	      if (errno != EINTR)
   401		emacs_abort ();
   402	    }


I made a patch below to avoid calling the SIGCHLD handler on Darwin,
but I wonder if this can also be meaningful in other platforms.  WDYT?

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

diff --git a/src/callproc.c b/src/callproc.c
index 333dbb8cb4..83597bd580 100644
--- a/src/callproc.c
+++ b/src/callproc.c
@@ -634,6 +634,11 @@ call_process (ptrdiff_t nargs, Lisp_Object *args, int filefd,
 
   if (pid == 0)
     {
+#ifdef DARWIN_OS
+      /* Call to waitpid from the SIGCHLD signal handler results in
+	 ECHILD in the vfork child context.  */
+      signal (SIGCHLD, SIG_DFL);
+#endif
       unblock_child_signal (&oldset);
 
 #ifdef DARWIN_OS
@@ -681,6 +686,18 @@ call_process (ptrdiff_t nargs, Lisp_Object *args, int filefd,
 	}
     }
 
+#ifdef DARWIN_OS
+  {
+    struct sigaction action;
+
+    /* Call the SIGCHLD handler in case we have received SIGCHLD in
+       the vfork child context.  The signal mask is restored by
+       unblock_child_signal below.  */
+    sigaction (SIGCHLD, 0, &action);
+    pthread_sigmask (SIG_BLOCK, &action.sa_mask, 0);
+    (*action.sa_handler) (SIGCHLD);
+  }
+#endif
   unblock_child_signal (&oldset);
   unblock_input ();
 
diff --git a/src/process.c b/src/process.c
index c47e1eac52..95867e27db 100644
--- a/src/process.c
+++ b/src/process.c
@@ -1940,6 +1940,11 @@ create_process (Lisp_Object process, char **new_argv, Lisp_Object current_dir)
       /* Emacs ignores SIGPIPE, but the child should not.  */
       signal (SIGPIPE, SIG_DFL);
 
+#ifdef DARWIN_OS
+      /* Call to waitpid from the SIGCHLD signal handler results in
+	 ECHILD in the vfork child context.  */
+      signal (SIGCHLD, SIG_DFL);
+#endif
       /* Stop blocking SIGCHLD in the child.  */
       unblock_child_signal (&oldset);
 
@@ -1962,6 +1967,18 @@ create_process (Lisp_Object process, char **new_argv, Lisp_Object current_dir)
   if (pid >= 0)
     p->alive = 1;
 
+#ifdef DARWIN_OS
+  {
+    struct sigaction action;
+
+    /* Call the SIGCHLD handler in case we have received SIGCHLD in
+       the vfork child context.  The signal mask is restored by
+       unblock_child_signal below.  */
+    pthread_sigmask (SIG_BLOCK, &action.sa_mask, 0);
+    sigaction (SIGCHLD, 0, &action);
+    (*action.sa_handler) (SIGCHLD);
+  }
+#endif
   /* Stop blocking in the parent.  */
   unblock_child_signal (&oldset);
   unblock_input ();

Re: SIGCHLD in vfork child context

[YAMAMOTO Mitsuharu]

>>>>> On Sat, 13 May 2017 14:41:59 +0900, YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp> said:

> Recently Emacs on Darwin has changed to use vfork rather than fork
> for performance reasons (Bug#26397).  The latest Mac port contains a
> similar change, but I received a report telling that this causes
> occasional hangs.

> It seems that calling waitpid from the SIGCHLD handler results in
> ECHILD in the vfork child context, and in this case, Emacs aborts
> and then hangs in such a context for some reason on macOS.

> I made a patch below to avoid calling the SIGCHLD handler on Darwin,
> but I wonder if this can also be meaningful in other platforms.
> WDYT?

The original reporter could come up with a procedure to reproduce:

1. Enter (call-process-region "abc" nil "cat" nil 0) in *scratch*
2. C-x ( C-x C-e C-x ) to define a macro that evaluates that form
3. C-u 2000 C-x e to evaluate the form 2000 times

I could reproduce the hang on macOS, but not on GNU/Linux.  Could
other platform users try it to see if the patch for Darwin is also
meaningful there?


				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

Re: SIGCHLD in vfork child context

[Paul Eggert]

YAMAMOTO Mitsuharu wrote:
> It seems that calling waitpid from the SIGCHLD handler results in
> ECHILD in the vfork child context

Why is the vfork child process getting a SIGCHLD signal? That process doesn't 
have any children. Is the macOS kernel simply buggy? If so, I suggest adding a 
short explanation of the situation to this comment:

> +    /* Call the SIGCHLD handler in case we have received SIGCHLD in
> +       the vfork child context.  The signal mask is restored by
> +       unblock_child_signal below.  */

If not, then I don't understand the situation....

> +    (*action.sa_handler) (SIGCHLD);

No need for the * and the first set of parens. I.e., this is OK:

     action.sa_handler (SIGCHLD);

Otherwise, looks OK; thanks.

Re: SIGCHLD in vfork child context

[Andreas Schwab]

On Mai 16 2017, Paul Eggert <eggert@cs.ucla.edu> wrote:

> YAMAMOTO Mitsuharu wrote:
>> It seems that calling waitpid from the SIGCHLD handler results in
>> ECHILD in the vfork child context
>
> Why is the vfork child process getting a SIGCHLD signal?

In any case, the sigchld handler should handle that case gracefully.
Spurious SIGCHLD should not cause emacs to abort.

> That process
> doesn't have any children. Is the macOS kernel simply buggy? If so, I
> suggest adding a short explanation of the situation to this comment:
>
>> +    /* Call the SIGCHLD handler in case we have received SIGCHLD in
>> +       the vfork child context.  The signal mask is restored by
>> +       unblock_child_signal below.  */
>
> If not, then I don't understand the situation....
>
>> +    (*action.sa_handler) (SIGCHLD);
>
> No need for the * and the first set of parens. I.e., this is OK:
>
>     action.sa_handler (SIGCHLD);
>
> Otherwise, looks OK; thanks.

IMHO this is barking up the wrong tree.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: SIGCHLD in vfork child context

[Paul Eggert]

Andreas Schwab wrote:
> IMHO this is barking up the wrong tree.

Which tree should we be barking up instead?

Re: SIGCHLD in vfork child context

[Andreas Schwab]

On Mai 16 2017, Paul Eggert <eggert@cs.ucla.edu> wrote:

> Andreas Schwab wrote:
>> IMHO this is barking up the wrong tree.
>
> Which tree should we be barking up instead?

Don't abort.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: SIGCHLD in vfork child context

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 222 bytes --]

Andreas Schwab wrote:
>> Which tree should we be barking up instead?
> Don't abort.

Good suggestion, thanks. I installed the attached patch to try to bark up the 
right tree, or to chew up the tree bark, or whatever....


[-- Attachment #2: 0001-Attempt-to-work-around-macOS-vfork-bug.patch --]
[-- Type: text/x-patch, Size: 4896 bytes --]

From 7c951fd51832badb09055a8e177f8ec358cbbdcf Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 19 May 2017 00:11:48 -0700
Subject: [PATCH] Attempt to work around macOS vfork bug

Problem reported by YAMAMOTO Mitsuharu in:
http://lists.gnu.org/archive/html/emacs-devel/2017-05/msg00342.html
This is related to the fix for Bug#26397.
* src/callproc.c (call_process_cleanup, call_process) [!MSDOS]:
Report internal error if wait_for_termination fails.
* src/sysdep.c (get_child_status): Return -1 if waitpid is
buggy, instead of aborting.
(wait_for_termination): Return bool success value.
All callers changed.
---
 src/callproc.c | 13 +++++++++----
 src/sysdep.c   | 25 ++++++++++++++-----------
 src/syswait.h  |  2 +-
 3 files changed, 24 insertions(+), 16 deletions(-)

diff --git a/src/callproc.c b/src/callproc.c
index e967e45..7c85eed 100644
--- a/src/callproc.c
+++ b/src/callproc.c
@@ -202,10 +202,11 @@ call_process_cleanup (Lisp_Object buffer)
       message1 ("Waiting for process to die...(type C-g again to kill it instantly)");
 
       /* This will quit on C-g.  */
-      wait_for_termination (synch_process_pid, 0, 1);
-
+      bool wait_ok = wait_for_termination (synch_process_pid, NULL, true);
       synch_process_pid = 0;
-      message1 ("Waiting for process to die...done");
+      message1 (wait_ok
+		? "Waiting for process to die...done"
+		: "Waiting for process to die...internal error");
     }
 #endif	/* !MSDOS */
 }
@@ -866,9 +867,10 @@ call_process (ptrdiff_t nargs, Lisp_Object *args, int filefd,
 	       make_number (total_read));
     }
 
+  bool wait_ok = true;
 #ifndef MSDOS
   /* Wait for it to terminate, unless it already has.  */
-  wait_for_termination (pid, &status, fd0 < 0);
+  wait_ok = wait_for_termination (pid, &status, fd0 < 0);
 #endif
 
   /* Don't kill any children that the subprocess may have left behind
@@ -878,6 +880,9 @@ call_process (ptrdiff_t nargs, Lisp_Object *args, int filefd,
   SAFE_FREE ();
   unbind_to (count, Qnil);
 
+  if (!wait_ok)
+    return build_unibyte_string ("internal error");
+
   if (WIFSIGNALED (status))
     {
       const char *signame;
diff --git a/src/sysdep.c b/src/sysdep.c
index ac6eed0..70f4a9d 100644
--- a/src/sysdep.c
+++ b/src/sysdep.c
@@ -368,8 +368,8 @@ init_baud_rate (int fd)
    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.  */
+   Return CHILD if successful, 0 if no status is available, and a
+   negative value (setting errno) if waitpid is buggy.  */
 static pid_t
 get_child_status (pid_t child, int *status, int options, bool interruptible)
 {
@@ -392,13 +392,14 @@ get_child_status (pid_t child, int *status, int options, bool interruptible)
       pid = waitpid (child, status, options);
       if (0 <= pid)
 	break;
-
-      /* Check that CHILD is a child process that has not been reaped,
-	 and that STATUS and OPTIONS are valid.  Otherwise abort,
-	 as continuing after this internal error could cause Emacs to
-	 become confused and kill innocent-victim processes.  */
       if (errno != EINTR)
-	emacs_abort ();
+	{
+	  /* Most likely, waitpid is buggy and the operating system
+	     lost track of the child somehow.  Return -1 and let the
+	     caller try to figure things out.  Possibly the bug could
+	     cause Emacs to kill the wrong process.  Oh well.  */
+	  return pid;
+	}
     }
 
   /* If successful and status is requested, tell wait_reading_process_output
@@ -413,11 +414,13 @@ get_child_status (pid_t child, int *status, int options, bool interruptible)
    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
+   If INTERRUPTIBLE, this function is interruptible by a signal.
+   Return true if successful, false (setting errno) if CHILD cannot be
+   waited for because waitpid is buggy.  */
+bool
 wait_for_termination (pid_t child, int *status, bool interruptible)
 {
-  get_child_status (child, status, 0, interruptible);
+  return 0 <= get_child_status (child, status, 0, interruptible);
 }
 
 /* Report whether the subprocess with process id CHILD has changed status.
diff --git a/src/syswait.h b/src/syswait.h
index 846a975..055562a 100644
--- a/src/syswait.h
+++ b/src/syswait.h
@@ -56,7 +56,7 @@ along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.  */
 #endif
 
 /* Defined in sysdep.c.  */
-extern void wait_for_termination (pid_t, int *, bool);
+extern bool wait_for_termination (pid_t, int *, bool);
 extern pid_t child_status_changed (pid_t, int *, int);
 
 #endif /* EMACS_SYSWAIT_H */
-- 
2.7.4

Re: SIGCHLD in vfork child context

[YAMAMOTO Mitsuharu]

>>>>> On Tue, 16 May 2017 01:58:37 -0700, Paul Eggert <eggert@cs.ucla.edu> said:

>> It seems that calling waitpid from the SIGCHLD handler results in
>> ECHILD in the vfork child context

> Why is the vfork child process getting a SIGCHLD signal? That process doesn't 
> have any children. Is the macOS kernel simply buggy? If so, I suggest adding a 
> short explanation of the situation to this comment:

>> +    /* Call the SIGCHLD handler in case we have received SIGCHLD in
>> +       the vfork child context.  The signal mask is restored by
>> +       unblock_child_signal below.  */

I'll make the comment for the first hunk more explicit about the
situation:

#ifdef DARWIN_OS
      /* On Darwin, SIGCHLD for status change of a child process can
	 be delivered to the vfork child context.  It will confuse the
	 SIGCHLD handler because waitpid results in ECHILD there.  */
      signal (SIGCHLD, SIG_DFL);
#endif

>> +    (*action.sa_handler) (SIGCHLD);

> No need for the * and the first set of parens. I.e., this is OK:

>      action.sa_handler (SIGCHLD);

> Otherwise, looks OK; thanks.

Thanks for the comments.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

Re: SIGCHLD in vfork child context

[Paul Eggert]

YAMAMOTO Mitsuharu wrote:
> Thanks for the comments.

You're welcome, but in the meantime Andreas convinced me that we were heading in 
the wrong direction. Please take a look at the patch I just emailed (installed 
in master) and let us know what you think.

Re: SIGCHLD in vfork child context

[YAMAMOTO Mitsuharu]

>>>>> On Fri, 19 May 2017 00:18:33 -0700, Paul Eggert <eggert@cs.ucla.edu> said:

>> Thanks for the comments.

> You're welcome, but in the meantime Andreas convinced me that we
> were heading in the wrong direction. Please take a look at the patch
> I just emailed (installed in master) and let us know what you think.

Emacs no longer hangs with the procedure to reproduce the problem.
Thanks.

I think resetting the SIGCHLD handler in the vfork child on Darwin is
still a good thing to do even with the change you installed, if the
SIGCHLD handler call in such a context is unexpected (and does not
happen on the other platforms).

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

Re: SIGCHLD in vfork child context

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 423 bytes --]

YAMAMOTO Mitsuharu wrote:
> I think resetting the SIGCHLD handler in the vfork child on Darwin is
> still a good thing to do even with the change you installed, if the
> SIGCHLD handler call in such a context is unexpected (and does not
> happen on the other platforms).

Although I'm still not entirely sure we've diagnosed the problem correctly, it 
shouldn't hurt to run that code on macOS, so I installed the attached.

[-- Attachment #2: 0001-Work-around-macOS-bug-with-vforked-child.patch --]
[-- Type: text/x-patch, Size: 928 bytes --]

From 611880e11714007a1b5303eedb89bdfacb0a574a Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sun, 21 May 2017 01:46:44 -0700
Subject: [PATCH] Work around macOS bug with vforked child

* src/callproc.c (call_process) [DARWIN_OS]:
Include workaround for apparent macOS bug.
---
 src/callproc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/src/callproc.c b/src/callproc.c
index 7c85eed..4cec02b 100644
--- a/src/callproc.c
+++ b/src/callproc.c
@@ -631,6 +631,14 @@ call_process (ptrdiff_t nargs, Lisp_Object *args, int filefd,
 
   if (pid == 0)
     {
+#ifdef DARWIN_OS
+      /* Work around a macOS bug, where SIGCHLD is apparently
+	 delivered to a vforked child instead of to its parent.  See:
+	 http://lists.gnu.org/archive/html/emacs-devel/2017-05/msg00342.html
+      */
+      signal (SIGCHLD, SIG_DFL);
+#endif
+
       unblock_child_signal (&oldset);
 
 #ifdef DARWIN_OS
-- 
2.7.4

Re: SIGCHLD in vfork child context

[mituharu]

> YAMAMOTO Mitsuharu wrote:
>> I think resetting the SIGCHLD handler in the vfork child on Darwin is
>> still a good thing to do even with the change you installed, if the
>> SIGCHLD handler call in such a context is unexpected (and does not
>> happen on the other platforms).
>
> Although I'm still not entirely sure we've diagnosed the problem
> correctly, it
> shouldn't hurt to run that code on macOS, so I installed the attached.

Could you install a similar change to process.c, too?

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

Re: SIGCHLD in vfork child context

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 123 bytes --]

mituharu@math.s.chiba-u.ac.jp wrote:
> Could you install a similar change to process.c, too?

Sure, done via the attached.

[-- Attachment #2: 0001-Work-around-macOS-bug-in-create_process-too.patch --]
[-- Type: text/x-patch, Size: 1039 bytes --]

From b1b570410f63e1f204677827530023ffc12218d6 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sun, 21 May 2017 02:00:02 -0700
Subject: [PATCH] Work around macOS bug in create_process, too

* src/process.c (create_process) [DARWIN_OS]:
Reset SIGCHLD after vfork here, too.
---
 src/process.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/src/process.c b/src/process.c
index c301739..2a1c2ee 100644
--- a/src/process.c
+++ b/src/process.c
@@ -2051,11 +2051,16 @@ create_process (Lisp_Object process, char **new_argv, Lisp_Object current_dir)
 
 #ifdef DARWIN_OS
   /* Darwin doesn't let us run setsid after a vfork, so use fork when
-     necessary. */
+     necessary.  Also, reset SIGCHLD handling after a vfork, as
+     apparently macOS can mistakenly deliver SIGCHLD to the child.  */
   if (pty_flag)
     pid = fork ();
   else
-    pid = vfork ();
+    {
+      pid = vfork ();
+      if (pid == 0)
+	signal (SIGCHLD, SIG_DFL);
+    }
 #else
   pid = vfork ();
 #endif
-- 
2.7.4