[glib/glib-2-54] gsubprocess: Fix a critical calling communicate() with no pipes

[Philip Withnall <pwithnall src gnome org>]

commit b4811aa15b27f368ff7359f77a5cd9dc33f82c35
Author: Philip Withnall <withnall endlessm com>
Date:   Fri Feb 9 12:25:54 2018 +0000

    gsubprocess: Fix a critical calling communicate() with no pipes
    
    If calling g_subprocess_communicate() on a GSubprocess with no
    stdout/stderr pipe, a critical warning would be emitted from
    g_memory_output_stream_steal_as_bytes(), as it would be called on a NULL
    output stream.
    
    Fix that, improve the relevant GIR annotations, and expand the unit
    tests to cover it (and various other combinations of flags).
    
    Signed-off-by: Philip Withnall <withnall endlessm com>
    
    https://bugzilla.gnome.org/show_bug.cgi?id=793331

 gio/gsubprocess.c       |  20 +++---
 gio/tests/gsubprocess.c | 187 +++++++++++++++++++++++++++++++++++-------------
 2 files changed, 149 insertions(+), 58 deletions(-)
---
diff --git a/gio/gsubprocess.c b/gio/gsubprocess.c
index 8525f3bfc..531f4be67 100644
--- a/gio/gsubprocess.c
+++ b/gio/gsubprocess.c
@@ -1628,8 +1628,8 @@ g_subprocess_communicate_internal (GSubprocess         *subprocess,
  * @subprocess: a #GSubprocess
  * @stdin_buf: (nullable): data to send to the stdin of the subprocess, or %NULL
  * @cancellable: a #GCancellable
- * @stdout_buf: (out): data read from the subprocess stdout
- * @stderr_buf: (out): data read from the subprocess stderr
+ * @stdout_buf: (out) (nullable) (optional) (transfer full): data read from the subprocess stdout
+ * @stderr_buf: (out) (nullable) (optional) (transfer full): data read from the subprocess stderr
  * @error: a pointer to a %NULL #GError pointer, or %NULL
  *
  * Communicate with the subprocess until it terminates, and all input
@@ -1733,8 +1733,8 @@ g_subprocess_communicate_async (GSubprocess         *subprocess,
  * g_subprocess_communicate_finish:
  * @subprocess: Self
  * @result: Result
- * @stdout_buf: (out): Return location for stdout data
- * @stderr_buf: (out): Return location for stderr data
+ * @stdout_buf: (out) (nullable) (optional) (transfer full): Return location for stdout data
+ * @stderr_buf: (out) (nullable) (optional) (transfer full): Return location for stderr data
  * @error: Error
  *
  * Complete an invocation of g_subprocess_communicate_async().
@@ -1761,9 +1761,9 @@ g_subprocess_communicate_finish (GSubprocess   *subprocess,
   if (success)
     {
       if (stdout_buf)
-        *stdout_buf = g_memory_output_stream_steal_as_bytes (state->stdout_buf);
+        *stdout_buf = (state->stdout_buf != NULL) ? g_memory_output_stream_steal_as_bytes 
(state->stdout_buf) : NULL;
       if (stderr_buf)
-        *stderr_buf = g_memory_output_stream_steal_as_bytes (state->stderr_buf);
+        *stderr_buf = (state->stderr_buf != NULL) ? g_memory_output_stream_steal_as_bytes 
(state->stderr_buf) : NULL;
     }
 
   g_object_unref (result);
@@ -1775,8 +1775,8 @@ g_subprocess_communicate_finish (GSubprocess   *subprocess,
  * @subprocess: a #GSubprocess
  * @stdin_buf: (nullable): data to send to the stdin of the subprocess, or %NULL
  * @cancellable: a #GCancellable
- * @stdout_buf: (out): data read from the subprocess stdout
- * @stderr_buf: (out): data read from the subprocess stderr
+ * @stdout_buf: (out) (nullable) (optional) (transfer full): data read from the subprocess stdout
+ * @stderr_buf: (out) (nullable) (optional) (transfer full): data read from the subprocess stderr
  * @error: a pointer to a %NULL #GError pointer, or %NULL
  *
  * Like g_subprocess_communicate(), but validates the output of the
@@ -1882,8 +1882,8 @@ communicate_result_validate_utf8 (const char            *stream_name,
  * g_subprocess_communicate_utf8_finish:
  * @subprocess: Self
  * @result: Result
- * @stdout_buf: (out): Return location for stdout data
- * @stderr_buf: (out): Return location for stderr data
+ * @stdout_buf: (out) (nullable) (optional) (transfer full): Return location for stdout data
+ * @stderr_buf: (out) (nullable) (optional) (transfer full): Return location for stderr data
  * @error: Error
  *
  * Complete an invocation of g_subprocess_communicate_utf8_async().
diff --git a/gio/tests/gsubprocess.c b/gio/tests/gsubprocess.c
index 39f748bf4..ad65a1df7 100644
--- a/gio/tests/gsubprocess.c
+++ b/gio/tests/gsubprocess.c
@@ -695,6 +695,7 @@ test_multi_1 (void)
 }
 
 typedef struct {
+  GSubprocessFlags flags;
   gboolean is_utf8;
   gboolean running;
   GError *error;
@@ -706,45 +707,67 @@ on_communicate_complete (GObject               *proc,
                          gpointer               user_data)
 {
   TestAsyncCommunicateData *data = user_data;
-  GBytes *stdout = NULL;
-  char *stdout_str = NULL;
+  GBytes *stdout_bytes = NULL, *stderr_bytes = NULL;
+  char *stdout_str = NULL, *stderr_str = NULL;
   const guint8 *stdout_data;
   gsize stdout_len;
 
   data->running = FALSE;
   if (data->is_utf8)
     (void) g_subprocess_communicate_utf8_finish ((GSubprocess*)proc, result,
-                                                 &stdout_str, NULL, &data->error);
+                                                 &stdout_str, &stderr_str, &data->error);
   else
     (void) g_subprocess_communicate_finish ((GSubprocess*)proc, result,
-                                            &stdout, NULL, &data->error);
+                                            &stdout_bytes, &stderr_bytes, &data->error);
   if (data->error)
       return;
 
-  if (!data->is_utf8)
+  if (data->flags & G_SUBPROCESS_FLAGS_STDOUT_PIPE)
     {
-      g_assert (stdout != NULL);
-      stdout_data = g_bytes_get_data (stdout, &stdout_len);
+      if (data->is_utf8)
+        {
+          stdout_data = (guint8*)stdout_str;
+          stdout_len = strlen (stdout_str);
+        }
+      else
+        stdout_data = g_bytes_get_data (stdout_bytes, &stdout_len);
+
+      g_assert_cmpmem (stdout_data, stdout_len, "# hello world\n", 14);
+    }
+  else
+    {
+      g_assert_null (stdout_str);
+      g_assert_null (stdout_bytes);
+    }
+
+  if (data->flags & G_SUBPROCESS_FLAGS_STDERR_PIPE)
+    {
+      if (data->is_utf8)
+        g_assert_nonnull (stderr_str);
+      else
+        g_assert_nonnull (stderr_bytes);
     }
   else
     {
-      g_assert (stdout_str != NULL);
-      stdout_data = (guint8*)stdout_str;
-      stdout_len = strlen (stdout_str);
+      g_assert_null (stderr_str);
+      g_assert_null (stderr_bytes);
     }
 
-  g_assert_cmpmem (stdout_data, stdout_len, "hello world", 11);
-  if (stdout)
-    g_bytes_unref (stdout);
+  g_clear_pointer (&stdout_bytes, g_bytes_unref);
+  g_clear_pointer (&stderr_bytes, g_bytes_unref);
   g_free (stdout_str);
+  g_free (stderr_str);
 }
 
+/* Test g_subprocess_communicate_async() works correctly with a variety of flags,
+ * as passed in via @test_data. */
 static void
-test_communicate_async (void)
+test_communicate_async (gconstpointer test_data)
 {
+  GSubprocessFlags flags = GPOINTER_TO_INT (test_data);
   GError *error = NULL;
   GPtrArray *args;
-  TestAsyncCommunicateData data = { 0, };
+  TestAsyncCommunicateData data = { flags, 0, };
   GSubprocess *proc;
   GCancellable *cancellable = NULL;
   GBytes *input;
@@ -752,12 +775,14 @@ test_communicate_async (void)
 
   args = get_test_subprocess_args ("cat", NULL);
   proc = g_subprocess_newv ((const gchar* const*)args->pdata,
-                            G_SUBPROCESS_FLAGS_STDIN_PIPE | G_SUBPROCESS_FLAGS_STDOUT_PIPE,
+                            G_SUBPROCESS_FLAGS_STDIN_PIPE | flags,
                             &error);
   g_assert_no_error (error);
   g_ptr_array_free (args, TRUE);
 
-  hellostring = "hello world";
+  /* Include a leading hash and trailing newline so that if this gets onto the
+   * test’s stdout, it doesn’t mess up TAP output. */
+  hellostring = "# hello world\n";
   input = g_bytes_new_static (hellostring, strlen (hellostring));
 
   g_subprocess_communicate_async (proc, input,
@@ -775,60 +800,76 @@ test_communicate_async (void)
   g_object_unref (proc);
 }
 
+/* Test g_subprocess_communicate() works correctly with a variety of flags,
+ * as passed in via @test_data. */
 static void
-test_communicate (void)
+test_communicate (gconstpointer test_data)
 {
+  GSubprocessFlags flags = GPOINTER_TO_INT (test_data);
   GError *error = NULL;
   GPtrArray *args;
   GSubprocess *proc;
   GCancellable *cancellable = NULL;
   GBytes *input;
   const gchar *hellostring;
-  GBytes *stdout;
+  GBytes *stdout_bytes, *stderr_bytes;
   const gchar *stdout_data;
   gsize stdout_len;
 
   args = get_test_subprocess_args ("cat", NULL);
   proc = g_subprocess_newv ((const gchar* const*)args->pdata,
-                            G_SUBPROCESS_FLAGS_STDIN_PIPE
-                            | G_SUBPROCESS_FLAGS_STDOUT_PIPE
-                            | G_SUBPROCESS_FLAGS_STDERR_MERGE,
+                            G_SUBPROCESS_FLAGS_STDIN_PIPE | flags,
                             &error);
   g_assert_no_error (error);
   g_ptr_array_free (args, TRUE);
 
-  hellostring = "hello world";
+  /* Include a leading hash and trailing newline so that if this gets onto the
+   * test’s stdout, it doesn’t mess up TAP output. */
+  hellostring = "# hello world\n";
   input = g_bytes_new_static (hellostring, strlen (hellostring));
 
-  g_subprocess_communicate (proc, input, cancellable, &stdout, NULL, &error);
+  g_subprocess_communicate (proc, input, cancellable, &stdout_bytes, &stderr_bytes, &error);
   g_assert_no_error (error);
-  stdout_data = g_bytes_get_data (stdout, &stdout_len);
 
-  g_assert_cmpmem (stdout_data, stdout_len, "hello world", 11);
+  if (flags & G_SUBPROCESS_FLAGS_STDOUT_PIPE)
+    {
+      stdout_data = g_bytes_get_data (stdout_bytes, &stdout_len);
+      g_assert_cmpmem (stdout_data, stdout_len, "# hello world\n", 14);
+    }
+  else
+    g_assert_null (stdout_bytes);
+  if (flags & G_SUBPROCESS_FLAGS_STDERR_PIPE)
+    g_assert_nonnull (stderr_bytes);
+  else
+    g_assert_null (stderr_bytes);
 
   g_bytes_unref (input);
-  g_bytes_unref (stdout);
+  g_clear_pointer (&stdout_bytes, g_bytes_unref);
+  g_clear_pointer (&stderr_bytes, g_bytes_unref);
   g_object_unref (proc);
 }
 
+/* Test g_subprocess_communicate_utf8_async() works correctly with a variety of
+ * flags, as passed in via @test_data. */
 static void
-test_communicate_utf8_async (void)
+test_communicate_utf8_async (gconstpointer test_data)
 {
+  GSubprocessFlags flags = GPOINTER_TO_INT (test_data);
   GError *error = NULL;
   GPtrArray *args;
-  TestAsyncCommunicateData data = { 0, };
+  TestAsyncCommunicateData data = { flags, 0, };
   GSubprocess *proc;
   GCancellable *cancellable = NULL;
 
   args = get_test_subprocess_args ("cat", NULL);
   proc = g_subprocess_newv ((const gchar* const*)args->pdata,
-                            G_SUBPROCESS_FLAGS_STDIN_PIPE | G_SUBPROCESS_FLAGS_STDOUT_PIPE,
+                            G_SUBPROCESS_FLAGS_STDIN_PIPE | flags,
                             &error);
   g_assert_no_error (error);
   g_ptr_array_free (args, TRUE);
 
   data.is_utf8 = TRUE;
-  g_subprocess_communicate_utf8_async (proc, "hello world",
+  g_subprocess_communicate_utf8_async (proc, "# hello world\n",
                                        cancellable,
                                        on_communicate_complete, 
                                        &data);
@@ -842,33 +883,43 @@ test_communicate_utf8_async (void)
   g_object_unref (proc);
 }
 
+/* Test g_subprocess_communicate_utf8() works correctly with a variety of flags,
+ * as passed in via @test_data. */
 static void
-test_communicate_utf8 (void)
+test_communicate_utf8 (gconstpointer test_data)
 {
+  GSubprocessFlags flags = GPOINTER_TO_INT (test_data);
   GError *error = NULL;
   GPtrArray *args;
   GSubprocess *proc;
   GCancellable *cancellable = NULL;
   const gchar *stdin_buf;
-  gchar *stdout_buf;
+  gchar *stdout_buf, *stderr_buf;
 
   args = get_test_subprocess_args ("cat", NULL);
   proc = g_subprocess_newv ((const gchar* const*)args->pdata,
-                            G_SUBPROCESS_FLAGS_STDIN_PIPE
-                            | G_SUBPROCESS_FLAGS_STDOUT_PIPE
-                            | G_SUBPROCESS_FLAGS_STDERR_MERGE,
+                            G_SUBPROCESS_FLAGS_STDIN_PIPE | flags,
                             &error);
   g_assert_no_error (error);
   g_ptr_array_free (args, TRUE);
 
-  stdin_buf = "hello world";
+  /* Include a leading hash and trailing newline so that if this gets onto the
+   * test’s stdout, it doesn’t mess up TAP output. */
+  stdin_buf = "# hello world\n";
 
-  g_subprocess_communicate_utf8 (proc, stdin_buf, cancellable, &stdout_buf, NULL, &error);
+  g_subprocess_communicate_utf8 (proc, stdin_buf, cancellable, &stdout_buf, &stderr_buf, &error);
   g_assert_no_error (error);
 
-  g_assert (strcmp (stdout_buf, "hello world") == 0);
+  if (flags & G_SUBPROCESS_FLAGS_STDOUT_PIPE)
+    g_assert_cmpstr (stdout_buf, ==, "# hello world\n");
+  else
+    g_assert_null (stdout_buf);
+  if (flags & G_SUBPROCESS_FLAGS_STDERR_PIPE)
+    g_assert_nonnull (stderr_buf);
+  else     g_assert_null (stderr_buf);
+
   g_free (stdout_buf);
-  
+  g_free (stderr_buf);
   g_object_unref (proc);
 }
 
@@ -903,15 +954,16 @@ test_communicate_nothing (void)
 static void
 test_communicate_utf8_invalid (void)
 {
+  GSubprocessFlags flags = G_SUBPROCESS_FLAGS_STDOUT_PIPE;
   GError *error = NULL;
   GPtrArray *args;
-  TestAsyncCommunicateData data = { 0, };
+  TestAsyncCommunicateData data = { flags, 0, };
   GSubprocess *proc;
   GCancellable *cancellable = NULL;
 
   args = get_test_subprocess_args ("cat", NULL);
   proc = g_subprocess_newv ((const gchar* const*)args->pdata,
-                            G_SUBPROCESS_FLAGS_STDIN_PIPE | G_SUBPROCESS_FLAGS_STDOUT_PIPE,
+                            G_SUBPROCESS_FLAGS_STDIN_PIPE | flags,
                             &error);
   g_assert_no_error (error);
   g_ptr_array_free (args, TRUE);
@@ -1409,6 +1461,22 @@ test_launcher_environment (void)
 int
 main (int argc, char **argv)
 {
+  const struct
+    {
+      const gchar *subtest;
+      GSubprocessFlags flags;
+    }
+  flags_vectors[] =
+    {
+      { "", G_SUBPROCESS_FLAGS_STDOUT_PIPE | G_SUBPROCESS_FLAGS_STDERR_MERGE },
+      { "/no-pipes", G_SUBPROCESS_FLAGS_NONE },
+      { "/separate-stderr", G_SUBPROCESS_FLAGS_STDOUT_PIPE | G_SUBPROCESS_FLAGS_STDERR_PIPE },
+      { "/stdout-only", G_SUBPROCESS_FLAGS_STDOUT_PIPE },
+      { "/stderr-only", G_SUBPROCESS_FLAGS_STDERR_PIPE },
+      { "/stdout-silence", G_SUBPROCESS_FLAGS_STDOUT_SILENCE },
+    };
+  gsize i;
+
   g_test_init (&argc, &argv, NULL);
   g_test_bug_base ("https://bugzilla.gnome.org/";);
 
@@ -1430,12 +1498,35 @@ main (int argc, char **argv)
   g_test_add_func ("/gsubprocess/cat-utf8", test_cat_utf8);
   g_test_add_func ("/gsubprocess/cat-eof", test_cat_eof);
   g_test_add_func ("/gsubprocess/multi1", test_multi_1);
-  g_test_add_func ("/gsubprocess/communicate", test_communicate);
-  g_test_add_func ("/gsubprocess/communicate-async", test_communicate_async);
-  g_test_add_func ("/gsubprocess/communicate-utf8", test_communicate_utf8);
-  g_test_add_func ("/gsubprocess/communicate-utf8-async", test_communicate_utf8_async);
-  g_test_add_func ("/gsubprocess/communicate-utf8-invalid", test_communicate_utf8_invalid);
-  g_test_add_func ("/gsubprocess/communicate-nothing", test_communicate_nothing);
+
+  /* Add various tests for g_subprocess_communicate() with different flags. */
+  for (i = 0; i < G_N_ELEMENTS (flags_vectors); i++)
+    {
+      gchar *test_path = NULL;
+
+      test_path = g_strdup_printf ("/gsubprocess/communicate%s", flags_vectors[i].subtest);
+      g_test_add_data_func (test_path, GINT_TO_POINTER (flags_vectors[i].flags),
+                            test_communicate);
+      g_free (test_path);
+
+      test_path = g_strdup_printf ("/gsubprocess/communicate/async%s", flags_vectors[i].subtest);
+      g_test_add_data_func (test_path, GINT_TO_POINTER (flags_vectors[i].flags),
+                            test_communicate_async);
+      g_free (test_path);
+
+      test_path = g_strdup_printf ("/gsubprocess/communicate/utf8%s", flags_vectors[i].subtest);
+      g_test_add_data_func (test_path, GINT_TO_POINTER (flags_vectors[i].flags),
+                            test_communicate_utf8);
+      g_free (test_path);
+
+      test_path = g_strdup_printf ("/gsubprocess/communicate/utf8/async%s", flags_vectors[i].subtest);
+      g_test_add_data_func (test_path, GINT_TO_POINTER (flags_vectors[i].flags),
+                            test_communicate_utf8_async);
+      g_free (test_path);
+    }
+
+  g_test_add_func ("/gsubprocess/communicate/utf8/invalid", test_communicate_utf8_invalid);
+  g_test_add_func ("/gsubprocess/communicate/nothing", test_communicate_nothing);
   g_test_add_func ("/gsubprocess/terminate", test_terminate);
   g_test_add_func ("/gsubprocess/env", test_env);
   g_test_add_func ("/gsubprocess/env/inherit", test_env_inherit);