[glib/wip/subprocess-2013: 1/6] gmain: Add private API to create Unix child watch that uses waitid()

[Ryan Lortie <ryanl src gnome org>]

commit 47d82f236dce51d967228e75361d5c530d104eef
Author: Colin Walters <walters verbum org>
Date:   Mon May 21 17:09:06 2012 -0400

    gmain: Add private API to create Unix child watch that uses waitid()
    
    This avoids collecting the zombie child, which means that the PID
    can't be reused.  This prevents possible race conditions that might
    occur were one to send e.g. SIGTERM to a child.
    
    This race condition has always existed due to the way we called
    waitpid() for the app, but the window was widened when we moved the
    waitpid() calls into a separate thread.
    
    If waitid() isn't available, we return NULL, and consumers of this
    private API (namely, GSubprocess) will need to handle that.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=672102

 configure.ac        |    2 +-
 glib/glib-private.c |    3 +-
 glib/glib-private.h |   10 +++
 glib/gmain.c        |  163 +++++++++++++++++++++++++++++++++++++-------------
 4 files changed, 133 insertions(+), 45 deletions(-)
---
diff --git a/configure.ac b/configure.ac
index aea8ba5..445b1b8 100644
--- a/configure.ac
+++ b/configure.ac
@@ -992,7 +992,7 @@ AC_MSG_RESULT(unsigned $glib_size_type)
 
 # Check for some functions
 AC_CHECK_FUNCS(lstat strerror strsignal memmove vsnprintf stpcpy strcasecmp strncasecmp poll getcwd vasprintf setenv unsetenv getc_unlocked readlink symlink fdwalk memmem)
-AC_CHECK_FUNCS(chown lchmod lchown fchmod fchown link utimes getgrgid getpwuid getresuid)
+AC_CHECK_FUNCS(chown lchmod lchown fchmod fchown link utimes getgrgid getpwuid getresuid waitid)
 AC_CHECK_FUNCS(getmntent_r setmntent endmntent hasmntopt getfsstat getvfsstat)
 # Check for high-resolution sleep functions
 AC_CHECK_FUNCS(splice)
diff --git a/glib/glib-private.c b/glib/glib-private.c
index d2da2cd..6c2642d 100644
--- a/glib/glib-private.c
+++ b/glib/glib-private.c
@@ -43,7 +43,8 @@ glib__private__ (void)
     g_get_worker_context,
 
     g_check_setuid,
-    g_main_context_new_with_next_id
+    g_main_context_new_with_next_id,
+    g_child_watch_source_new_with_flags
   };
 
   return &table;
diff --git a/glib/glib-private.h b/glib/glib-private.h
index 7e2e61e..8ecfce8 100644
--- a/glib/glib-private.h
+++ b/glib/glib-private.h
@@ -23,6 +23,11 @@
 #include <glib.h>
 #include "gwakeup.h"
 
+typedef enum {
+  _G_CHILD_WATCH_FLAGS_NONE = 0,
+  _G_CHILD_WATCH_FLAGS_WNOWAIT = (1 << 0)
+} _GChildWatchFlags;
+
 GMainContext *          g_get_worker_context            (void);
 gboolean                g_check_setuid                  (void);
 GMainContext *          g_main_context_new_with_next_id (guint next_id);
@@ -33,6 +38,9 @@ GLIB_AVAILABLE_IN_ALL
 gchar *_glib_get_locale_dir    (void);
 #endif
 
+GSource *               g_child_watch_source_new_with_flags (GPid pid,
+                                                             gint flags);
+
 #define GLIB_PRIVATE_CALL(symbol) (glib__private__()->symbol)
 
 typedef struct {
@@ -49,6 +57,8 @@ typedef struct {
 
   gboolean              (* g_check_setuid)              (void);
   GMainContext *        (* g_main_context_new_with_next_id) (guint next_id);
+  GSource *             (* g_child_watch_source_new_with_flags) (GPid pid,
+                                                                 gint flags);
   /* Add other private functions here, initialize them in glib-private.c */
 } GLibPrivateVTable;
 
diff --git a/glib/gmain.c b/glib/gmain.c
index 9f9eccc..57dc846 100644
--- a/glib/gmain.c
+++ b/glib/gmain.c
@@ -293,7 +293,9 @@ struct _GChildWatchSource
 #ifdef G_OS_WIN32
   GPollFD     poll;
 #else /* G_OS_WIN32 */
-  gboolean    child_exited;
+  guint       child_exited : 1;
+  guint       wnowait : 1;
+  guint       reserved : 30;
 #endif /* G_OS_WIN32 */
 };
 
@@ -4773,6 +4775,82 @@ wake_source (GSource *source)
   G_UNLOCK(main_context_list);
 }
 
+/* This is a hack; we want to use the newer waitid() call, but the use
+ * of the waitpid() status code API is baked into the GChildWatchSource
+ * callback signature, so we need to synthesize it from the siginfo_t
+ * we get from waitid().
+ */ 
+#ifdef HAVE_WAITID
+#define __G_MAKE_EXITSTATUS(ecode, signum) ((ecode) << 8 | (signum))
+static gint
+waitpid_status_from_siginfo (siginfo_t *siginfo)
+{
+  G_STATIC_ASSERT(((WIFEXITED (__G_MAKE_EXITSTATUS (1, 0))) &&		\
+		   !(WIFSIGNALED (__G_MAKE_EXITSTATUS (1, 0))) &&	\
+		   (WEXITSTATUS (__G_MAKE_EXITSTATUS (1, 0)) == 1)));
+  G_STATIC_ASSERT((!WIFEXITED (__G_MAKE_EXITSTATUS (0, 1))) &&		\
+		  (WIFSIGNALED (__G_MAKE_EXITSTATUS (0, 1))) &&		\
+		  (WTERMSIG (__G_MAKE_EXITSTATUS (0, 1)) == 1));
+  if (siginfo->si_code == CLD_EXITED)
+    return __G_MAKE_EXITSTATUS(siginfo->si_status, 0);
+ else if (siginfo->si_code == CLD_KILLED || siginfo->si_code == CLD_DUMPED)
+   return __G_MAKE_EXITSTATUS(0, siginfo->si_status);
+ else
+   return __G_MAKE_EXITSTATUS(0xFF, 0);
+}
+#undef __G_MAKE_EXITSTATUS
+#endif /* HAVE_WAITID */
+
+/* Returns TRUE if we need to wake the source */
+static gboolean
+unix_child_watch_source_waitpid (GChildWatchSource    *source)
+{
+  pid_t pid;
+
+  if (source->child_exited)
+    return FALSE;
+
+#ifdef HAVE_WAITID
+  if (source->wnowait)
+    {
+      siginfo_t siginfo;
+      int r;
+
+      siginfo.si_pid = 0;
+      do
+        r = waitid (P_PID, (id_t)source->pid, &siginfo, WEXITED | WNOHANG | WNOWAIT);
+      while (r == -1 && errno == EINTR);
+
+      if (r == 0 && siginfo.si_pid == source->pid)
+	{
+	  source->child_exited = TRUE;
+	  source->child_status = waitpid_status_from_siginfo (&siginfo);
+	  return TRUE;
+	}
+    } else
+#endif
+    {
+      do
+        pid = waitpid (source->pid, &source->child_status, WNOHANG);
+      while (pid == -1 && errno == EINTR);
+
+      if (pid > 0)
+        {
+          source->child_exited = TRUE;
+          return TRUE;
+        }
+      else if (pid == -1 && errno == ECHILD)
+        {
+          g_warning ("GChildWatchSource: Exit status of a child process was requested but ECHILD was received by waitpid(). Most likely the process is ignoring SIGCHLD, or some other thread is invoking waitpid() with a nonpositive first argument; either behavior can break applications that use g_child_watch_add()/g_spawn_sync() either directly or indirectly.");
+          source->child_exited = TRUE;
+          source->child_status = 0;
+          return TRUE;
+        }
+    }
+
+  return FALSE;
+}
+
 static void
 dispatch_unix_signals (void)
 {
@@ -4799,28 +4877,9 @@ dispatch_unix_signals (void)
       for (node = unix_child_watches; node; node = node->next)
         {
           GChildWatchSource *source = node->data;
-
-          if (!source->child_exited)
-            {
-              pid_t pid;
-              do
-                {
-                  pid = waitpid (source->pid, &source->child_status, WNOHANG);
-                  if (pid > 0)
-                    {
-                      source->child_exited = TRUE;
-                      wake_source ((GSource *) source);
-                    }
-                  else if (pid == -1 && errno == ECHILD)
-                    {
-                      g_warning ("GChildWatchSource: Exit status of a child process was requested but ECHILD was received by waitpid(). Most likely the process is ignoring SIGCHLD, or some other thread is invoking waitpid() with a nonpositive first argument; either behavior can break applications that use g_child_watch_add()/g_spawn_sync() either directly or indirectly.");
-                      source->child_exited = TRUE;
-                      source->child_status = 0;
-                      wake_source ((GSource *) source);
-                    }
-                }
-              while (pid == -1 && errno == EINTR);
-            }
+	  
+	  if (unix_child_watch_source_waitpid (source))
+	    wake_source ((GSource *) source);
         }
     }
 
@@ -5012,6 +5071,43 @@ g_unix_signal_handler (int signum)
 
 #endif /* !G_OS_WIN32 */
 
+GSource *
+g_child_watch_source_new_with_flags (GPid pid,
+				     gint flags)
+{
+  GSource *source;
+  GChildWatchSource *child_watch_source;
+
+#if defined(G_OS_UNIX) && !defined(HAVE_WAITID)
+  if (flags & _G_CHILD_WATCH_FLAGS_WNOWAIT)
+    return NULL;
+#else
+  source = g_source_new (&g_child_watch_funcs, sizeof (GChildWatchSource));
+  child_watch_source = (GChildWatchSource *)source;
+
+  child_watch_source->pid = pid;
+#if defined(G_OS_UNIX) && defined(HAVE_WAITID)
+  if (flags & _G_CHILD_WATCH_FLAGS_WNOWAIT)
+    child_watch_source->wnowait = TRUE;
+#endif
+
+#ifdef G_OS_WIN32
+  child_watch_source->poll.fd = (gintptr) pid;
+  child_watch_source->poll.events = G_IO_IN;
+
+  g_source_add_poll (source, &child_watch_source->poll);
+#else /* G_OS_WIN32 */
+  G_LOCK (unix_signal_lock);
+  ensure_unix_signal_handler_installed_unlocked (SIGCHLD);
+  unix_child_watches = g_slist_prepend (unix_child_watches, child_watch_source);
+  unix_child_watch_source_waitpid (child_watch_source);
+  G_UNLOCK (unix_signal_lock);
+#endif /* G_OS_WIN32 */
+
+  return source;
+#endif
+}
+
 /**
  * g_child_watch_source_new:
  * @pid: process to watch. On POSIX the pid of a child process. On
@@ -5044,26 +5140,7 @@ g_unix_signal_handler (int signum)
 GSource *
 g_child_watch_source_new (GPid pid)
 {
-  GSource *source = g_source_new (&g_child_watch_funcs, sizeof (GChildWatchSource));
-  GChildWatchSource *child_watch_source = (GChildWatchSource *)source;
-
-  child_watch_source->pid = pid;
-
-#ifdef G_OS_WIN32
-  child_watch_source->poll.fd = (gintptr) pid;
-  child_watch_source->poll.events = G_IO_IN;
-
-  g_source_add_poll (source, &child_watch_source->poll);
-#else /* G_OS_WIN32 */
-  G_LOCK (unix_signal_lock);
-  ensure_unix_signal_handler_installed_unlocked (SIGCHLD);
-  unix_child_watches = g_slist_prepend (unix_child_watches, child_watch_source);
-  if (waitpid (pid, &child_watch_source->child_status, WNOHANG) > 0)
-    child_watch_source->child_exited = TRUE;
-  G_UNLOCK (unix_signal_lock);
-#endif /* G_OS_WIN32 */
-
-  return source;
+  return g_child_watch_source_new_with_flags (pid, 0);
 }
 
 /**