* bug#8855: dbus error at startup @ 2011-06-13 17:49 Dan Nicolaescu 2011-06-26 10:22 ` Michael Albinus ` (4 more replies) 0 siblings, 5 replies; 19+ messages in thread From: Dan Nicolaescu @ 2011-06-13 17:49 UTC (permalink / raw To: 8855 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? ^ permalink raw reply [flat|nested] 19+ messages in thread
* bug#8855: dbus error at startup 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 ` (3 subsequent siblings) 4 siblings, 0 replies; 19+ messages in thread From: Michael Albinus @ 2011-06-26 10:22 UTC (permalink / raw To: Dan Nicolaescu; +Cc: 8855 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* bug#8855: dbus error at startup 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 2012-10-25 16:24 ` Eli Zaretskii 2012-10-29 6:27 ` bug#8855: confirmation from glib side Paul Eggert ` (2 subsequent siblings) 4 siblings, 1 reply; 19+ messages in thread From: Paul Eggert @ 2012-10-25 5:35 UTC (permalink / raw To: 8855 [-- 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); ^ permalink raw reply [flat|nested] 19+ messages in thread
* bug#8855: dbus error at startup 2012-10-25 5:35 ` Paul Eggert @ 2012-10-25 16:24 ` Eli Zaretskii 2012-10-25 20:57 ` Paul Eggert 2012-10-29 1:38 ` Paul Eggert 0 siblings, 2 replies; 19+ messages in thread From: Eli Zaretskii @ 2012-10-25 16:24 UTC (permalink / raw To: Paul Eggert; +Cc: 8855 > 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? ^ permalink raw reply [flat|nested] 19+ messages in thread
* bug#8855: dbus error at startup 2012-10-25 16:24 ` Eli Zaretskii @ 2012-10-25 20:57 ` Paul Eggert 2012-10-29 1:38 ` Paul Eggert 1 sibling, 0 replies; 19+ messages in thread From: Paul Eggert @ 2012-10-25 20:57 UTC (permalink / raw To: Eli Zaretskii; +Cc: 8855 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* bug#8855: dbus error at startup 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 1 sibling, 1 reply; 19+ messages in thread From: Paul Eggert @ 2012-10-29 1:38 UTC (permalink / raw To: Eli Zaretskii; +Cc: 8855 [-- 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); ^ permalink raw reply [flat|nested] 19+ messages in thread
* bug#8855: dbus error at startup 2012-10-29 1:38 ` Paul Eggert @ 2012-10-29 9:24 ` Chong Yidong 2012-10-29 17:06 ` Eli Zaretskii 0 siblings, 1 reply; 19+ messages in thread From: Chong Yidong @ 2012-10-29 9:24 UTC (permalink / raw To: Paul Eggert; +Cc: 8855 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* bug#8855: dbus error at startup 2012-10-29 9:24 ` Chong Yidong @ 2012-10-29 17:06 ` Eli Zaretskii 2012-10-31 7:36 ` Paul Eggert 0 siblings, 1 reply; 19+ messages in thread From: Eli Zaretskii @ 2012-10-29 17:06 UTC (permalink / raw To: Chong Yidong; +Cc: eggert, 8855 > 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)? ^ permalink raw reply [flat|nested] 19+ messages in thread
* bug#8855: dbus error at startup 2012-10-29 17:06 ` Eli Zaretskii @ 2012-10-31 7:36 ` Paul Eggert 0 siblings, 0 replies; 19+ messages in thread From: Paul Eggert @ 2012-10-31 7:36 UTC (permalink / raw To: Eli Zaretskii; +Cc: Chong Yidong, 8855 [-- 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* bug#8855: confirmation from glib side 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 @ 2012-10-29 6:27 ` 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 4 siblings, 0 replies; 19+ messages in thread From: Paul Eggert @ 2012-10-29 6:27 UTC (permalink / raw To: 8855 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>. ^ permalink raw reply [flat|nested] 19+ messages in thread
* bug#8855: installed patch into trunk 2011-06-13 17:49 bug#8855: dbus error at startup Dan Nicolaescu ` (2 preceding siblings ...) 2012-10-29 6:27 ` bug#8855: confirmation from glib side Paul Eggert @ 2012-11-03 18:34 ` Paul Eggert 2012-11-27 2:32 ` bug#8855: Fix backported to Emacs 24 Paul Eggert 4 siblings, 0 replies; 19+ messages in thread From: Paul Eggert @ 2012-11-03 18:34 UTC (permalink / raw To: 8855-done I installed the patch into the trunk as bzr 110784 and am marking this as done. ^ permalink raw reply [flat|nested] 19+ messages in thread
* bug#8855: Fix backported to Emacs 24 2011-06-13 17:49 bug#8855: dbus error at startup Dan Nicolaescu ` (3 preceding siblings ...) 2012-11-03 18:34 ` bug#8855: installed patch into trunk Paul Eggert @ 2012-11-27 2:32 ` Paul Eggert 4 siblings, 0 replies; 19+ messages in thread From: Paul Eggert @ 2012-11-27 2:32 UTC (permalink / raw To: 8855 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Backport fix for bug #8855? @ 2012-11-21 22:33 Ken Brown 2012-11-21 23:50 ` bug#8855: Fix backported to Emacs 24 Paul Eggert 0 siblings, 1 reply; 19+ messages in thread From: Ken Brown @ 2012-11-21 22:33 UTC (permalink / raw To: Paul Eggert; +Cc: Emacs Is there any chance of backporting the fix for bug #8855 to the emacs-24 branch? The bug is pretty annoying, and there haven't been any problems reported with the fix, as far as I know. Ken ^ permalink raw reply [flat|nested] 19+ messages in thread
* bug#8855: Fix backported to Emacs 24 2012-11-21 22:33 Backport fix for bug #8855? Ken Brown @ 2012-11-21 23:50 ` Paul Eggert 2012-11-22 3:49 ` Eli Zaretskii 2012-11-22 15:13 ` Stefan Monnier 0 siblings, 2 replies; 19+ messages in thread From: Paul Eggert @ 2012-11-21 23:50 UTC (permalink / raw To: 8855 [-- Attachment #1: Type: text/plain, Size: 91 bytes --] Attached is the fixed backported to the emacs-24 branch (bzr 110936) if there's interest. [-- Attachment #2: g_spawn_sync.txt --] [-- Type: text/plain, Size: 11638 bytes --] === modified file 'src/ChangeLog' --- src/ChangeLog 2012-11-21 19:28:14 +0000 +++ src/ChangeLog 2012-11-21 23:43:47 +0000 @@ -1,3 +1,34 @@ +2012-11-21 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-11-21 Eli Zaretskii <eliz@gnu.org> * indent.c (Fvertical_motion): If the starting position is covered === modified file 'src/process.c' --- src/process.c 2012-10-31 17:27:29 +0000 +++ src/process.c 2012-11-21 23:43:47 +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); @@ -1880,6 +1874,8 @@ #endif XPROCESS (process)->pid = pid; + if (0 <= pid) + XPROCESS (process)->alive = 1; /* Stop blocking signals in the parent. */ #ifdef SIGCHLD @@ -6273,9 +6269,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 @@ -6298,13 +6320,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. */ @@ -6312,68 +6344,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. */ @@ -6392,38 +6425,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-11-21 23:43:47 +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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* bug#8855: Fix backported to Emacs 24 2012-11-21 23:50 ` bug#8855: Fix backported to Emacs 24 Paul Eggert @ 2012-11-22 3:49 ` Eli Zaretskii 2012-11-22 15:13 ` Stefan Monnier 1 sibling, 0 replies; 19+ messages in thread From: Eli Zaretskii @ 2012-11-22 3:49 UTC (permalink / raw To: Paul Eggert; +Cc: 8855 > Date: Wed, 21 Nov 2012 15:50:59 -0800 > From: Paul Eggert <eggert@cs.ucla.edu> > > Attached is the fixed backported to the emacs-24 branch > (bzr 110936) if there's interest. The related changes for MS-Windows (r110922) are needed as well. ^ permalink raw reply [flat|nested] 19+ messages in thread
* bug#8855: Fix backported to Emacs 24 2012-11-21 23:50 ` bug#8855: Fix backported to Emacs 24 Paul Eggert 2012-11-22 3:49 ` Eli Zaretskii @ 2012-11-22 15:13 ` Stefan Monnier 2012-11-22 19:20 ` Paul Eggert 1 sibling, 1 reply; 19+ messages in thread From: Stefan Monnier @ 2012-11-22 15:13 UTC (permalink / raw To: Paul Eggert; +Cc: 8855 > Attached is the fixed backported to the emacs-24 branch > (bzr 110936) if there's interest. There's something I don't understand. On Oct 29, Chong said: > > 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. So how come the patch is not in emacs-24 yet? Stefan ^ permalink raw reply [flat|nested] 19+ messages in thread
* bug#8855: Fix backported to Emacs 24 2012-11-22 15:13 ` Stefan Monnier @ 2012-11-22 19:20 ` Paul Eggert 2012-11-23 2:40 ` Chong Yidong 0 siblings, 1 reply; 19+ messages in thread From: Paul Eggert @ 2012-11-22 19:20 UTC (permalink / raw To: Stefan Monnier; +Cc: 8855 On 11/22/2012 07:13 AM, Stefan Monnier wrote: > So how come the patch is not in emacs-24 yet? I understood Chong's comment to be about the trunk, not about emacs-24. The trunk was frozen at the time, if I recall, so his permission would have been needed for the trunk. ^ permalink raw reply [flat|nested] 19+ messages in thread
* bug#8855: Fix backported to Emacs 24 2012-11-22 19:20 ` Paul Eggert @ 2012-11-23 2:40 ` Chong Yidong 2012-11-23 21:46 ` Ken Brown 0 siblings, 1 reply; 19+ messages in thread From: Chong Yidong @ 2012-11-23 2:40 UTC (permalink / raw To: Paul Eggert; +Cc: 8855 Paul Eggert <eggert@cs.ucla.edu> writes: > On 11/22/2012 07:13 AM, Stefan Monnier wrote: >> So how come the patch is not in emacs-24 yet? > > I understood Chong's comment to be about the trunk, not about > emacs-24. The trunk was frozen at the time, if I recall, so his > permission would have been needed for the trunk. The emacs-24 branch hadn't even been created when I wrote that, so "commits to the trunk" at the time meant "code going into 24.3". Feel free to backport it now. ^ permalink raw reply [flat|nested] 19+ messages in thread
* bug#8855: Fix backported to Emacs 24 2012-11-23 2:40 ` Chong Yidong @ 2012-11-23 21:46 ` Ken Brown 2012-11-23 22:22 ` Paul Eggert 0 siblings, 1 reply; 19+ messages in thread From: Ken Brown @ 2012-11-23 21:46 UTC (permalink / raw To: Chong Yidong; +Cc: Paul Eggert, 8855 On 11/22/2012 9:40 PM, Chong Yidong wrote: > Paul Eggert <eggert@cs.ucla.edu> writes: > >> On 11/22/2012 07:13 AM, Stefan Monnier wrote: >>> So how come the patch is not in emacs-24 yet? >> >> I understood Chong's comment to be about the trunk, not about >> emacs-24. The trunk was frozen at the time, if I recall, so his >> permission would have been needed for the trunk. > > The emacs-24 branch hadn't even been created when I wrote that, so > "commits to the trunk" at the time meant "code going into 24.3". > > Feel free to backport it now. It would be good to get this done before tomorrow's release of 24.2.90. Paul, is there still time to do it? Ken ^ permalink raw reply [flat|nested] 19+ messages in thread
* bug#8855: Fix backported to Emacs 24 2012-11-23 21:46 ` Ken Brown @ 2012-11-23 22:22 ` Paul Eggert 0 siblings, 0 replies; 19+ messages in thread From: Paul Eggert @ 2012-11-23 22:22 UTC (permalink / raw To: Ken Brown; +Cc: Chong Yidong, 8855 On 11/23/2012 01:46 PM, Ken Brown wrote: > Paul, is there still time to do it? Yes, done with Eli's suggestions as emacs-24 bzr 110946. On 11/22/2012 06:40 PM, Chong Yidong wrote: > > The emacs-24 branch hadn't even been created when I wrote that, so > "commits to the trunk" at the time meant "code going into 24.3". Sorry about the confusion -- my memory must have been faulty. ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2012-11-27 2:32 UTC | newest] Thread overview: 19+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 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 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 -- strict thread matches above, loose matches on Subject: below -- 2012-11-21 22:33 Backport fix for bug #8855? Ken Brown 2012-11-21 23:50 ` bug#8855: Fix backported to Emacs 24 Paul Eggert 2012-11-22 3:49 ` Eli Zaretskii 2012-11-22 15:13 ` Stefan Monnier 2012-11-22 19:20 ` Paul Eggert 2012-11-23 2:40 ` Chong Yidong 2012-11-23 21:46 ` Ken Brown 2012-11-23 22:22 ` Paul Eggert
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.