unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
* bug#12697: 24.2; Emacs crashes when using it as the commit editor for git
@ 2012-10-21 20:17 Matthew Leach
  2012-10-22  8:26 ` Andreas Schwab
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Matthew Leach @ 2012-10-21 20:17 UTC (permalink / raw)
  To: 12697

When I use Emacs as the editor for commit messages in git, by way of
setting the EDITOR environment variable, i.e. "export EDITOR='emacs -nw
'" in my .bashrc, if I press C-g when Emacs opens this causes Emacs to
crash and not shut down correctly as the console is not reset properly
and text from the buffer remains on the screen when I am returned to a
terminal prompt.

I am using git version 1.7.12.4


In GNU Emacs 24.2.1 (x86_64-unknown-linux-gnu, GTK+ Version 3.4.4)
 of 2012-08-28 on shaun
Windowing system distributor `The X.Org Foundation', version 11.0.11300000
Configured using:
 `configure '--prefix=/usr' '--sysconfdir=/etc' '--libexecdir=/usr/lib'
 '--localstatedir=/var' '--with-x-toolkit=gtk3' '--with-xft'
 'CFLAGS=-march=x86-64 -mtune=generic -O2 -pipe -fstack-protector
 --param=ssp-buffer-size=4 -D_FORTIFY_SOURCE=2'
 'LDFLAGS=-Wl,-O1,--sort-common,--as-needed,-z,relro''

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: en_GB.UTF-8
  value of $XMODIFIERS: nil
  locale-coding-system: utf-8-unix
  default enable-multibyte-characters: t

Major mode: Fundamental

Minor modes in effect:
  show-paren-mode: t
  tooltip-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Recent input:
<help-echo> <help-echo> <help-echo> <help-echo> <help-echo> 
<help-echo> <help-echo> <help-echo> <help-echo> <help-echo> 
<help-echo> <help-echo> <help-echo> <help-echo> <help-echo> 
<help-echo> <help-echo> <help-echo> <help-echo> <help-echo> 
<help-echo> <help-echo> <help-echo> <menu-bar> <help-menu> 
<send-emacs-bug-report>

Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.

Load-path shadows:
None found.

Features:
(shadow sort gnus-util mail-extr emacsbug message idna format-spec
rfc822 mml easymenu mml-sec mm-decode mm-bodies mm-encode mail-parse
rfc2231 mailabbrev gmm-utils mailheader sendmail regexp-opt rfc2047
rfc2045 ietf-drums mm-util mail-prsvr mail-utils edmacro kmacro
tango-dark-theme paren ido time-date tooltip ediff-hook vc-hooks
lisp-float-type mwheel x-win x-dnd tool-bar dnd fontset image fringe
lisp-mode register page menu-bar rfn-eshadow timer select scroll-bar
mouse jit-lock font-lock syntax facemenu font-core frame cham georgian
utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao korean
japanese hebrew greek romanian slovak czech european ethiopic indian
cyrillic chinese case-table epa-hook jka-cmpr-hook help simple abbrev
minibuffer loaddefs button faces cus-face files text-properties overlay
sha1 md5 base64 format env code-pages mule custom widget
hashtable-print-readable backquote make-network-process dbusbind
dynamic-setting system-font-setting font-render-setting move-toolbar gtk
x-toolkit x multi-tty emacs)





^ permalink raw reply	[flat|nested] 10+ messages in thread

* bug#12697: 24.2; Emacs crashes when using it as the commit editor for git
  2012-10-21 20:17 bug#12697: 24.2; Emacs crashes when using it as the commit editor for git Matthew Leach
@ 2012-10-22  8:26 ` Andreas Schwab
  2012-10-22 17:15   ` Eli Zaretskii
  2012-10-27  5:31 ` bug#12697: " Paul Eggert
  2012-10-31 17:28 ` Paul Eggert
  2 siblings, 1 reply; 10+ messages in thread
From: Andreas Schwab @ 2012-10-22  8:26 UTC (permalink / raw)
  To: Matthew Leach; +Cc: 12697

Matthew Leach <matthew@mattleach.net> writes:

> When I use Emacs as the editor for commit messages in git, by way of
> setting the EDITOR environment variable, i.e. "export EDITOR='emacs -nw
> '" in my .bashrc, if I press C-g when Emacs opens this causes Emacs to

C-g in emacs -nw is equivalent to SIGINT on the controlling terminal.
This causes all processes in the same process group to receive SIGINT,
which includes the parent git process, now that emacs no longer puts
itself and the terminal into its own process group.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."





^ permalink raw reply	[flat|nested] 10+ messages in thread

* bug#12697: 24.2; Emacs crashes when using it as the commit editor for git
  2012-10-22  8:26 ` Andreas Schwab
@ 2012-10-22 17:15   ` Eli Zaretskii
  2012-10-22 17:39     ` Andreas Schwab
  0 siblings, 1 reply; 10+ messages in thread
From: Eli Zaretskii @ 2012-10-22 17:15 UTC (permalink / raw)
  To: Andreas Schwab; +Cc: matthew, 12697

> From: Andreas Schwab <schwab@linux-m68k.org>
> Date: Mon, 22 Oct 2012 10:26:35 +0200
> Cc: 12697@debbugs.gnu.org
> 
> C-g in emacs -nw is equivalent to SIGINT on the controlling terminal.
> This causes all processes in the same process group to receive SIGINT,
> which includes the parent git process

Shouldn't git block SIGINT when it invokes $EDITOR?

> now that emacs no longer puts itself and the terminal into its own
> process group.

Did we have good reason to stop doing that?





^ permalink raw reply	[flat|nested] 10+ messages in thread

* bug#12697: 24.2; Emacs crashes when using it as the commit editor for git
  2012-10-22 17:15   ` Eli Zaretskii
@ 2012-10-22 17:39     ` Andreas Schwab
  2012-10-22 20:22       ` Eli Zaretskii
  0 siblings, 1 reply; 10+ messages in thread
From: Andreas Schwab @ 2012-10-22 17:39 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: matthew, 12697

Eli Zaretskii <eliz@gnu.org> writes:

>> C-g in emacs -nw is equivalent to SIGINT on the controlling terminal.
>> This causes all processes in the same process group to receive SIGINT,
>> which includes the parent git process
>
> Shouldn't git block SIGINT when it invokes $EDITOR?

Maybe.  Perhaps the OP should report it to the git list.

>> now that emacs no longer puts itself and the terminal into its own
>> process group.
>
> Did we have good reason to stop doing that?

IMHO no.  All the BSD_PGRPS stuff was removed on the premise that it
wasn't used any more, but disabling BSD_PGRPS in s/gnu-linux.h was
already questionable.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."





^ permalink raw reply	[flat|nested] 10+ messages in thread

* bug#12697: 24.2; Emacs crashes when using it as the commit editor for git
  2012-10-22 17:39     ` Andreas Schwab
@ 2012-10-22 20:22       ` Eli Zaretskii
  2012-10-22 21:34         ` Andreas Schwab
  0 siblings, 1 reply; 10+ messages in thread
From: Eli Zaretskii @ 2012-10-22 20:22 UTC (permalink / raw)
  To: Andreas Schwab; +Cc: matthew, 12697

> From: Andreas Schwab <schwab@linux-m68k.org>
> Cc: matthew@mattleach.net,  12697@debbugs.gnu.org
> Date: Mon, 22 Oct 2012 19:39:55 +0200
> 
> >> now that emacs no longer puts itself and the terminal into its own
> >> process group.
> >
> > Did we have good reason to stop doing that?
> 
> IMHO no.  All the BSD_PGRPS stuff was removed on the premise that it
> wasn't used any more, but disabling BSD_PGRPS in s/gnu-linux.h was
> already questionable.

Come think about it, why not stop depending on SIGINT for quitting?
We already do that in GUI session (AFAIU), so why should TTY frames be
different?  We could just detect C-g in the SIGIO handler, no?

If this makes sense, then this whole issue would go away.  Of course,
we will then lose the "emergency exit" feature, but since core dumps
are reportedly disabled on many systems anyway, that doesn't sound
like too much of a loss, and GUI sessions already cannot use it
anyway.





^ permalink raw reply	[flat|nested] 10+ messages in thread

* bug#12697: 24.2; Emacs crashes when using it as the commit editor for git
  2012-10-22 20:22       ` Eli Zaretskii
@ 2012-10-22 21:34         ` Andreas Schwab
  0 siblings, 0 replies; 10+ messages in thread
From: Andreas Schwab @ 2012-10-22 21:34 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: matthew, 12697

Eli Zaretskii <eliz@gnu.org> writes:

> We could just detect C-g in the SIGIO handler, no?

That works already (just disable ISIG in init_sys_modes to try and see).

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."





^ permalink raw reply	[flat|nested] 10+ messages in thread

* bug#12697: Emacs crashes when using it as the commit editor for git
  2012-10-21 20:17 bug#12697: 24.2; Emacs crashes when using it as the commit editor for git Matthew Leach
  2012-10-22  8:26 ` Andreas Schwab
@ 2012-10-27  5:31 ` Paul Eggert
  2012-10-27  7:52   ` Eli Zaretskii
  2012-10-31 17:28 ` Paul Eggert
  2 siblings, 1 reply; 10+ messages in thread
From: Paul Eggert @ 2012-10-27  5:31 UTC (permalink / raw)
  To: 12697; +Cc: Matthew Leach, Andreas Schwab

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

I tried Andreas's suggestion of disabling ISIG, but
this didn't work on Solaris, and it doesn't look like
it'd work on any host where USABLE_SIGIO is false.

Instead, how about the attached patch?  It reenables
the BSD_PGRPS idea, although reimplemented, as the old
code can't easily be resurrected as-is.  I haven't
thought about how this affects the Windows port, so
I'll CC: this to Eli.

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

=== modified file 'src/ChangeLog'
--- src/ChangeLog	2012-10-26 09:46:46 +0000
+++ src/ChangeLog	2012-10-27 05:22:54 +0000
@@ -1,3 +1,24 @@
+2012-10-27  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-26  Eli Zaretskii  <eliz@gnu.org>
 
 	* w32fns.c (w32_wnd_proc) <WM_MOUSEMOVE>: Don't enable tracking of

=== 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-27 05:07:17 +0000
@@ -683,6 +683,70 @@
 }
 
 \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.  */
+static void
+tcsetpgrp_without_stopping (int fd, pid_t pgid)
+{
+  signal_handler_t handler;
+  block_input ();
+  handler = signal (SIGTTOU, SIG_IGN);
+  tcsetpgrp (fd, pgid);
+  signal (SIGTTOU, handler);
+  unblock_input ();
+}
+
+/* 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 +863,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 +1297,7 @@
   dos_ttcooked ();
 #endif
 
+  widen_foreground_group (fileno (tty_out->input));
 }
 \f
 #ifdef HAVE_PTYS


^ permalink raw reply	[flat|nested] 10+ messages in thread

* bug#12697: Emacs crashes when using it as the commit editor for git
  2012-10-27  5:31 ` bug#12697: " Paul Eggert
@ 2012-10-27  7:52   ` Eli Zaretskii
  2012-10-28 18:57     ` Paul Eggert
  0 siblings, 1 reply; 10+ messages in thread
From: Eli Zaretskii @ 2012-10-27  7:52 UTC (permalink / raw)
  To: Paul Eggert; +Cc: matthew, 12697, schwab

> Date: Fri, 26 Oct 2012 22:31:21 -0700
> From: Paul Eggert <eggert@cs.ucla.edu>
> CC: Andreas Schwab <schwab@linux-m68k.org>, Eli Zaretskii <eliz@gnu.org>, 
>  Matthew Leach <matthew@mattleach.net>
> 
> Instead, how about the attached patch?  It reenables
> the BSD_PGRPS idea, although reimplemented, as the old
> code can't easily be resurrected as-is.  I haven't
> thought about how this affects the Windows port

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.

Other than that, this should be OK, I think, since, if setpgid is not
available, it is redirected to setpgrp, which already has a no-op
implementation for Windows.





^ permalink raw reply	[flat|nested] 10+ messages in thread

* bug#12697: Emacs crashes when using it as the commit editor for git
  2012-10-27  7:52   ` Eli Zaretskii
@ 2012-10-28 18:57     ` Paul Eggert
  0 siblings, 0 replies; 10+ messages in thread
From: Paul Eggert @ 2012-10-28 18:57 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: matthew, 12697, schwab

[-- 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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* bug#12697: Emacs crashes when using it as the commit editor for git
  2012-10-21 20:17 bug#12697: 24.2; Emacs crashes when using it as the commit editor for git Matthew Leach
  2012-10-22  8:26 ` Andreas Schwab
  2012-10-27  5:31 ` bug#12697: " Paul Eggert
@ 2012-10-31 17:28 ` Paul Eggert
  2 siblings, 0 replies; 10+ messages in thread
From: Paul Eggert @ 2012-10-31 17:28 UTC (permalink / raw)
  To: 12697-done

I installed the patch into the trunk as bzr 110750
and am marking this as done.





^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2012-10-31 17:28 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-10-21 20:17 bug#12697: 24.2; Emacs crashes when using it as the commit editor for git Matthew Leach
2012-10-22  8:26 ` Andreas Schwab
2012-10-22 17:15   ` Eli Zaretskii
2012-10-22 17:39     ` Andreas Schwab
2012-10-22 20:22       ` Eli Zaretskii
2012-10-22 21:34         ` Andreas Schwab
2012-10-27  5:31 ` bug#12697: " Paul Eggert
2012-10-27  7:52   ` Eli Zaretskii
2012-10-28 18:57     ` Paul Eggert
2012-10-31 17:28 ` Paul Eggert

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).