Merge branch 'mcatanzaro/gdbus-dir-addresses' into 'master'

gdbus: support unix:dir= addresses, and related cleanups

Closes #1808

See merge request GNOME/glib!911

gio/gdbusaddress.c

@@ -127,12 +127,14 @@ is_valid_unix (const gchar  *address_entry,
   GList *keys;
   GList *l;
   const gchar *path;
+  const gchar *dir;
   const gchar *tmpdir;
   const gchar *abstract;
 
   ret = FALSE;
   keys = NULL;
   path = NULL;
+  dir = NULL;
   tmpdir = NULL;
   abstract = NULL;
 
@@ -142,6 +144,8 @@ is_valid_unix (const gchar  *address_entry,
       const gchar *key = l->data;
       if (g_strcmp0 (key, "path") == 0)
         path = g_hash_table_lookup (key_value_pairs, key);
+      else if (g_strcmp0 (key, "dir") == 0)
+        dir = g_hash_table_lookup (key_value_pairs, key);
       else if (g_strcmp0 (key, "tmpdir") == 0)
         tmpdir = g_hash_table_lookup (key_value_pairs, key);
       else if (g_strcmp0 (key, "abstract") == 0)
@@ -158,9 +162,8 @@ is_valid_unix (const gchar  *address_entry,
         }
     }
 
-  if ((path != NULL && tmpdir != NULL) ||
+  /* Exactly one key must be set */
-      (tmpdir != NULL && abstract != NULL) ||
+  if ((path != NULL) + (dir != NULL) + (tmpdir != NULL) + (abstract != NULL) > 1)
-      (abstract != NULL && path != NULL))
     {
       g_set_error (error,
              G_IO_ERROR,
@@ -169,12 +172,12 @@ is_valid_unix (const gchar  *address_entry,
              address_entry);
       goto out;
     }
-  else if (path == NULL && tmpdir == NULL && abstract == NULL)
+  else if (path == NULL && dir == NULL && tmpdir == NULL && abstract == NULL)
     {
       g_set_error (error,
                    G_IO_ERROR,
                    G_IO_ERROR_INVALID_ARGUMENT,
-                   _("Address “%s” is invalid (need exactly one of path, tmpdir or abstract keys)"),
+                   _("Address “%s” is invalid (need exactly one of path, dir, tmpdir, or abstract keys)"),
                    address_entry);
       goto out;
     }

gio/gdbusserver.c

@@ -101,6 +101,7 @@ struct _GDBusServer
 
   gchar *client_address;
 
+  gchar *unix_socket_path;
   GSocketListener *listener;
   gboolean is_using_listener;
   gulong run_signal_handler_id;
@@ -161,6 +162,17 @@ static void initable_iface_init       (GInitableIface *initable_iface);
 G_DEFINE_TYPE_WITH_CODE (GDBusServer, g_dbus_server, G_TYPE_OBJECT,
                          G_IMPLEMENT_INTERFACE (G_TYPE_INITABLE, initable_iface_init))
 
+static void
+g_dbus_server_dispose (GObject *object)
+{
+  GDBusServer *server = G_DBUS_SERVER (object);
+
+  if (server->active)
+    g_dbus_server_stop (server);
+
+  G_OBJECT_CLASS (g_dbus_server_parent_class)->dispose (object);
+}
+
 static void
 g_dbus_server_finalize (GObject *object)
 {
@@ -183,10 +195,20 @@ g_dbus_server_finalize (GObject *object)
       memset (server->nonce, '\0', 16);
       g_free (server->nonce);
     }
-  /* we could unlink the nonce file but I don't
+
-   * think it's really worth the effort/risk
+  if (server->unix_socket_path)
-   */
+    {
-  g_free (server->nonce_file);
+      if (g_unlink (server->unix_socket_path) != 0)
+        g_warning ("Failed to delete %s: %s", server->unix_socket_path, g_strerror (errno));
+      g_free (server->unix_socket_path);
+    }
+
+  if (server->nonce_file)
+    {
+      if (g_unlink (server->nonce_file) != 0)
+        g_warning ("Failed to delete %s: %s", server->nonce_file, g_strerror (errno));
+      g_free (server->nonce_file);
+    }
 
   g_main_context_unref (server->main_context_at_construction);
 
@@ -270,6 +292,7 @@ g_dbus_server_class_init (GDBusServerClass *klass)
 {
   GObjectClass *gobject_class = G_OBJECT_CLASS (klass);
 
+  gobject_class->dispose      = g_dbus_server_dispose;
   gobject_class->finalize     = g_dbus_server_finalize;
   gobject_class->set_property = g_dbus_server_set_property;
   gobject_class->get_property = g_dbus_server_get_property;
@@ -641,7 +664,7 @@ random_ascii (void)
   return ret;
 }
 
-/* note that address_entry has already been validated => exactly one of path, tmpdir or abstract keys are set */
+/* note that address_entry has already been validated => exactly one of path, dir, tmpdir, or abstract keys are set */
 static gboolean
 try_unix (GDBusServer  *server,
           const gchar  *address_entry,
@@ -650,6 +673,7 @@ try_unix (GDBusServer  *server,
 {
   gboolean ret;
   const gchar *path;
+  const gchar *dir;
   const gchar *tmpdir;
   const gchar *abstract;
   GSocketAddress *address;
@@ -658,6 +682,7 @@ try_unix (GDBusServer  *server,
   address = NULL;
 
   path = g_hash_table_lookup (key_value_pairs, "path");
+  dir = g_hash_table_lookup (key_value_pairs, "dir");
   tmpdir = g_hash_table_lookup (key_value_pairs, "tmpdir");
   abstract = g_hash_table_lookup (key_value_pairs, "abstract");
 
@@ -665,20 +690,21 @@ try_unix (GDBusServer  *server,
     {
       address = g_unix_socket_address_new (path);
     }
-  else if (tmpdir != NULL)
+  else if (dir != NULL || tmpdir != NULL)
     {
       gint n;
       GString *s;
       GError *local_error;
 
     retry:
-      s = g_string_new (tmpdir);
+      s = g_string_new (tmpdir != NULL ? tmpdir : dir);
       g_string_append (s, "/dbus-");
       for (n = 0; n < 8; n++)
         g_string_append_c (s, random_ascii ());
 
-      /* prefer abstract namespace if available */
+      /* prefer abstract namespace if available for tmpdir: addresses
-      if (g_unix_socket_address_abstract_names_supported ())
+       * abstract namespace is disallowed for dir: addresses */
+      if (tmpdir != NULL && g_unix_socket_address_abstract_names_supported ())
         address = g_unix_socket_address_new_with_type (s->str,
                                                        -1,
                                                        G_UNIX_SOCKET_ADDRESS_ABSTRACT);
@@ -713,7 +739,7 @@ try_unix (GDBusServer  *server,
           g_set_error_literal (error,
                                G_IO_ERROR,
                                G_IO_ERROR_NOT_SUPPORTED,
-                               _("Abstract name space not supported"));
+                               _("Abstract namespace not supported"));
           goto out;
         }
       address = g_unix_socket_address_new_with_type (abstract,
@@ -743,24 +769,30 @@ try_unix (GDBusServer  *server,
       /* Fill out client_address if the connection attempt worked */
       if (ret)
         {
+          const char *address_path;
+          char *escaped_path;
+
           server->is_using_listener = TRUE;
+          address_path = g_unix_socket_address_get_path (G_UNIX_SOCKET_ADDRESS (address));
+          escaped_path = g_dbus_address_escape_value (address_path);
 
           switch (g_unix_socket_address_get_address_type (G_UNIX_SOCKET_ADDRESS (address)))
             {
             case G_UNIX_SOCKET_ADDRESS_ABSTRACT:
-              server->client_address = g_strdup_printf ("unix:abstract=%s",
+              server->client_address = g_strdup_printf ("unix:abstract=%s", escaped_path);
-                                                        g_unix_socket_address_get_path (G_UNIX_SOCKET_ADDRESS (address)));
               break;
 
             case G_UNIX_SOCKET_ADDRESS_PATH:
-              server->client_address = g_strdup_printf ("unix:path=%s",
+              server->client_address = g_strdup_printf ("unix:path=%s", escaped_path);
-                                                        g_unix_socket_address_get_path (G_UNIX_SOCKET_ADDRESS (address)));
+              server->unix_socket_path = g_strdup (address_path);
               break;
 
             default:
               g_assert_not_reached ();
               break;
             }
+
+          g_free (escaped_path);
         }
       g_object_unref (address);
     }
@@ -852,6 +884,7 @@ try_tcp (GDBusServer  *server,
       gsize bytes_written;
       gsize bytes_remaining;
       char *file_escaped;
+      char *host_escaped;
 
       server->nonce = g_new0 (guchar, 16);
       for (n = 0; n < 16; n++)
@@ -891,11 +924,13 @@ try_tcp (GDBusServer  *server,
         }
       if (!g_close (fd, error))
         goto out;
-      file_escaped = g_uri_escape_string (server->nonce_file, "/\\", FALSE);
+      host_escaped = g_dbus_address_escape_value (host);
+      file_escaped = g_dbus_address_escape_value (server->nonce_file);
       server->client_address = g_strdup_printf ("nonce-tcp:host=%s,port=%d,noncefile=%s",
-                                                host,
+                                                host_escaped,
                                                 port_num,
                                                 file_escaped);
+      g_free (host_escaped);
       g_free (file_escaped);
     }
   else

gio/tests/gdbus-addresses.c

@@ -113,9 +113,13 @@ test_unix_address (void)
   g_assert_false (g_dbus_is_address ("unix:path=/foo;abstract=/bar"));
   assert_is_supported_address ("unix:path=/tmp/concrete;unix:abstract=/tmp/abstract");
   assert_is_supported_address ("unix:tmpdir=/tmp");
+  assert_is_supported_address ("unix:dir=/tmp");
   assert_not_supported_address ("unix:tmpdir=/tmp,path=/tmp");
   assert_not_supported_address ("unix:tmpdir=/tmp,abstract=/tmp/foo");
+  assert_not_supported_address ("unix:tmpdir=/tmp,dir=/tmp");
   assert_not_supported_address ("unix:path=/tmp,abstract=/tmp/foo");
+  assert_not_supported_address ("unix:path=/tmp,dir=/tmp");
+  assert_not_supported_address ("unix:abstract=/tmp/foo,dir=/tmp");
   assert_not_supported_address ("unix:");
 #endif
 }

gio/tests/gdbus-peer.c

@@ -53,6 +53,7 @@ static gboolean is_unix = TRUE;
 static gboolean is_unix = FALSE;
 #endif
 
+static gchar *tmpdir = NULL;
 static gchar *tmp_address = NULL;
 static gchar *test_guid = NULL;
 static GMutex service_loop_lock;
@@ -267,6 +268,58 @@ on_proxy_signal_received_with_name_set (GDBusProxy *proxy,
 
 /* ---------------------------------------------------------------------------------------------------- */
 
+static void
+setup_test_address (void)
+{
+  if (is_unix)
+    {
+      g_test_message ("Testing with unix:tmpdir address");
+      if (g_unix_socket_address_abstract_names_supported ())
+	tmp_address = g_strdup ("unix:tmpdir=/tmp/gdbus-test-");
+      else
+	{
+	  tmpdir = g_dir_make_tmp ("gdbus-test-XXXXXX", NULL);
+	  tmp_address = g_strdup_printf ("unix:tmpdir=%s", tmpdir);
+	}
+    }
+  else
+    tmp_address = g_strdup ("nonce-tcp:");
+}
+
+#ifdef G_OS_UNIX
+static void
+setup_dir_test_address (void)
+{
+  g_test_message ("Testing with unix:dir address");
+  tmpdir = g_dir_make_tmp ("gdbus-test-XXXXXX", NULL);
+  tmp_address = g_strdup_printf ("unix:dir=%s", tmpdir);
+}
+
+static void
+setup_path_test_address (void)
+{
+  g_test_message ("Testing with unix:path address");
+  tmpdir = g_dir_make_tmp ("gdbus-test-XXXXXX", NULL);
+  tmp_address = g_strdup_printf ("unix:path=%s/gdbus-peer-socket", tmpdir);
+}
+#endif
+
+static void
+teardown_test_address (void)
+{
+  g_free (tmp_address);
+  if (tmpdir)
+    {
+      /* Ensuring the rmdir succeeds also ensures any sockets created on the
+       * filesystem are also deleted.
+       */
+      g_assert_cmpint (g_rmdir (tmpdir), ==, 0);
+      g_clear_pointer (&tmpdir, g_free);
+    }
+}
+
+/* ---------------------------------------------------------------------------------------------------- */
+
 static gboolean
 on_authorize_authenticated_peer (GDBusAuthObserver *observer,
                                  GIOStream         *stream,
@@ -656,7 +709,7 @@ read_all_from_fd (gint fd, gsize *out_len, GError **error)
 #endif
 
 static void
-test_peer (void)
+do_test_peer (void)
 {
   GDBusConnection *c;
   GDBusConnection *c2;
@@ -977,6 +1030,27 @@ test_peer (void)
   g_thread_join (service_thread);
 }
 
+static void
+test_peer (void)
+{
+  /* Run this test multiple times using different address formats to ensure
+   * they all work.
+   */
+  setup_test_address ();
+  do_test_peer ();
+  teardown_test_address ();
+
+#ifdef G_OS_UNIX
+  setup_dir_test_address ();
+  do_test_peer ();
+  teardown_test_address ();
+
+  setup_path_test_address ();
+  do_test_peer ();
+  teardown_test_address ();
+#endif
+}
+
 /* ---------------------------------------------------------------------------------------------------- */
 
 typedef struct
@@ -1116,6 +1190,8 @@ delayed_message_processing (void)
   GThread *service_thread;
   guint n;
 
+  setup_test_address ();
+
   data = g_new0 (DmpData, 1);
 
   service_thread = g_thread_new ("dmp",
@@ -1160,6 +1236,7 @@ delayed_message_processing (void)
   g_main_loop_quit (data->loop);
   g_thread_join (service_thread);
   dmp_data_free (data);
+  teardown_test_address ();
 }
 
 /* ---------------------------------------------------------------------------------------------------- */
@@ -1351,12 +1428,16 @@ test_nonce_tcp (void)
   g_error_free (error);
   g_assert (c == NULL);
 
-  g_free (nonce_file);
+  /* Recreate the nonce-file so we can ensure the server deletes it when stopped. */
+  g_assert_cmpint (g_creat (nonce_file, 0600), !=, -1);
 
   g_dbus_server_stop (server);
   g_object_unref (server);
   server = NULL;
 
+  g_assert_false (g_file_test (nonce_file, G_FILE_TEST_EXISTS));
+  g_free (nonce_file);
+
   g_main_loop_quit (service_loop);
   g_thread_join (service_thread);
 
@@ -1616,6 +1697,8 @@ codegen_test_peer (void)
   GVariant            *value;
   const gchar         *s;
 
+  setup_test_address ();
+
   /* bring up a server - we run the server in a different thread to avoid deadlocks */
   service_thread = g_thread_new ("codegen_test_peer",
                                  codegen_service_thread_func,
@@ -1719,6 +1802,8 @@ codegen_test_peer (void)
   g_object_unref (animal1);
   g_object_unref (animal2);
   g_thread_join (service_thread);
+
+  teardown_test_address ();
 }
 
 /* ---------------------------------------------------------------------------------------------------- */
@@ -1730,7 +1815,6 @@ main (int   argc,
 {
   gint ret;
   GDBusNodeInfo *introspection_data = NULL;
-  gchar *tmpdir = NULL;
 
   g_test_init (&argc, &argv, NULL);
 
@@ -1740,19 +1824,6 @@ main (int   argc,
 
   test_guid = g_dbus_generate_guid ();
 
-  if (is_unix)
-    {
-      if (g_unix_socket_address_abstract_names_supported ())
-	tmp_address = g_strdup ("unix:tmpdir=/tmp/gdbus-test-");
-      else
-	{
-	  tmpdir = g_dir_make_tmp ("gdbus-test-XXXXXX", NULL);
-	  tmp_address = g_strdup_printf ("unix:tmpdir=%s", tmpdir);
-	}
-    }
-  else
-    tmp_address = g_strdup ("nonce-tcp:");
-
   /* all the tests rely on a shared main loop */
   loop = g_main_loop_new (NULL, FALSE);
 
@@ -1769,13 +1840,6 @@ main (int   argc,
   g_main_loop_unref (loop);
   g_free (test_guid);
   g_dbus_node_info_unref (introspection_data);
-  if (is_unix)
-    g_free (tmp_address);
-  if (tmpdir)
-    {
-      g_rmdir (tmpdir);
-      g_free (tmpdir);
-    }
 
   return ret;
 }