From ce0022933c255313e010b27f977f4ae02aad1e7e Mon Sep 17 00:00:00 2001
From: Colin Walters <walters@verbum.org>
Date: Mon, 29 Oct 2012 15:44:16 -0400
Subject: [PATCH] Merge waitpid() from g_spawn_sync into gmain()

This is preparatory work for a future commit which will add a
"catchall" waitpid API.  If we don't synchronize here with the worker
thread, race conditions are possible.

This also ensures we have an error message if someone adds a child
watch for a nonexistent pid, etc.  Previously, we'd simply keep
calling waitpid() getting ECHILD, and ignoring it until the source was
removed. Now, we g_warning() and fire the source.

Thirdly, this ensures that the waitpid() call in gmain handles EINTR,
like the g_spawn_sync() one did.

https://bugzilla.gnome.org/show_bug.cgi?id=687061
---
 glib/gmain.c  | 20 ++++++++++++----
 glib/gspawn.c | 76 +++++++++++++++++++++++++++++++----------------------------
 2 files changed, 56 insertions(+), 40 deletions(-)

diff --git a/glib/gmain.c b/glib/gmain.c
index 133b0f1..af1092d 100644
--- a/glib/gmain.c
+++ b/glib/gmain.c
@@ -4421,12 +4421,24 @@ dispatch_unix_signals (void)
 
           if (!source->child_exited)
             {
-              if (waitpid (source->pid, &source->child_status, WNOHANG) > 0)
+              pid_t pid;
+              do
                 {
-                  source->child_exited = TRUE;
-
-                  wake_source ((GSource *) source);
+                  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);
             }
         }
     }
diff --git a/glib/gspawn.c b/glib/gspawn.c
index 5e4922d..3545a78 100644
--- a/glib/gspawn.c
+++ b/glib/gspawn.c
@@ -46,6 +46,7 @@
 
 #include "genviron.h"
 #include "gmem.h"
+#include "gmain.h"
 #include "gshell.h"
 #include "gstring.h"
 #include "gstrfuncs.h"
@@ -212,6 +213,21 @@ read_data (GString *str,
     }
 }
 
+typedef struct {
+  GMainLoop *loop;
+  gint *status_p;
+} SyncWaitpidData;
+
+static void
+on_sync_waitpid (GPid     pid,
+                 gint     status,
+                 gpointer user_data)
+{
+  SyncWaitpidData *data = user_data;
+  *(data->status_p) = status;
+  g_main_loop_quit (data->loop);
+}
+
 /**
  * g_spawn_sync:
  * @working_directory: (allow-none): child's current working directory, or %NULL to inherit parent's
@@ -267,6 +283,7 @@ g_spawn_sync (const gchar          *working_directory,
   GString *errstr = NULL;
   gboolean failed;
   gint status;
+  SyncWaitpidData waitpid_data;
   
   g_return_val_if_fail (argv != NULL, FALSE);
   g_return_val_if_fail (!(flags & G_SPAWN_DO_NOT_REAP_CHILD), FALSE);
@@ -399,45 +416,32 @@ g_spawn_sync (const gchar          *working_directory,
     close_and_invalidate (&outpipe);
   if (errpipe >= 0)
     close_and_invalidate (&errpipe);
-  
-  /* Wait for child to exit, even if we have
-   * an error pending.
+
+  /* Now create a temporary main context and loop, with just one
+   * waitpid source.  We used to invoke waitpid() directly here, but
+   * this way we unify with the worker thread in gmain.c.
    */
- again:
-      
-  ret = waitpid (pid, &status, 0);
+  {
+    GMainContext *context;
+    GMainLoop *loop;
+    GSource *source;
 
-  if (ret < 0)
-    {
-      if (errno == EINTR)
-        goto again;
-      else if (errno == ECHILD)
-        {
-          if (exit_status)
-            {
-              g_warning ("In call to g_spawn_sync(), 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_spawn_sync either directly or indirectly.");
-            }
-          else
-            {
-              /* We don't need the exit status. */
-            }
-        }
-      else
-        {
-          if (!failed) /* avoid error pileups */
-            {
-              int errsv = errno;
+    context = g_main_context_new ();
+    loop = g_main_loop_new (context, TRUE);
 
-              failed = TRUE;
-                  
-              g_set_error (error,
-                           G_SPAWN_ERROR,
-                           G_SPAWN_ERROR_READ,
-                           _("Unexpected error in waitpid() (%s)"),
-                           g_strerror (errsv));
-            }
-        }
-    }
+    waitpid_data.loop = loop;
+    waitpid_data.status_p = &status;
+    
+    source = g_child_watch_source_new (pid);
+    g_source_set_callback (source, (GSourceFunc)on_sync_waitpid, &waitpid_data, NULL);
+    g_source_attach (source, context);
+    g_source_unref (source);
+    
+    g_main_loop_run (loop);
+
+    g_main_context_unref (context);
+    g_main_loop_unref (loop);
+  }
   
   if (failed)
     {
-- 
2.7.4