From: Paul Eggert <eggert@cs.ucla.edu>
To: 8855@debbugs.gnu.org
Subject: bug#8855: dbus error at startup
Date: Wed, 24 Oct 2012 22:35:15 -0700 [thread overview]
Message-ID: <5088CF93.2020802@cs.ucla.edu> (raw)
In-Reply-To: <yxqzkllvcyk.fsf@fencepost.gnu.org>
[-- 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);
next prev parent reply other threads:[~2012-10-25 5:35 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-06-13 17:49 bug#8855: dbus error at startup Dan Nicolaescu
2011-06-26 10:22 ` Michael Albinus
2012-10-25 5:35 ` Paul Eggert [this message]
2012-10-25 16:24 ` Eli Zaretskii
2012-10-25 20:57 ` Paul Eggert
2012-10-29 1:38 ` Paul Eggert
2012-10-29 9:24 ` Chong Yidong
2012-10-29 17:06 ` Eli Zaretskii
2012-10-31 7:36 ` Paul Eggert
2012-10-29 6:27 ` bug#8855: confirmation from glib side Paul Eggert
2012-11-03 18:34 ` bug#8855: installed patch into trunk Paul Eggert
2012-11-27 2:32 ` bug#8855: Fix backported to Emacs 24 Paul Eggert
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5088CF93.2020802@cs.ucla.edu \
--to=eggert@cs.ucla.edu \
--cc=8855@debbugs.gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.