bug#554: OSX: with-temp-buffer kills unrelated processes

bug#554: OSX: with-temp-buffer kills unrelated processes

[Markus Triska]

Let w.el consist of the following forms:

 (start-process "bc1" (current-buffer) "bc")

 (with-temp-buffer (start-process "bc2" (current-buffer) "bc"))

When I do "emacs -Q w.el", and press C-M-x on the first form, "bc"
(the GNU arbitrary precision calculator) is started and puts its
start-up message at the end of the buffer (you can also use any other
program that waits for further input). When I then press C-M-x on the
second form, the first process receives SIGHUP, and the message reads:

 "Process bc1 hangup"

I can reproduce this with Emacs 22.2 and latest CVS on OSX. I tried to
trace it in gdb by putting a breakpoint at process_send_signal, but
couldn't reproduce it in the debugger. Otherwise it works every time.

In GNU Emacs 22.2.1 (i386-apple-darwin8.11.1)
 of 2008-05-19 on mt-computer.local
configured using `configure  '--prefix=/opt/local' '--without-x' '--without-carbon' 'CC=/usr/bin/gcc-4.0' 'CFLAGS=-O2' 'LDFLAGS=-L/opt/local/lib' 'CPPFLAGS=-I/opt/local/include' 'CPP=/usr/bin/cpp-4.0''

Important settings:
  value of $LC_ALL: nil
  value of $LC_COLLATE: nil
  value of $LC_CTYPE: nil
  value of $LC_MESSAGES: nil
  value of $LC_MONETARY: nil
  value of $LC_NUMERIC: nil
  value of $LC_TIME: nil
  value of $LANG: nil
  locale-coding-system: nil
  default-enable-multibyte-characters: t

bug#554: OSX: with-temp-buffer kills unrelated processes

[YAMAMOTO Mitsuharu]

>>>>> On Sun, 13 Jul 2008 19:39:57 +0200 (CEST), Markus Triska <markus.triska@gmx.at> said:
--text follows this line--
> Let w.el consist of the following forms:
>  (start-process "bc1" (current-buffer) "bc")

>  (with-temp-buffer (start-process "bc2" (current-buffer) "bc"))

> When I do "emacs -Q w.el", and press C-M-x on the first form, "bc"
> (the GNU arbitrary precision calculator) is started and puts its
> start-up message at the end of the buffer (you can also use any other
> program that waits for further input). When I then press C-M-x on the
> second form, the first process receives SIGHUP, and the message reads:

>  "Process bc1 hangup"

> I can reproduce this with Emacs 22.2 and latest CVS on OSX. I tried to
> trace it in gdb by putting a breakpoint at process_send_signal, but
> couldn't reproduce it in the debugger. Otherwise it works every time.

Just some analysis: not a solution I'm afraid.

It seems that the child process receives SIGHUP before it calls
execvp.  As the child process is still running the Emacs executable,
it sends SIGHUP to all the subprocesses when it receives SIGHUP
(fatal_error_signal -> shut_down_emacs -> kill_buffer_processes).

A similar problem can be found in following example:

  (process-running-child-p (start-process "bc1" (current-buffer) "bc"))
  => t

  (process-running-child-p (prog1 (start-process "bc1" (current-buffer) "bc")
			     (sleep-for 1)))
  => nil

I think these behaviors are not observable in other platforms, because
they use vfork, and the parent side effectively blocks until the child
side calls execvp or others in its typical implementations.  Emacs on
Darwin is using fork instead of vfork because the latter had (or
have?) some problems I'm not familiar with.

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

bug#554: OSX: with-temp-buffer kills unrelated processes

[YAMAMOTO Mitsuharu]

>>>>> On Mon, 14 Jul 2008 19:26:52 +0900, YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp> said:

> It seems that the child process receives SIGHUP before it calls
> execvp.  As the child process is still running the Emacs executable,
> it sends SIGHUP to all the subprocesses when it receives SIGHUP
> (fatal_error_signal -> shut_down_emacs -> kill_buffer_processes).

> I think these behaviors are not observable in other platforms,
> because they use vfork, and the parent side effectively blocks until
> the child side calls execvp or others in its typical
> implementations.  Emacs on Darwin is using fork instead of vfork
> because the latter had (or have?) some problems I'm not familiar
> with.

OK, close-on-exec seems to be usable for the `fork' case.  How about
the patch below?

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

Index: src/callproc.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/callproc.c,v
retrieving revision 1.221.2.3
diff -c -p -r1.221.2.3 callproc.c
*** src/callproc.c	8 Jan 2008 04:30:19 -0000	1.221.2.3
--- src/callproc.c	16 Jul 2008 08:03:57 -0000
*************** static int relocate_fd ();
*** 1192,1199 ****
     Therefore, the superior process must save and restore the value
     of environ around the vfork and the call to this function.
  
!    SET_PGRP is nonzero if we should put the subprocess into a separate
!    process group.
  
     CURRENT_DIR is an elisp string giving the path of the current
     directory the subprocess should have.  Since we can't really signal
--- 1192,1199 ----
     Therefore, the superior process must save and restore the value
     of environ around the vfork and the call to this function.
  
!    RET_ON_FAIL is nonzero if we should return from the function on
!    failure of execvp rather than calling _exit.
  
     CURRENT_DIR is an elisp string giving the path of the current
     directory the subprocess should have.  Since we can't really signal
*************** static int relocate_fd ();
*** 1201,1210 ****
     executable directory by the parent.  */
  
  int
! child_setup (in, out, err, new_argv, set_pgrp, current_dir)
       int in, out, err;
       register char **new_argv;
!      int set_pgrp;
       Lisp_Object current_dir;
  {
    char **env;
--- 1201,1210 ----
     executable directory by the parent.  */
  
  int
! child_setup (in, out, err, new_argv, ret_on_fail, current_dir)
       int in, out, err;
       register char **new_argv;
!      int ret_on_fail;
       Lisp_Object current_dir;
  {
    char **env;
*************** child_setup (in, out, err, new_argv, set
*** 1412,1418 ****
    emacs_write (1, "Can't exec program: ", 20);
    emacs_write (1, new_argv[0], strlen (new_argv[0]));
    emacs_write (1, "\n", 1);
!   _exit (1);
  #endif /* not WINDOWSNT */
  #endif /* not MSDOS */
  }
--- 1412,1419 ----
    emacs_write (1, "Can't exec program: ", 20);
    emacs_write (1, new_argv[0], strlen (new_argv[0]));
    emacs_write (1, "\n", 1);
!   if (!ret_on_fail)
!     _exit (1);
  #endif /* not WINDOWSNT */
  #endif /* not MSDOS */
  }
Index: src/process.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/process.c,v
retrieving revision 1.512.2.12
diff -c -p -r1.512.2.12 process.c
*** src/process.c	27 Feb 2008 15:07:14 -0000	1.512.2.12
--- src/process.c	16 Jul 2008 08:03:58 -0000
*************** create_process (process, new_argv, curre
*** 1842,1847 ****
--- 1842,1850 ----
    int inchannel, outchannel;
    pid_t pid;
    int sv[2];
+ #if !defined (WINDOWSNT) && defined (FD_CLOEXEC)
+   int wait_child_setup[2];
+ #endif
  #ifdef POSIX_SIGNALS
    sigset_t procmask;
    sigset_t blocked;
*************** create_process (process, new_argv, curre
*** 1884,1895 ****
  #endif
        if (forkin < 0)
  	report_file_error ("Opening pty", Qnil);
- #if defined (RTU) || defined (UNIPLUS) || defined (DONT_REOPEN_PTY)
-       /* In the case that vfork is defined as fork, the parent process
- 	 (Emacs) may send some data before the child process completes
- 	 tty options setup.  So we setup tty before forking.  */
-       child_setup_tty (forkout);
- #endif /* RTU or UNIPLUS or DONT_REOPEN_PTY */
  #else
        forkin = forkout = -1;
  #endif /* not USG, or USG_SUBTTY_WORKS */
--- 1887,1892 ----
*************** create_process (process, new_argv, curre
*** 1924,1929 ****
--- 1921,1945 ----
      }
  #endif /* not SKTPAIR */
  
+ #if !defined (WINDOWSNT) && defined (FD_CLOEXEC)
+     {
+       int tem;
+ 
+       tem = pipe (wait_child_setup);
+       if (tem < 0)
+ 	report_file_error ("Creating pipe", Qnil);
+       tem = fcntl (wait_child_setup[1], F_GETFD, 0);
+       if (tem >= 0)
+ 	tem = fcntl (wait_child_setup[1], F_SETFD, tem | FD_CLOEXEC);
+       if (tem < 0)
+ 	{
+ 	  emacs_close (wait_child_setup[0]);
+ 	  emacs_close (wait_child_setup[1]);
+ 	  report_file_error ("Setting file descriptor flags", Qnil);
+ 	}
+     }
+ #endif
+ 
  #if 0
    /* Replaced by close_process_descs */
    set_exclusive_use (inchannel);
*************** create_process (process, new_argv, curre
*** 2174,2189 ****
  #endif /* SIGCHLD */
  #endif /* !POSIX_SIGNALS */
  
- #if !defined (RTU) && !defined (UNIPLUS) && !defined (DONT_REOPEN_PTY)
  	if (pty_flag)
  	  child_setup_tty (xforkout);
- #endif /* not RTU and not UNIPLUS and not DONT_REOPEN_PTY */
  #ifdef WINDOWSNT
  	pid = child_setup (xforkin, xforkout, xforkout,
  			   new_argv, 1, current_dir);
  #else  /* not WINDOWSNT */
  	child_setup (xforkin, xforkout, xforkout,
  		     new_argv, 1, current_dir);
  #endif /* not WINDOWSNT */
        }
      environ = save_environ;
--- 2190,2210 ----
  #endif /* SIGCHLD */
  #endif /* !POSIX_SIGNALS */
  
  	if (pty_flag)
  	  child_setup_tty (xforkout);
  #ifdef WINDOWSNT
  	pid = child_setup (xforkin, xforkout, xforkout,
  			   new_argv, 1, current_dir);
  #else  /* not WINDOWSNT */
+ #ifdef FD_CLOEXEC
+ 	emacs_close (wait_child_setup[0]);
+ #endif
  	child_setup (xforkin, xforkout, xforkout,
  		     new_argv, 1, current_dir);
+ #ifdef FD_CLOEXEC
+ 	emacs_close (wait_child_setup[1]);
+ #endif
+ 	_exit (1);
  #endif /* not WINDOWSNT */
        }
      environ = save_environ;
*************** create_process (process, new_argv, curre
*** 2235,2240 ****
--- 2256,2276 ----
        else
  #endif
  	XPROCESS (process)->tty_name = Qnil;
+ 
+ #if !defined (WINDOWSNT) && defined (FD_CLOEXEC)
+       /* Wait for child_setup to complete in case that vfork is
+ 	 actually defined as fork.  The descriptor wait_child_setup[1]
+ 	 of a pipe is closed at the child side either by close-on-exec
+ 	 on successful execvp or by the explicit emacs_close call
+ 	 before _exit above.  */
+       {
+ 	char dummy;
+ 
+ 	emacs_close (wait_child_setup[1]);
+ 	emacs_read (wait_child_setup[0], &dummy, 1);
+ 	emacs_close (wait_child_setup[0]);
+       }
+ #endif
      }
  
    /* Restore the signal state whether vfork succeeded or not.

bug#554: OSX: with-temp-buffer kills unrelated processes

[Markus Triska]

YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp> writes:

> OK, close-on-exec seems to be usable for the `fork' case.  How about
> the patch below?

Thank you very much; I've tested the following adapted version of your
patch with the latest CVS trunk of Emacs, and the problem seems gone.

All the best,
Markus


diff --git a/src/callproc.c b/src/callproc.c
index a6de766..3d61e64 100644
--- a/src/callproc.c
+++ b/src/callproc.c
@@ -1047,8 +1047,8 @@ add_env (char **env, char **new_env, char *string)
    Therefore, the superior process must save and restore the value
    of environ around the vfork and the call to this function.
 
-   SET_PGRP is nonzero if we should put the subprocess into a separate
-   process group.
+   RET_ON_FAIL is nonzero if we should return from the function on
+   failure of execvp rather than calling _exit.
 
    CURRENT_DIR is an elisp string giving the path of the current
    directory the subprocess should have.  Since we can't really signal
@@ -1056,10 +1056,10 @@ add_env (char **env, char **new_env, char *string)
    executable directory by the parent.  */
 
 int
-child_setup (in, out, err, new_argv, set_pgrp, current_dir)
+child_setup (in, out, err, new_argv, ret_on_fail, current_dir)
      int in, out, err;
      register char **new_argv;
-     int set_pgrp;
+     int ret_on_fail;
      Lisp_Object current_dir;
 {
   char **env;
@@ -1291,7 +1291,8 @@ child_setup (in, out, err, new_argv, set_pgrp, current_dir)
   emacs_write (1, "Can't exec program: ", 20);
   emacs_write (1, new_argv[0], strlen (new_argv[0]));
   emacs_write (1, "\n", 1);
-  _exit (1);
+  if (!ret_on_fail)
+    _exit (1);
 #endif /* not WINDOWSNT */
 #endif /* not MSDOS */
 }
diff --git a/src/process.c b/src/process.c
index c82584a..b0bebeb 100644
--- a/src/process.c
+++ b/src/process.c
@@ -1851,6 +1851,9 @@ create_process (process, new_argv, current_dir)
   int inchannel, outchannel;
   pid_t pid;
   int sv[2];
+#if !defined (WINDOWSNT) && defined (FD_CLOEXEC)
+  int wait_child_setup[2];
+#endif
 #ifdef POSIX_SIGNALS
   sigset_t procmask;
   sigset_t blocked;
@@ -1893,12 +1896,6 @@ create_process (process, new_argv, current_dir)
 #endif
       if (forkin < 0)
 	report_file_error ("Opening pty", Qnil);
-#if defined (DONT_REOPEN_PTY)
-      /* In the case that vfork is defined as fork, the parent process
-	 (Emacs) may send some data before the child process completes
-	 tty options setup.  So we setup tty before forking.  */
-      child_setup_tty (forkout);
-#endif /* DONT_REOPEN_PTY */
 #else
       forkin = forkout = -1;
 #endif /* not USG, or USG_SUBTTY_WORKS */
@@ -1924,6 +1921,25 @@ create_process (process, new_argv, current_dir)
       forkin = sv[0];
     }
 
+#if !defined (WINDOWSNT) && defined (FD_CLOEXEC)
+    {
+      int tem;
+
+      tem = pipe (wait_child_setup);
+      if (tem < 0)
+	report_file_error ("Creating pipe", Qnil);
+      tem = fcntl (wait_child_setup[1], F_GETFD, 0);
+      if (tem >= 0)
+	tem = fcntl (wait_child_setup[1], F_SETFD, tem | FD_CLOEXEC);
+      if (tem < 0)
+	{
+	  emacs_close (wait_child_setup[0]);
+	  emacs_close (wait_child_setup[1]);
+	  report_file_error ("Setting file descriptor flags", Qnil);
+	}
+    }
+#endif
+
 #if 0
   /* Replaced by close_process_descs */
   set_exclusive_use (inchannel);
@@ -2150,16 +2166,21 @@ create_process (process, new_argv, current_dir)
 #endif /* SIGCHLD */
 #endif /* !POSIX_SIGNALS */
 
-#if !defined (DONT_REOPEN_PTY)
 	if (pty_flag)
 	  child_setup_tty (xforkout);
-#endif /* not DONT_REOPEN_PTY */
 #ifdef WINDOWSNT
 	pid = child_setup (xforkin, xforkout, xforkout,
 			   new_argv, 1, current_dir);
 #else  /* not WINDOWSNT */
+#ifdef FD_CLOEXEC
+	emacs_close (wait_child_setup[0]);
+#endif
 	child_setup (xforkin, xforkout, xforkout,
 		     new_argv, 1, current_dir);
+#ifdef FD_CLOEXEC
+	emacs_close (wait_child_setup[0]);
+#endif
+	_exit (1);
 #endif /* not WINDOWSNT */
       }
     environ = save_environ;
@@ -2211,6 +2232,21 @@ create_process (process, new_argv, current_dir)
       else
 #endif
 	XPROCESS (process)->tty_name = Qnil;
+
+#if !defined (WINDOWSNT) && defined (FD_CLOEXEC)
+      /* Wait for child_setup to complete in case that vfork is
+	 actually defined as fork.  The descriptor wait_child_setup[1]
+	 of a pipe is closed at the child side either by close-on-exec
+	 on successful execvp or by the explicit emacs_close call
+	 before _exit above.  */
+      {
+	char dummy;
+
+	emacs_close (wait_child_setup[1]);
+	emacs_read (wait_child_setup[0], &dummy, 1);
+	emacs_close (wait_child_setup[0]);
+      }
+#endif
     }
 
   /* Restore the signal state whether vfork succeeded or not.

bug#554: OSX: with-temp-buffer kills unrelated processes

[YAMAMOTO Mitsuharu]

>>>>> On Wed, 16 Jul 2008 17:27:47 +0200, Markus Triska <markus.triska@gmx.at> said:

> YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp> writes:
>> OK, close-on-exec seems to be usable for the `fork' case.  How
>> about the patch below?

> Thank you very much; I've tested the following adapted version of
> your patch with the latest CVS trunk of Emacs, and the problem seems
> gone.

Thanks for testing.

Although whether _exit closes file descriptors or not is
implementation-dependent, the current Emacs code seems to assume that
it always does (in its supported platforms).  If so, close before
_exit and the changes to callproc.c in my previous patch can be
removed.  I'll install a simplified one later.

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