bug#12697: Emacs crashes when using it as the commit editor for git

[Paul Eggert <eggert@cs.ucla.edu>]

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

On 10/27/2012 12:52 AM, Eli Zaretskii wrote:

> The termios-related stuff (the call to tcsetgrp) in sysdep.c should be
> #ifedf'ed away for Windows (for DOS_NT, actually).  Also, the
> references to SIGTTOU should be conditioned on that signal being
> defined.

Thanks, that problem occurs with older POSIXish systems too.
Here's a simple fix for it.  I'm attaching the resulting
combined patch, relative to trunk bzr 110706.

The bug is a relatively serious one, so I'm inclined to install
the combined patch soon.

=== modified file 'src/sysdep.c'
--- src/sysdep.c	2012-10-27 05:07:17 +0000
+++ src/sysdep.c	2012-10-28 18:39:51 +0000
@@ -716,16 +716,21 @@
 /* Safely set a controlling terminal FD's process group to PGID.
    If we are not in the foreground already, POSIX requires tcsetpgrp
    to deliver a SIGTTOU signal, which would stop us.  This is an
-   annoyance, so temporarily ignore the signal.  */
+   annoyance, so temporarily ignore the signal.
+
+   In practice, platforms lacking SIGTTOU also lack tcsetpgrp, so
+   skip all this unless SIGTTOU is defined.  */
 static void
 tcsetpgrp_without_stopping (int fd, pid_t pgid)
 {
+#ifdef SIGTTOU
   signal_handler_t handler;
   block_input ();
   handler = signal (SIGTTOU, SIG_IGN);
   tcsetpgrp (fd, pgid);
   signal (SIGTTOU, handler);
   unblock_input ();
+#endif
 }
 
 /* Split off the foreground process group to Emacs alone.  When we are



[-- Attachment #2: git-commit.txt --]
[-- Type: text/plain, Size: 8224 bytes --]

=== modified file 'src/ChangeLog'
--- src/ChangeLog	2012-10-28 17:42:52 +0000
+++ src/ChangeLog	2012-10-28 18:33:41 +0000
@@ -1,3 +1,24 @@
+2012-10-28  Paul Eggert  <eggert@cs.ucla.edu>
+
+	Fix crash when using Emacs as commit editor for git (Bug#12697).
+	* callproc.c (setpgrp): Remove macro, as we now use setpgid
+	and it is configured in conf_post.h.
+	(Fcall_process): Don't invoke both setsid and setpgid; the former
+	is enough, if it exists.
+	* callproc.c (Fcall_process, child_setup):
+	* process.c (create_process): Use setpgid.
+	* conf_post.h (setpgid) [!HAVE_SETPGID]: New macro, which substitutes
+	for the real thing.
+	* dispnew.c (init_display): Initialize the foreground group
+	if we are running a tty display.
+	* emacs.c (main): Do not worry about setpgrp; init_display does it now.
+	* lisp.h (init_foreground_group): New decl.
+	* sysdep.c (inherited_pgroup): New static var.
+	(init_foreground_group, tcsetpgrp_without_stopping)
+	(narrow_foreground_group, widen_foreground_group): New functions.
+	(init_sys_modes): Narrow foreground group.
+	(reset_sys_modes): Widen foreground group.
+
 2012-10-28  Eli Zaretskii  <eliz@gnu.org>
 
 	* w32proc.c (TIMER_TICKS_PER_SEC): New macro.

=== modified file 'src/callproc.c'
--- src/callproc.c	2012-10-19 19:25:18 +0000
+++ src/callproc.c	2012-10-27 05:22:54 +0000
@@ -64,13 +64,6 @@
 #include "nsterm.h"
 #endif
 
-#ifdef HAVE_SETPGID
-#if !defined (USG)
-#undef setpgrp
-#define setpgrp setpgid
-#endif
-#endif
-
 /* Pattern used by call-process-region to make temp files.  */
 static Lisp_Object Vtemp_file_name_pattern;
 
@@ -618,14 +611,12 @@
       {
 	if (fd[0] >= 0)
 	  emacs_close (fd[0]);
+
 #ifdef HAVE_SETSID
 	setsid ();
+#else
+	setpgid (0, 0);
 #endif
-#if defined (USG)
-	setpgrp ();
-#else
-	setpgrp (pid, pid);
-#endif /* USG */
 
 	/* Emacs ignores SIGPIPE, but the child should not.  */
 	signal (SIGPIPE, SIG_DFL);
@@ -1295,13 +1286,9 @@
   if (err != in && err != out)
     emacs_close (err);
 
-#if defined (USG)
-#ifndef SETPGRP_RELEASES_CTTY
-  setpgrp ();			/* No arguments but equivalent in this case */
+#if defined HAVE_SETPGID || ! (defined USG && defined SETPGRP_RELEASES_CTTY)
+  setpgid (pid, pid);
 #endif
-#else /* not USG */
-  setpgrp (pid, pid);
-#endif /* not USG */
 
   /* setpgrp_of_tty is incorrect here; it uses input_fd.  */
   tcsetpgrp (0, pid);

=== modified file 'src/conf_post.h'
--- src/conf_post.h	2012-10-19 19:25:18 +0000
+++ src/conf_post.h	2012-10-27 05:07:17 +0000
@@ -112,6 +112,14 @@
 #endif
 /* End of gnulib-related stuff.  */
 
+#ifndef HAVE_SETPGID
+# ifdef USG
+#  define setpgid(pid, pgid) setpgrp ()
+# else
+#  define setpgid(pid, pgid) setpgrp (pid, pgid)
+# endif
+#endif
+
 /* Define one of these for easier conditionals.  */
 #ifdef HAVE_X_WINDOWS
 /* We need a little extra space, see ../../lisp/loadup.el and the

=== modified file 'src/dispnew.c'
--- src/dispnew.c	2012-10-17 19:02:44 +0000
+++ src/dispnew.c	2012-10-27 05:07:17 +0000
@@ -6283,6 +6283,8 @@
     struct terminal *t;
     struct frame *f = XFRAME (selected_frame);
 
+    init_foreground_group ();
+
     /* Open a display on the controlling tty. */
     t = init_tty (0, terminal_type, 1); /* Errors are fatal. */
 

=== modified file 'src/emacs.c'
--- src/emacs.c	2012-10-25 04:35:39 +0000
+++ src/emacs.c	2012-10-27 05:07:17 +0000
@@ -1091,19 +1091,14 @@
 #endif /* DOS_NT */
     }
 
-  if (! noninteractive)
-    {
-#if defined (USG5) && defined (INTERRUPT_INPUT)
-      setpgrp ();
-#endif
 #if defined (HAVE_PTHREAD) && !defined (SYSTEM_MALLOC) && !defined (DOUG_LEA_MALLOC)
-      {
-        extern void malloc_enable_thread (void);
+  if (! noninteractive)
+    {
+      extern void malloc_enable_thread (void);
 
-	malloc_enable_thread ();
-      }
+      malloc_enable_thread ();
+    }
 #endif
-    }
 
   init_signals (dumping);
 

=== modified file 'src/lisp.h'
--- src/lisp.h	2012-10-21 07:23:34 +0000
+++ src/lisp.h	2012-10-27 05:07:17 +0000
@@ -3474,6 +3474,7 @@
 extern char *get_current_dir_name (void);
 #endif
 extern void stuff_char (char c);
+extern void init_foreground_group (void);
 extern void init_sigio (int);
 extern void sys_subshell (void);
 extern void sys_suspend (void);

=== modified file 'src/process.c'
--- src/process.c	2012-10-19 19:25:18 +0000
+++ src/process.c	2012-10-27 05:07:17 +0000
@@ -1759,12 +1759,10 @@
 #endif
 	}
 #else /* not HAVE_SETSID */
-#ifdef USG
-      /* It's very important to call setpgrp here and no time
+      /* It's very important to call setpgid here and no time
 	 afterwards.  Otherwise, we lose our controlling tty which
 	 is set when we open the pty. */
-      setpgrp ();
-#endif /* USG */
+      setpgid (0, 0);
 #endif /* not HAVE_SETSID */
 #if defined (LDISC1)
       if (pty_flag && xforkin >= 0)
@@ -1802,11 +1800,7 @@
 	  /* In order to get a controlling terminal on some versions
 	     of BSD, it is necessary to put the process in pgrp 0
 	     before it opens the terminal.  */
-#ifdef HAVE_SETPGID
 	  setpgid (0, 0);
-#else
-	  setpgrp (0, 0);
-#endif
 #endif
 	}
 #endif /* TIOCNOTTY */

=== modified file 'src/sysdep.c'
--- src/sysdep.c	2012-10-25 04:35:39 +0000
+++ src/sysdep.c	2012-10-28 18:50:00 +0000
@@ -683,6 +683,75 @@
 }
 
 \f
+/* Saving and restoring the process group of Emacs's terminal.  */
+
+/* The process group of which Emacs was a member when it initially
+   started.
+
+   If Emacs was in its own process group (i.e. inherited_pgroup ==
+   getpid ()), then we know we're running under a shell with job
+   control (Emacs would never be run as part of a pipeline).
+   Everything is fine.
+
+   If Emacs was not in its own process group, then we know we're
+   running under a shell (or a caller) that doesn't know how to
+   separate itself from Emacs (like sh).  Emacs must be in its own
+   process group in order to receive SIGIO correctly.  In this
+   situation, we put ourselves in our own pgroup, forcibly set the
+   tty's pgroup to our pgroup, and make sure to restore and reinstate
+   the tty's pgroup just like any other terminal setting.  If
+   inherited_group was not the tty's pgroup, then we'll get a
+   SIGTTmumble when we try to change the tty's pgroup, and a CONT if
+   it goes foreground in the future, which is what should happen.  */
+
+static pid_t inherited_pgroup;
+
+void
+init_foreground_group (void)
+{
+  pid_t pgrp = EMACS_GETPGRP (0);
+  inherited_pgroup = getpid () == pgrp ? 0 : pgrp;
+}
+
+/* Safely set a controlling terminal FD's process group to PGID.
+   If we are not in the foreground already, POSIX requires tcsetpgrp
+   to deliver a SIGTTOU signal, which would stop us.  This is an
+   annoyance, so temporarily ignore the signal.
+
+   In practice, platforms lacking SIGTTOU also lack tcsetpgrp, so
+   skip all this unless SIGTTOU is defined.  */
+static void
+tcsetpgrp_without_stopping (int fd, pid_t pgid)
+{
+#ifdef SIGTTOU
+  signal_handler_t handler;
+  block_input ();
+  handler = signal (SIGTTOU, SIG_IGN);
+  tcsetpgrp (fd, pgid);
+  signal (SIGTTOU, handler);
+  unblock_input ();
+#endif
+}
+
+/* Split off the foreground process group to Emacs alone.  When we are
+   in the foreground, but not started in our own process group,
+   redirect the tty device handle FD to point to our own process
+   group.  FD must be the file descriptor of the controlling tty.  */
+static void
+narrow_foreground_group (int fd)
+{
+  if (inherited_pgroup && setpgid (0, 0) == 0)
+    tcsetpgrp_without_stopping (fd, getpid ());
+}
+
+/* Set the tty to our original foreground group.  */
+static void
+widen_foreground_group (int fd)
+{
+  if (inherited_pgroup && setpgid (0, inherited_pgroup) == 0)
+    tcsetpgrp_without_stopping (fd, inherited_pgroup);
+}
+\f
 /* Getting and setting emacs_tty structures.  */
 
 /* Set *TC to the parameters associated with the terminal FD.
@@ -799,6 +868,8 @@
   if (!tty_out->output)
     return;                     /* The tty is suspended. */
 
+  narrow_foreground_group (fileno (tty_out->input));
+
   if (! tty_out->old_tty)
     tty_out->old_tty = xmalloc (sizeof *tty_out->old_tty);
 
@@ -1231,6 +1302,7 @@
   dos_ttcooked ();
 #endif
 
+  widen_foreground_group (fileno (tty_out->input));
 }
 \f
 #ifdef HAVE_PTYS