Code review round 2

Styling issues and obvious memory leaks.

gio/gdbusaddress.c

@@ -84,7 +84,7 @@
  * Since GLib 2.72, `unix:` addresses are supported on Windows with `AF_UNIX`
  * support (Windows 10).
  *
- * `unixexec:` address type support.
+ * Since GLib 2.80, `unixexec:` addresses are supported on Unix systems.
  */
 
 static gchar *get_session_address_platform_specific (GError **error);
@@ -305,8 +305,7 @@ is_valid_unixexec (const gchar *address_entry,
                    GHashTable *key_value_pairs,
                    GError **error)
 {
-  gboolean ret = TRUE;
-  gchar *path = g_hash_table_lookup (key_value_pairs, "path");
+  const gchar *path = g_hash_table_lookup (key_value_pairs, "path");
 
   if (path == NULL ||
       path[0] == '\0')
@@ -314,12 +313,11 @@ is_valid_unixexec (const gchar *address_entry,
       g_set_error (error,
                    G_IO_ERROR,
                    G_IO_ERROR_INVALID_ARGUMENT,
-                   _ ("Error in address “%s” — missing key “path”"),
+                   _("Error in address “%s” — missing key “path”"),
                    address_entry);
-      ret = FALSE;
+      return FALSE;
     }
-
-  return ret;
+  return TRUE;
 }
 #endif
 
@@ -585,15 +583,16 @@ out:
 }
 
 #ifdef G_OS_UNIX
+/* This function is called between fork() and exec() and hence must be
+ * async-signal-safe (see signal-safety(7)). */
 static gint
-sane_dup2 (gint fd1, gint fd2)
+safe_dup2 (gint fd1, gint fd2)
 {
   gint ret;
 
-retry:
-  ret = dup2 (fd1, fd2);
-  if (ret < 0 && errno == EINTR)
-    goto retry;
+  do
+    ret = dup2 (fd1, fd2);
+  while (ret < 0 && (errno == EINTR || errno == EBUSY));
 
   return ret;
 }
@@ -606,9 +605,9 @@ unixexec_prepare_child (gpointer data)
   gint *s = (gint *) data;
 
   /* We want to keep socket pair alive. Therefore we don't set CLOEXEC on it */
-  fd = sane_dup2 (s[1], STDIN_FILENO);
+  fd = safe_dup2 (s[1], STDIN_FILENO);
   g_assert (fd == STDIN_FILENO);
-  fd = sane_dup2 (s[1], STDOUT_FILENO);
+  fd = safe_dup2 (s[1], STDOUT_FILENO);
   g_assert (fd == STDOUT_FILENO);
 }
 
@@ -631,9 +630,9 @@ g_dbus_address_try_connect_one (const gchar   *address_entry,
 
 #ifdef G_OS_UNIX
 /* Mark it as static and keep it in gdbusaddress.c until some other glib
-   file needs it
+   file needs it, returns TRUE on SUCCESS and FALSE on error.
  */
-static void
+static gboolean
 g_socketpair (gint domain,
               gint type,
               gint protocol,
@@ -657,9 +656,9 @@ g_socketpair (gint domain,
       int errsv = errno;
 
       g_set_error (error, G_IO_ERROR, g_io_error_from_errno (errsv),
-                   _ ("Unable to create socketpair: %s"), g_strerror (errsv));
+                   _("Unable to create socketpair: %s"), g_strerror (errsv));
       errno = errsv;
-      return;
+      return FALSE;
     }
 
   if (fcntl (s[0], F_SETFD, FD_CLOEXEC) < 0 ||
@@ -668,13 +667,16 @@ g_socketpair (gint domain,
       int errsv = errno;
       g_close (s[0], NULL);
       g_close (s[1], NULL);
+      s[0] = -1;
+      s[1] = -1;
       g_set_error (error,
                    G_IO_ERROR,
                    g_io_error_from_errno (errsv),
-                   _ ("Failed to call fcntl: %s"),
+                   _("Failed to call fcntl: %s"),
                    g_strerror (errsv));
-      return;
+      return FALSE;
     }
+  return TRUE;
 }
 #endif
 
@@ -739,12 +741,12 @@ g_dbus_address_connect (const gchar   *address_entry,
       gchar *args;
       guint argnum = 1;
 
-      g_socketpair (AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK, 0, s, error);
-
-      if (*error != NULL)
+      if (!g_socketpair (AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK, 0, s, error))
         goto out;
 
-      argv = g_malloc_n (g_hash_table_size (key_value_pairs) + 2, sizeof (gchar *));
+      argv = g_malloc_n (g_hash_table_size (key_value_pairs) + 2, sizeof (gchar *)); // Can return NULL if allocation fails
+      if (argv == NULL)
+        goto out;
       /* We know path exist if we made it here */
       argv[i++] = g_hash_table_lookup (key_value_pairs, "path");
       if (g_hash_table_contains (key_value_pairs, "argv0"))
@@ -779,6 +781,9 @@ g_dbus_address_connect (const gchar   *address_entry,
         {
           g_close (s[0], NULL);
           g_close (s[1], NULL);
+          s[0] = -1;
+          s[1] = -1;
+          g_free (argv);
           goto out;
         }
 
@@ -788,10 +793,14 @@ g_dbus_address_connect (const gchar   *address_entry,
       if (fdsocket == NULL)
         {
           g_close (s[0], NULL);
+          g_free (argv);
+          g_free (child_pid);
           goto out;
         }
       ret = (GIOStream *) g_socket_connection_factory_create_connection (fdsocket);
       g_child_watch_add (child_pid, unixexec_child_watch, ret);
+      g_free (fdsocket);
+      g_free (argv);
     }
 #endif
   else if (g_strcmp0 (transport_name, "tcp") == 0 || g_strcmp0 (transport_name, "nonce-tcp") == 0)

gio/tests/gdbus-addresses.c

@@ -123,11 +123,10 @@ test_unix_address (void)
   assert_not_supported_address ("unix:path=/tmp,dir=/tmp");
   assert_not_supported_address ("unix:abstract=/tmp/foo,dir=/tmp");
   assert_not_supported_address ("unix:");
-}
 #else
   g_test_skip ("Skipping Unix address tests on non-Unix platform");
-}
 #endif
+}
 
 static void
 test_unixexec_address_connect (void)
@@ -184,11 +183,10 @@ test_unixexec_address_connect (void)
       &error);
   g_assert_no_error (error);
   g_free (con);
-}
 #else
   g_test_skip ("Skipping Unix address tests on non-Unix platform");
-}
 #endif
+}
 
 static void
 test_unixexec_address (void)
@@ -210,11 +208,10 @@ test_unixexec_address (void)
                                       &error);
   g_assert_no_error (error);
   g_object_unref (s);
-}
 #else
   g_test_skip ("Skipping Unix address tests on non-Unix platform");
-}
 #endif
+}
 
 static void
 test_nonce_tcp_address (void)