bug#8855: dbus error at startup

bug#8855: dbus error at startup

[Dan Nicolaescu]

This is with emacs from trunk, using the Lucid toolkit.

When doing:

ssh A_LOCAL_USER@localhost
emacs -Q

This error is shown:

(process:2012): GLib-WARNING **: In call to g_spawn_sync(), exit status of a child process was requested but SIGCHLD action was set to SIG_IGN and ECHILD was received by waitpid(), so exit status can't be returned. This is a bug in the program calling g_spawn_sync(); either don't request the exit status, or don't set the SIGCHLD action.
GConf Error: Failed to contact configuration server; the most common cause is a missing or misconfigured D-Bus session bus daemon. See http://projects.gnome.org/gconf/ for information. (Details -  1: Failed to get connection to session: Abnormal program termination spawning command line `dbus-launch --autolaunch=5ff056e06a6f310f5110670047532bd4 --binary-syntax --close-stderr': )


then emacs starts just fine, and I can see the "dbus-launch" process with that command line is running too.
Is emacs not waiting long enough for that dbus process to start, or is there
something else going on here?

bug#8855: dbus error at startup

[Michael Albinus]

Dan Nicolaescu <dann@gnu.org> writes:

> Is emacs not waiting long enough for that dbus process to start, or is there
> something else going on here?

It does not seem to be an error in Emacs, but in GIO. See discussion in
<https://bugzilla.redhat.com/show_bug.cgi?id=654027>.

I have no idea which SIGCHLD setting we shall apply, 'though.

Best regards, Michael.

bug#8855: dbus error at startup

[Paul Eggert]

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

I ran into this bug today, and have verified that it's not
a GIO bug per se, but is a bug in the interaction between
Emacs and glib, which causes Emacs to mess up glib.
Patch attached.

[-- Attachment #2: g_spawn_sync.txt --]
[-- Type: text/plain, Size: 12400 bytes --]

=== modified file 'src/ChangeLog'
--- src/ChangeLog	2012-10-25 04:35:39 +0000
+++ src/ChangeLog	2012-10-25 05:29:40 +0000
@@ -1,5 +1,38 @@
 2012-10-25  Paul Eggert  <eggert@cs.ucla.edu>
 
+	Fix a race condition that causes Emacs to mess up glib (Bug#8855).
+	The symptom is a diagnostic "GLib-WARNING **: In call to
+	g_spawn_sync(), exit status of a child process was requested but
+	SIGCHLD action was set to SIG_IGN and ECHILD was received by
+	waitpid(), so exit status can't be returned."  The diagnostic
+	is partly wrong, as the SIGCHLD action is not set to SIG_IGN.
+	The real bug is a race condition between Emacs and glib: Emacs
+	does a waitpid (-1, ...) and reaps glib's subprocess by mistake,
+	so that glib can't find it.  Work around the bug by invoking
+	waitpid only on subprocesses that Emacs itself creates.
+	Perhaps this bug fix can be made more efficient by invoking
+	waitpid once on a process group instead of invoking it on each
+	known child, but at least this code fixes the bug.
+	* process.c (deleted_pid_list, Fdelete_process, create_process)
+	(wait_reading_process_output, Fsignal_process)
+	(record_child_status_change, handle_child_signal)
+	(deliver_child_signal, init_process_emacs, syms_of_process):
+	* sysdep.c (emacs_sigaction_init, init_signals):
+	Assume SIGCHLD is defined.  This is true on all Emacs porting
+	targets nowadays, and the code no longer works otherwise.
+	* process.c (create_process, record_child_status_change):
+	Don't use special value -1 in pid field, as the caller now must
+	know the pid rather than having the callee infer it.  The
+	inference was sometimes incorrect anyway, due to another race.
+	(process_status_retrieved): New function.
+	(record_child_status_change): Use it.  Return bool.
+	Accept negative 1st argument, which means to wait for one of
+	the processes that Emacs already knows about.
+	(handle_child_signal): Let record_child_status_change do all
+	the work, since we do not want to reap all exited child processes,
+	only the child processes that Emacs itself created.
+	* syswait.h: Adjust to above API changes.
+
 	Don't assume process IDs fit in int.
 	* emacs.c (shut_down_emacs) [!DOS_NT]:
 	* sysdep.c (sys_suspend) [SIGTSTP && !MSDOS]:

=== modified file 'src/process.c'
--- src/process.c	2012-10-19 19:25:18 +0000
+++ src/process.c	2012-10-25 05:29:40 +0000
@@ -792,14 +792,11 @@
 }
 
 
-#ifdef SIGCHLD
 /* Fdelete_process promises to immediately forget about the process, but in
    reality, Emacs needs to remember those processes until they have been
-   treated by the SIGCHLD handler; otherwise this handler would consider the
-   process as being synchronous and say that the synchronous process is
-   dead.  */
+   treated by the SIGCHLD handler and waitpid has been invoked on them;
+   otherwise they might fill up the kernel's process table.  */
 static Lisp_Object deleted_pid_list;
-#endif
 
 DEFUN ("delete-process", Fdelete_process, Sdelete_process, 1, 1, 0,
        doc: /* Delete PROCESS: kill it and forget about it immediately.
@@ -822,7 +819,6 @@
     }
   else if (p->infd >= 0)
     {
-#ifdef SIGCHLD
       Lisp_Object symbol;
       pid_t pid = p->pid;
 
@@ -840,7 +836,6 @@
 	deleted_pid_list
 	  = Fdelete (make_fixnum_or_float (pid), deleted_pid_list);
       else
-#endif
 	{
 	  Fkill_process (process, Qnil);
 	  /* Do this now, since remove_process will make the
@@ -1598,9 +1593,7 @@
 #if !defined (WINDOWSNT) && defined (FD_CLOEXEC)
   int wait_child_setup[2];
 #endif
-#ifdef SIGCHLD
   sigset_t blocked;
-#endif
   /* Use volatile to protect variables from being clobbered by vfork.  */
   volatile int forkin, forkout;
   volatile int pty_flag = 0;
@@ -1704,29 +1697,18 @@
   if (inchannel > max_process_desc)
     max_process_desc = inchannel;
 
-  /* Until we store the proper pid, enable the SIGCHLD handler
-     to recognize an unknown pid as standing for this process.
-     It is very important not to let this `marker' value stay
-     in the table after this function has returned; if it does
-     it might cause call-process to hang and subsequent asynchronous
-     processes to get their return values scrambled.  */
-  XPROCESS (process)->pid = -1;
-
-  /* This must be called after the above line because it may signal an
-     error. */
+  /* This may signal an error. */
   setup_process_coding_systems (process);
 
   encoded_current_dir = ENCODE_FILE (current_dir);
 
   block_input ();
 
-#ifdef SIGCHLD
   /* Block SIGCHLD until we have a chance to store the new fork's
      pid in its process structure.  */
   sigemptyset (&blocked);
   sigaddset (&blocked, SIGCHLD);
   pthread_sigmask (SIG_BLOCK, &blocked, 0);
-#endif
 
 #if HAVE_WORKING_VFORK
   /* child_setup must clobber environ on systems with true vfork.
@@ -1860,10 +1842,8 @@
       /* Emacs ignores SIGPIPE, but the child should not.  */
       signal (SIGPIPE, SIG_DFL);
 
-#ifdef SIGCHLD
 	/* Stop blocking signals in the child.  */
       pthread_sigmask (SIG_SETMASK, &empty_mask, 0);
-#endif
 
       if (pty_flag)
 	child_setup_tty (xforkout);
@@ -1888,9 +1868,7 @@
   XPROCESS (process)->pid = pid;
 
   /* Stop blocking signals in the parent.  */
-#ifdef SIGCHLD
   pthread_sigmask (SIG_SETMASK, &empty_mask, 0);
-#endif
   unblock_input ();
 
   if (pid < 0)
@@ -4923,11 +4901,7 @@
 #endif /* HAVE_PTYS */
 	      /* If we can detect process termination, don't consider the
 		 process gone just because its pipe is closed.  */
-#ifdef SIGCHLD
-	      else if (nread == 0 && !NETCONN_P (proc) && !SERIALCONN_P (proc))
-		;
-#endif
-	      else
+	      else if (nread != 0 || NETCONN_P (proc) || SERIALCONN_P (proc))
 		{
 		  /* Preserve status of processes already terminated.  */
 		  XPROCESS (proc)->tick = ++process_tick;
@@ -6161,9 +6135,7 @@
 #ifdef SIGCONT
       parse_signal ("cont", SIGCONT);
 #endif
-#ifdef SIGCHLD
       parse_signal ("chld", SIGCHLD);
-#endif
 #ifdef SIGTTIN
       parse_signal ("ttin", SIGTTIN);
 #endif
@@ -6279,9 +6251,30 @@
   return process;
 }
 \f
-/* On receipt of a signal that a child status has changed, loop asking
-   about children with changed statuses until the system says there
-   are no more.
+/* If the status of the process DESIRED has changed, return true and
+   set *STATUS to its exit status; otherwise, return false.
+   If HAVE is nonnegative, assume that HAVE = waitpid (HAVE, STATUS, ...)
+   has already been invoked, and do not invoke waitpid again.  */
+
+static bool
+process_status_retrieved (pid_t desired, pid_t have, int *status)
+{
+  if (have < 0)
+    {
+      do
+	have = waitpid (desired, status, WNOHANG | WUNTRACED);
+      while (have < 0 && errno == EINTR);
+    }
+
+  return have == desired;
+}
+
+/* If PID is nonnegative, the child process PID with wait status W has
+   changed its status; record this and return true.
+
+   If PID is negative, ignore W, and look for a known child process
+   of Emacs whose status has changed.  If one is found, record its new
+   status and return true; otherwise, return false.
 
    All we do is change the status; we do not run sentinels or print
    notifications.  That is saved for the next time keyboard input is
@@ -6304,11 +6297,9 @@
    ** Malloc WARNING: This should never call malloc either directly or
    indirectly; if it does, that is a bug  */
 
-/* Record the changed status of the child process PID with wait status W.  */
-void
+bool
 record_child_status_change (pid_t pid, int w)
 {
-#ifdef SIGCHLD
   Lisp_Object proc;
   struct Lisp_Process *p;
   Lisp_Object tail;
@@ -6319,11 +6310,18 @@
   for (tail = deleted_pid_list; CONSP (tail); tail = XCDR (tail))
     {
       Lisp_Object xpid = XCAR (tail);
-      if ((INTEGERP (xpid) && pid == XINT (xpid))
-	  || (FLOATP (xpid) && pid == XFLOAT_DATA (xpid)))
+      bool all_pids_are_fixnums
+	= (MOST_NEGATIVE_FIXNUM <= TYPE_MINIMUM (pid_t)
+	   && TYPE_MAXIMUM (pid_t) <= MOST_POSITIVE_FIXNUM);
+      pid_t deleted_pid;
+      if (all_pids_are_fixnums || INTEGERP (xpid))
+	deleted_pid = XINT (xpid);
+      else
+	deleted_pid = XFLOAT_DATA (xpid);
+      if (process_status_retrieved (deleted_pid, pid, &w))
 	{
 	  XSETCAR (tail, Qnil);
-	  return;
+	  return 1;
 	}
     }
 
@@ -6333,23 +6331,11 @@
     {
       proc = XCDR (XCAR (tail));
       p = XPROCESS (proc);
-      if (EQ (p->type, Qreal) && p->pid == pid)
+      if (EQ (p->type, Qreal) && process_status_retrieved (p->pid, pid, &w))
 	break;
       p = 0;
     }
 
-  /* Look for an asynchronous process whose pid hasn't been filled
-     in yet.  */
-  if (! p)
-    for (tail = Vprocess_alist; CONSP (tail); tail = XCDR (tail))
-      {
-	proc = XCDR (XCAR (tail));
-	p = XPROCESS (proc);
-	if (p->pid == -1)
-	  break;
-	p = 0;
-      }
-
   /* Change the status of the process that was found.  */
   if (p)
     {
@@ -6375,11 +6361,14 @@
 	 look around.  */
       if (input_available_clear_time)
 	*input_available_clear_time = make_emacs_time (0, 0);
+
+      return 1;
     }
-  /* There was no asynchronous process found for that pid: we have
-     a synchronous process.  */
-  else
+  else if (0 <= pid)
     {
+      /* The caller successfully waited for a pid but no asynchronous
+	 process was found for it, so this is a synchronous process.  */
+
       synch_process_alive = 0;
 
       /* Report the status of the synchronous process.  */
@@ -6392,11 +6381,13 @@
 	 look around.  */
       if (input_available_clear_time)
 	*input_available_clear_time = make_emacs_time (0, 0);
+
+      return 1;
     }
-#endif
+
+  return 0;
 }
 
-#ifdef SIGCHLD
 
 /* On some systems, the SIGCHLD handler must return right away.  If
    any more processes want to signal us, we will get another signal.
@@ -6413,23 +6404,8 @@
 static void
 handle_child_signal (int sig)
 {
-  do
-    {
-      pid_t pid;
-      int status;
-
-      do
-	pid = waitpid (-1, &status, WNOHANG | WUNTRACED);
-      while (pid < 0 && errno == EINTR);
-
-      /* PID == 0 means no processes found, PID == -1 means a real failure.
-	 Either way, we have done all our job.  */
-      if (pid <= 0)
-	break;
-
-      record_child_status_change (pid, status);
-    }
-  while (CAN_HANDLE_MULTIPLE_CHILDREN);
+  while (record_child_status_change (-1, 0) && CAN_HANDLE_MULTIPLE_CHILDREN)
+    continue;
 }
 
 static void
@@ -6438,7 +6414,6 @@
   deliver_process_signal (sig, handle_child_signal);
 }
 
-#endif /* SIGCHLD */
 \f
 
 static Lisp_Object
@@ -7287,7 +7262,6 @@
 
   inhibit_sentinels = 0;
 
-#ifdef SIGCHLD
 #ifndef CANNOT_DUMP
   if (! noninteractive || initialized)
 #endif
@@ -7296,7 +7270,6 @@
       emacs_sigaction_init (&action, deliver_child_signal);
       sigaction (SIGCHLD, &action, 0);
     }
-#endif
 
   FD_ZERO (&input_wait_mask);
   FD_ZERO (&non_keyboard_wait_mask);
@@ -7323,9 +7296,7 @@
 #endif
 
   Vprocess_alist = Qnil;
-#ifdef SIGCHLD
   deleted_pid_list = Qnil;
-#endif
   for (i = 0; i < MAXDESC; i++)
     {
       chan_process[i] = Qnil;
@@ -7454,9 +7425,7 @@
   DEFSYM (Qlast_nonmenu_event, "last-nonmenu-event");
 
   staticpro (&Vprocess_alist);
-#ifdef SIGCHLD
   staticpro (&deleted_pid_list);
-#endif
 
 #endif	/* subprocesses */
 

=== modified file 'src/sysdep.c'
--- src/sysdep.c	2012-10-25 04:35:39 +0000
+++ src/sysdep.c	2012-10-25 05:29:40 +0000
@@ -1444,9 +1444,7 @@
   /* When handling a signal, block nonfatal system signals that are caught
      by Emacs.  This makes race conditions less likely.  */
   sigaddset (&action->sa_mask, SIGALRM);
-#ifdef SIGCHLD
   sigaddset (&action->sa_mask, SIGCHLD);
-#endif
 #ifdef SIGDANGER
   sigaddset (&action->sa_mask, SIGDANGER);
 #endif
@@ -1635,9 +1633,7 @@
 # ifdef SIGCLD
       sys_siglist[SIGCLD] = "Child status changed";
 # endif
-# ifdef SIGCHLD
       sys_siglist[SIGCHLD] = "Child status changed";
-# endif
 # ifdef SIGCONT
       sys_siglist[SIGCONT] = "Continued";
 # endif

=== modified file 'src/syswait.h'
--- src/syswait.h	2012-09-23 22:25:22 +0000
+++ src/syswait.h	2012-10-25 05:29:40 +0000
@@ -23,6 +23,7 @@
 #ifndef EMACS_SYSWAIT_H
 #define EMACS_SYSWAIT_H
 
+#include <stdbool.h>
 #include <sys/types.h>
 
 #ifdef HAVE_SYS_WAIT_H	/* We have sys/wait.h with POSIXoid definitions. */
@@ -52,7 +53,7 @@
 #endif
 
 /* Defined in process.c.  */
-extern void record_child_status_change (pid_t, int);
+extern bool record_child_status_change (pid_t, int);
 
 /* Defined in sysdep.c.  */
 extern void wait_for_termination (pid_t);

bug#8855: dbus error at startup

[Eli Zaretskii]

> Date: Wed, 24 Oct 2012 22:35:15 -0700
> From: Paul Eggert <eggert@cs.ucla.edu>
> 
> I ran into this bug today, and have verified that it's not
> a GIO bug per se, but is a bug in the interaction between
> Emacs and glib, which causes Emacs to mess up glib.
> Patch attached.

Thanks, but is there perhaps a way to fix this less intrusively,
without messing with the whole chain of functions involved in SIGCHLD
and subprocess termination?

bug#8855: dbus error at startup

[Paul Eggert]

On 10/25/2012 09:24 AM, Eli Zaretskii wrote:
> is there perhaps a way to fix this less intrusively,
> without messing with the whole chain of functions involved in SIGCHLD
> and subprocess termination?

I looked for one but couldn't find one.

We could try to get the glib API fixed, but I can't offhand
think of an easy fix, and in the meantime we need to deal
with the glibs in the field with the current API, and for
those glibs we need a workaround anyway.

We could try to use process groups, so that Emacs invokes
waitpid just on a process group rather than on all children.
However, an executable can create a new process group and
join it so this would require that we create two processes
instead of one (the child simply forks and waits for the
grandchild which does the real work, so that any mucking
around by the grandchild cannot affect the child's process
group).  This would work, but it costs us one extra process
per subprocess, and I expect that in practice that'd be
worse than the proposed patch, both performance-wise and
intrusive-change-wise.

glib itself, when faced with this problem, does not use
process groups, but simply scans for all subprocesses that
it knows about, just as the proposed patch does.  See the
function dispatch_unix_signals in glib/gmain.c.

Looking at that function's callers, it may be that we can
use g_child_watch_source_new instead of waitpid, but it's
not clear to me how that would work, and I wonder whether the
result would be any less intrusive than the proposed patch.

bug#8855: dbus error at startup

[Paul Eggert]

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

Attached is a slightly less-intrusive version of the patch.
It omits the removal of "#ifdef SIGCHLD", which isn't
strictly needed to fix the bug.  I don't see any further
simplifications, unfortunately.  The patch fixes a bug that's
been reported multiple times so I'm thinking it may be
worthwhile to install now, even though there's a feature
freeze.


[-- Attachment #2: g_spawn_sync.txt --]
[-- Type: text/plain, Size: 7849 bytes --]

=== modified file 'src/ChangeLog'
--- src/ChangeLog	2012-10-28 17:42:52 +0000
+++ src/ChangeLog	2012-10-29 01:25:53 +0000
@@ -1,3 +1,31 @@
+2012-10-28  Paul Eggert  <eggert@cs.ucla.edu>
+
+	Fix a race condition that causes Emacs to mess up glib (Bug#8855).
+	The symptom is a diagnostic "GLib-WARNING **: In call to
+	g_spawn_sync(), exit status of a child process was requested but
+	SIGCHLD action was set to SIG_IGN and ECHILD was received by
+	waitpid(), so exit status can't be returned."  The diagnostic
+	is partly wrong, as the SIGCHLD action is not set to SIG_IGN.
+	The real bug is a race condition between Emacs and glib: Emacs
+	does a waitpid (-1, ...) and reaps glib's subprocess by mistake,
+	so that glib can't find it.  Work around the bug by invoking
+	waitpid only on subprocesses that Emacs itself creates.
+	Perhaps this bug fix can be made more efficient by invoking
+	waitpid once on a process group instead of invoking it on each
+	known child, but at least this code fixes the bug.
+	* process.c (create_process, record_child_status_change):
+	Don't use special value -1 in pid field, as the caller now must
+	know the pid rather than having the callee infer it.  The
+	inference was sometimes incorrect anyway, due to another race.
+	(process_status_retrieved): New function.
+	(record_child_status_change): Use it.  Return bool.
+	Accept negative 1st argument, which means to wait for one of
+	the processes that Emacs already knows about.
+	(handle_child_signal): Let record_child_status_change do all
+	the work, since we do not want to reap all exited child processes,
+	only the child processes that Emacs itself created.
+	* syswait.h: Adjust to above API changes.
+
 2012-10-28  Eli Zaretskii  <eliz@gnu.org>
 
 	* w32proc.c (TIMER_TICKS_PER_SEC): New macro.

=== modified file 'src/process.c'
--- src/process.c	2012-10-19 19:25:18 +0000
+++ src/process.c	2012-10-29 01:25:53 +0000
@@ -795,9 +795,8 @@
 #ifdef SIGCHLD
 /* Fdelete_process promises to immediately forget about the process, but in
    reality, Emacs needs to remember those processes until they have been
-   treated by the SIGCHLD handler; otherwise this handler would consider the
-   process as being synchronous and say that the synchronous process is
-   dead.  */
+   treated by the SIGCHLD handler and waitpid has been invoked on them;
+   otherwise they might fill up the kernel's process table.  */
 static Lisp_Object deleted_pid_list;
 #endif
 
@@ -1704,16 +1703,7 @@
   if (inchannel > max_process_desc)
     max_process_desc = inchannel;
 
-  /* Until we store the proper pid, enable the SIGCHLD handler
-     to recognize an unknown pid as standing for this process.
-     It is very important not to let this `marker' value stay
-     in the table after this function has returned; if it does
-     it might cause call-process to hang and subsequent asynchronous
-     processes to get their return values scrambled.  */
-  XPROCESS (process)->pid = -1;
-
-  /* This must be called after the above line because it may signal an
-     error. */
+  /* This may signal an error. */
   setup_process_coding_systems (process);
 
   encoded_current_dir = ENCODE_FILE (current_dir);
@@ -6279,9 +6269,30 @@
   return process;
 }
 \f
-/* On receipt of a signal that a child status has changed, loop asking
-   about children with changed statuses until the system says there
-   are no more.
+/* If the status of the process DESIRED has changed, return true and
+   set *STATUS to its exit status; otherwise, return false.
+   If HAVE is nonnegative, assume that HAVE = waitpid (HAVE, STATUS, ...)
+   has already been invoked, and do not invoke waitpid again.  */
+
+static bool
+process_status_retrieved (pid_t desired, pid_t have, int *status)
+{
+  if (have < 0)
+    {
+      do
+	have = waitpid (desired, status, WNOHANG | WUNTRACED);
+      while (have < 0 && errno == EINTR);
+    }
+
+  return have == desired;
+}
+
+/* If PID is nonnegative, the child process PID with wait status W has
+   changed its status; record this and return true.
+
+   If PID is negative, ignore W, and look for a known child process
+   of Emacs whose status has changed.  If one is found, record its new
+   status and return true; otherwise, return false.
 
    All we do is change the status; we do not run sentinels or print
    notifications.  That is saved for the next time keyboard input is
@@ -6304,8 +6315,7 @@
    ** Malloc WARNING: This should never call malloc either directly or
    indirectly; if it does, that is a bug  */
 
-/* Record the changed status of the child process PID with wait status W.  */
-void
+bool
 record_child_status_change (pid_t pid, int w)
 {
 #ifdef SIGCHLD
@@ -6319,11 +6329,18 @@
   for (tail = deleted_pid_list; CONSP (tail); tail = XCDR (tail))
     {
       Lisp_Object xpid = XCAR (tail);
-      if ((INTEGERP (xpid) && pid == XINT (xpid))
-	  || (FLOATP (xpid) && pid == XFLOAT_DATA (xpid)))
+      bool all_pids_are_fixnums
+	= (MOST_NEGATIVE_FIXNUM <= TYPE_MINIMUM (pid_t)
+	   && TYPE_MAXIMUM (pid_t) <= MOST_POSITIVE_FIXNUM);
+      pid_t deleted_pid;
+      if (all_pids_are_fixnums || INTEGERP (xpid))
+	deleted_pid = XINT (xpid);
+      else
+	deleted_pid = XFLOAT_DATA (xpid);
+      if (process_status_retrieved (deleted_pid, pid, &w))
 	{
 	  XSETCAR (tail, Qnil);
-	  return;
+	  return 1;
 	}
     }
 
@@ -6333,23 +6350,11 @@
     {
       proc = XCDR (XCAR (tail));
       p = XPROCESS (proc);
-      if (EQ (p->type, Qreal) && p->pid == pid)
+      if (EQ (p->type, Qreal) && process_status_retrieved (p->pid, pid, &w))
 	break;
       p = 0;
     }
 
-  /* Look for an asynchronous process whose pid hasn't been filled
-     in yet.  */
-  if (! p)
-    for (tail = Vprocess_alist; CONSP (tail); tail = XCDR (tail))
-      {
-	proc = XCDR (XCAR (tail));
-	p = XPROCESS (proc);
-	if (p->pid == -1)
-	  break;
-	p = 0;
-      }
-
   /* Change the status of the process that was found.  */
   if (p)
     {
@@ -6375,11 +6380,14 @@
 	 look around.  */
       if (input_available_clear_time)
 	*input_available_clear_time = make_emacs_time (0, 0);
+
+      return 1;
     }
-  /* There was no asynchronous process found for that pid: we have
-     a synchronous process.  */
-  else
+  else if (0 <= pid)
     {
+      /* The caller successfully waited for a pid but no asynchronous
+	 process was found for it, so this is a synchronous process.  */
+
       synch_process_alive = 0;
 
       /* Report the status of the synchronous process.  */
@@ -6392,8 +6400,12 @@
 	 look around.  */
       if (input_available_clear_time)
 	*input_available_clear_time = make_emacs_time (0, 0);
+
+      return 1;
     }
 #endif
+
+  return 0;
 }
 
 #ifdef SIGCHLD
@@ -6413,23 +6425,8 @@
 static void
 handle_child_signal (int sig)
 {
-  do
-    {
-      pid_t pid;
-      int status;
-
-      do
-	pid = waitpid (-1, &status, WNOHANG | WUNTRACED);
-      while (pid < 0 && errno == EINTR);
-
-      /* PID == 0 means no processes found, PID == -1 means a real failure.
-	 Either way, we have done all our job.  */
-      if (pid <= 0)
-	break;
-
-      record_child_status_change (pid, status);
-    }
-  while (CAN_HANDLE_MULTIPLE_CHILDREN);
+  while (record_child_status_change (-1, 0) && CAN_HANDLE_MULTIPLE_CHILDREN)
+    continue;
 }
 
 static void

=== modified file 'src/syswait.h'
--- src/syswait.h	2012-09-23 22:25:22 +0000
+++ src/syswait.h	2012-10-25 05:29:40 +0000
@@ -23,6 +23,7 @@
 #ifndef EMACS_SYSWAIT_H
 #define EMACS_SYSWAIT_H
 
+#include <stdbool.h>
 #include <sys/types.h>
 
 #ifdef HAVE_SYS_WAIT_H	/* We have sys/wait.h with POSIXoid definitions. */
@@ -52,7 +53,7 @@
 #endif
 
 /* Defined in process.c.  */
-extern void record_child_status_change (pid_t, int);
+extern bool record_child_status_change (pid_t, int);
 
 /* Defined in sysdep.c.  */
 extern void wait_for_termination (pid_t);

bug#8855: dbus error at startup

[Chong Yidong]

Paul Eggert <eggert@cs.ucla.edu> writes:

> Attached is a slightly less-intrusive version of the patch.
> It omits the removal of "#ifdef SIGCHLD", which isn't
> strictly needed to fix the bug.  I don't see any further
> simplifications, unfortunately.  The patch fixes a bug that's
> been reported multiple times so I'm thinking it may be
> worthwhile to install now, even though there's a feature
> freeze.

I think the patch is OK to commit.  Put the extended explanation in a
comment somewhere in the code, though---not in the ChangeLog entry.

bug#8855: dbus error at startup

[Eli Zaretskii]

> From: Chong Yidong <cyd@gnu.org>
> Cc: Eli Zaretskii <eliz@gnu.org>,  8855@debbugs.gnu.org
> Date: Mon, 29 Oct 2012 17:24:28 +0800
> 
> Paul Eggert <eggert@cs.ucla.edu> writes:
> 
> > Attached is a slightly less-intrusive version of the patch.
> > It omits the removal of "#ifdef SIGCHLD", which isn't
> > strictly needed to fix the bug.  I don't see any further
> > simplifications, unfortunately.  The patch fixes a bug that's
> > been reported multiple times so I'm thinking it may be
> > worthwhile to install now, even though there's a feature
> > freeze.
> 
> I think the patch is OK to commit.

I agree.

I also think that after the feature freeze is lifted, it would be good
to refactor the related code so that, e.g., the loop in
process_status_retrieved is joined with the loop(s) in
record_child_status_change.  Since we are now asking about a single
process at a time, I don't see why we need a loop inside a loop.
Previous code didn't have this nested-loop structure, AFAICS.

Btw, what about Posix platforms that don't have WNOHANG defined, and
therefore have 'waitpid' redirected to 'wait' -- are they the same
ones which have CAN_HANDLE_MULTIPLE_CHILDREN set to zero?  (The cpp
directives don't match.)  If some of them don't, then will this new
code work for them with 'wait'?

P.S.  Incidentally, the MS-Windows build always had a 'waitpid'-like
implementation of its 'wait' emulation, it just pretended to be
checking all of its child processes, because that's what the mainline
code wanted, but actually always knew which process it should check
for.  I guess time is ripe for refactoring that code into an emulation
of 'waitpid'.

P.P.S.  We use record_child_status_change in another place as well: in
sysdep.c:wait_for_termination_1.  Will the change in the former still
work correctly in the latter (and its callers)?

bug#8855: dbus error at startup

[Paul Eggert]

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

On 10/29/2012 10:06 AM, Eli Zaretskii wrote:

> Since we are now asking about a single
> process at a time, I don't see why we need a loop inside a loop.

Good point, and this is a performance regression, since the
old code was O(N) and the new O(N**2).  I have fixed that
in a revised proposal (attached).  While fixing this I
found another regression, as already successfully-
waited-for processes were being waited-for again.  This is also
fixed in the revised proposal.

> Btw, what about Posix platforms that don't have WNOHANG defined,

There aren't any anymore.  Posix has required WNOHANG since
Posix.1-1988 and everybody has it implemented by now.
I've added a comment to that effect.  (I also added a comment
along the line that cyd suggested.)

> (The cpp directives don't match.)

Thanks, fixed in the attached proposal.

> I guess time is ripe for refactoring that code into an emulation
> of 'waitpid'.

Yes, that'd be nice.  The attached proposal puts that into a FIXME
comment.

> We use record_child_status_change in another place as well: in
> sysdep.c:wait_for_termination_1.  Will the change in the former still
> work correctly in the latter (and its callers)?

Yes, it should.  One uses -1 as the pid as a special indicator; the
other doesn't, and the implementation treats -1 specially.

Thanks for the review.

[-- Attachment #2: g_spawn_sync.txt --]
[-- Type: text/plain, Size: 11638 bytes --]

=== modified file 'src/ChangeLog'
--- src/ChangeLog	2012-10-30 18:28:48 +0000
+++ src/ChangeLog	2012-10-31 07:22:36 +0000
@@ -1,3 +1,34 @@
+2012-10-31  Paul Eggert  <eggert@cs.ucla.edu>
+
+	Fix a race condition that causes Emacs to mess up glib (Bug#8855).
+	The symptom is a diagnostic "GLib-WARNING **: In call to
+	g_spawn_sync(), exit status of a child process was requested but
+	SIGCHLD action was set to SIG_IGN and ECHILD was received by
+	waitpid(), so exit status can't be returned."  The diagnostic
+	is partly wrong, as the SIGCHLD action is not set to SIG_IGN.
+	The real bug is a race condition between Emacs and glib: Emacs
+	does a waitpid (-1, ...) and reaps glib's subprocess by mistake,
+	so that glib can't find it.  Work around the bug by invoking
+	waitpid only on subprocesses that Emacs itself creates.
+	* process.c (create_process, record_child_status_change):
+	Don't use special value -1 in pid field, as the caller now must
+	know the pid rather than having the callee infer it.  The
+	inference was sometimes incorrect anyway, due to another race.
+	(create_process): Set new 'alive' member if child is created.
+	(process_status_retrieved): New function.
+	(record_child_status_change): Use it.
+	Accept negative 1st argument, which means to wait for the
+	processes that Emacs already knows about.  Move special-case code
+	for DOS_NT (which lacks WNOHANG) here, from caller.  Keep track of
+	processes that have already been waited for, by testing and
+	clearing new 'alive' member.
+	(CAN_HANDLE_MULTIPLE_CHILDREN): Remove, as record_child_status_change
+	now does this internally.
+	(handle_child_signal): Let record_child_status_change do all
+	the work, since we do not want to reap all exited child processes,
+	only the child processes that Emacs itself created.
+	* process.h (Lisp_Process): New boolean member 'alive'.
+
 2012-10-30  Jan Djärv  <jan.h.d@swipnet.se>
 
 	* frame.c (Fframe_pixel_height): Fix documentation (Bug#12733).

=== modified file 'src/process.c'
--- src/process.c	2012-10-19 19:25:18 +0000
+++ src/process.c	2012-10-31 07:19:20 +0000
@@ -130,6 +130,10 @@
 		       EMACS_TIME *, void *);
 #endif
 
+/* This is for DOS_NT ports.  FIXME: Remove this old portability cruft
+   by having DOS_NT ports implement waitpid instead of wait.  Nowadays
+   POSIXish hosts all define waitpid, WNOHANG, and WUNTRACED, as these
+   have been standard since POSIX.1-1988.  */
 #ifndef WNOHANG
 # undef waitpid
 # define waitpid(pid, status, options) wait (status)
@@ -795,9 +799,8 @@
 #ifdef SIGCHLD
 /* Fdelete_process promises to immediately forget about the process, but in
    reality, Emacs needs to remember those processes until they have been
-   treated by the SIGCHLD handler; otherwise this handler would consider the
-   process as being synchronous and say that the synchronous process is
-   dead.  */
+   treated by the SIGCHLD handler and waitpid has been invoked on them;
+   otherwise they might fill up the kernel's process table.  */
 static Lisp_Object deleted_pid_list;
 #endif
 
@@ -1704,16 +1707,7 @@
   if (inchannel > max_process_desc)
     max_process_desc = inchannel;
 
-  /* Until we store the proper pid, enable the SIGCHLD handler
-     to recognize an unknown pid as standing for this process.
-     It is very important not to let this `marker' value stay
-     in the table after this function has returned; if it does
-     it might cause call-process to hang and subsequent asynchronous
-     processes to get their return values scrambled.  */
-  XPROCESS (process)->pid = -1;
-
-  /* This must be called after the above line because it may signal an
-     error. */
+  /* This may signal an error. */
   setup_process_coding_systems (process);
 
   encoded_current_dir = ENCODE_FILE (current_dir);
@@ -1886,6 +1880,8 @@
 #endif
 
   XPROCESS (process)->pid = pid;
+  if (0 <= pid)
+    XPROCESS (process)->alive = 1;
 
   /* Stop blocking signals in the parent.  */
 #ifdef SIGCHLD
@@ -6279,9 +6275,35 @@
   return process;
 }
 \f
-/* On receipt of a signal that a child status has changed, loop asking
-   about children with changed statuses until the system says there
-   are no more.
+/* If the status of the process DESIRED has changed, return true and
+   set *STATUS to its exit status; otherwise, return false.
+   If HAVE is nonnegative, assume that HAVE = waitpid (HAVE, STATUS, ...)
+   has already been invoked, and do not invoke waitpid again.  */
+
+static bool
+process_status_retrieved (pid_t desired, pid_t have, int *status)
+{
+  if (have < 0)
+    {
+      /* Invoke waitpid only with a known process ID; do not invoke
+	 waitpid with a nonpositive argument.  Otherwise, Emacs might
+	 reap an unwanted process by mistake.  For example, invoking
+	 waitpid (-1, ...) can mess up glib by reaping glib's subprocesses,
+	 so that another thread running glib won't find them.  */
+      do
+	have = waitpid (desired, status, WNOHANG | WUNTRACED);
+      while (have < 0 && errno == EINTR);
+    }
+
+  return have == desired;
+}
+
+/* If PID is nonnegative, the child process PID with wait status W has
+   changed its status; record this and return true.
+
+   If PID is negative, ignore W, and look for known child processes
+   of Emacs whose status have changed.  For each one found, record its new
+   status.
 
    All we do is change the status; we do not run sentinels or print
    notifications.  That is saved for the next time keyboard input is
@@ -6304,13 +6326,23 @@
    ** Malloc WARNING: This should never call malloc either directly or
    indirectly; if it does, that is a bug  */
 
-/* Record the changed status of the child process PID with wait status W.  */
 void
 record_child_status_change (pid_t pid, int w)
 {
 #ifdef SIGCHLD
-  Lisp_Object proc;
-  struct Lisp_Process *p;
+
+# ifdef WNOHANG
+  /* On POSIXish hosts, record at most one child only if we already
+     know one child that has exited.  */
+  bool record_at_most_one_child = 0 <= pid;
+# else
+  /* On DOS_NT (the only porting target that lacks WNOHANG),
+     record the status of at most one child process, since the SIGCHLD
+     handler must return right away.  If any more processes want to
+     signal us, we will get another signal.  */
+  bool record_at_most_one_child = 1;
+# endif
+
   Lisp_Object tail;
 
   /* Find the process that signaled us, and record its status.  */
@@ -6318,68 +6350,69 @@
   /* The process can have been deleted by Fdelete_process.  */
   for (tail = deleted_pid_list; CONSP (tail); tail = XCDR (tail))
     {
+      bool all_pids_are_fixnums
+	= (MOST_NEGATIVE_FIXNUM <= TYPE_MINIMUM (pid_t)
+	   && TYPE_MAXIMUM (pid_t) <= MOST_POSITIVE_FIXNUM);
       Lisp_Object xpid = XCAR (tail);
-      if ((INTEGERP (xpid) && pid == XINT (xpid))
-	  || (FLOATP (xpid) && pid == XFLOAT_DATA (xpid)))
+      if (all_pids_are_fixnums ? INTEGERP (xpid) : NUMBERP (xpid))
 	{
-	  XSETCAR (tail, Qnil);
-	  return;
+	  pid_t deleted_pid;
+	  if (INTEGERP (xpid))
+	    deleted_pid = XINT (xpid);
+	  else
+	    deleted_pid = XFLOAT_DATA (xpid);
+	  if (process_status_retrieved (deleted_pid, pid, &w))
+	    {
+	      XSETCAR (tail, Qnil);
+	      if (record_at_most_one_child)
+		return;
+	    }
 	}
     }
 
   /* Otherwise, if it is asynchronous, it is in Vprocess_alist.  */
-  p = 0;
   for (tail = Vprocess_alist; CONSP (tail); tail = XCDR (tail))
     {
-      proc = XCDR (XCAR (tail));
-      p = XPROCESS (proc);
-      if (EQ (p->type, Qreal) && p->pid == pid)
-	break;
-      p = 0;
-    }
-
-  /* Look for an asynchronous process whose pid hasn't been filled
-     in yet.  */
-  if (! p)
-    for (tail = Vprocess_alist; CONSP (tail); tail = XCDR (tail))
-      {
-	proc = XCDR (XCAR (tail));
-	p = XPROCESS (proc);
-	if (p->pid == -1)
-	  break;
-	p = 0;
-      }
-
-  /* Change the status of the process that was found.  */
-  if (p)
-    {
-      int clear_desc_flag = 0;
-
-      p->tick = ++process_tick;
-      p->raw_status = w;
-      p->raw_status_new = 1;
-
-      /* If process has terminated, stop waiting for its output.  */
-      if ((WIFSIGNALED (w) || WIFEXITED (w))
-	  && p->infd >= 0)
-	clear_desc_flag = 1;
-
-      /* We use clear_desc_flag to avoid a compiler bug in Microsoft C.  */
-      if (clear_desc_flag)
+      Lisp_Object proc = XCDR (XCAR (tail));
+      struct Lisp_Process *p = XPROCESS (proc);
+      if (p->alive && process_status_retrieved (p->pid, pid, &w))
 	{
-	  FD_CLR (p->infd, &input_wait_mask);
-	  FD_CLR (p->infd, &non_keyboard_wait_mask);
+	  /* Change the status of the process that was found.  */
+	  p->tick = ++process_tick;
+	  p->raw_status = w;
+	  p->raw_status_new = 1;
+
+	  /* If process has terminated, stop waiting for its output.  */
+	  if (WIFSIGNALED (w) || WIFEXITED (w))
+	    {
+	      int clear_desc_flag = 0;
+	      p->alive = 0;
+	      if (p->infd >= 0)
+		clear_desc_flag = 1;
+
+	      /* clear_desc_flag avoids a compiler bug in Microsoft C.  */
+	      if (clear_desc_flag)
+		{
+		  FD_CLR (p->infd, &input_wait_mask);
+		  FD_CLR (p->infd, &non_keyboard_wait_mask);
+		}
+	    }
+
+	  /* Tell wait_reading_process_output that it needs to wake up and
+	     look around.  */
+	  if (input_available_clear_time)
+	    *input_available_clear_time = make_emacs_time (0, 0);
+
+	  if (record_at_most_one_child)
+	    return;
 	}
-
-      /* Tell wait_reading_process_output that it needs to wake up and
-	 look around.  */
-      if (input_available_clear_time)
-	*input_available_clear_time = make_emacs_time (0, 0);
     }
-  /* There was no asynchronous process found for that pid: we have
-     a synchronous process.  */
-  else
+
+  if (0 <= pid)
     {
+      /* The caller successfully waited for a pid but no asynchronous
+	 process was found for it, so this is a synchronous process.  */
+
       synch_process_alive = 0;
 
       /* Report the status of the synchronous process.  */
@@ -6398,38 +6431,10 @@
 
 #ifdef SIGCHLD
 
-/* On some systems, the SIGCHLD handler must return right away.  If
-   any more processes want to signal us, we will get another signal.
-   Otherwise, loop around to use up all the processes that have
-   something to tell us.  */
-#if (defined WINDOWSNT \
-     || (defined USG && !defined GNU_LINUX \
-	 && !(defined HPUX && defined WNOHANG)))
-enum { CAN_HANDLE_MULTIPLE_CHILDREN = 0 };
-#else
-enum { CAN_HANDLE_MULTIPLE_CHILDREN = 1 };
-#endif
-
 static void
 handle_child_signal (int sig)
 {
-  do
-    {
-      pid_t pid;
-      int status;
-
-      do
-	pid = waitpid (-1, &status, WNOHANG | WUNTRACED);
-      while (pid < 0 && errno == EINTR);
-
-      /* PID == 0 means no processes found, PID == -1 means a real failure.
-	 Either way, we have done all our job.  */
-      if (pid <= 0)
-	break;
-
-      record_child_status_change (pid, status);
-    }
-  while (CAN_HANDLE_MULTIPLE_CHILDREN);
+  record_child_status_change (-1, 0);
 }
 
 static void

=== modified file 'src/process.h'
--- src/process.h	2012-08-27 17:23:48 +0000
+++ src/process.h	2012-10-31 07:19:20 +0000
@@ -142,6 +142,9 @@
     /* Flag to set coding-system of the process buffer from the
        coding_system used to decode process output.  */
     unsigned int inherit_coding_system_flag : 1;
+    /* Whether the process is alive, i.e., can be waited for.  Running
+       processes can be waited for, but exited and fake processes cannot.  */
+    unsigned int alive : 1;
     /* Record the process status in the raw form in which it comes from `wait'.
        This is to avoid consing in a signal handler.  The `raw_status_new'
        flag indicates that `raw_status' contains a new status that still

bug#8855: confirmation from glib side

[Paul Eggert]

I got confirmation from Colin Walters that the Emacs code
is currently broken, with respect to glib, and that the
approach taken by the proposed patch is the right one.
See <https://bugzilla.gnome.org/show_bug.cgi?id=687075#c2>.

bug#8855: installed patch into trunk

[Paul Eggert]

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

bug#8855: Fix backported to Emacs 24

[Paul Eggert]

As discussed on emacs-devel, I had to undo that backport to
the emacs-24 branch.

The problem was that the fix for Bug#8855 introduces a new
bug, Bug#12980.  I have followed up at Bug#12980 but the patch
is nontrivial, so for now Bug#8855 remains unfixed on the
emacs-24 branch.