Remove refcounting from DBusAuth and DBusAuthorization

Those structs are for DBusTransport internal use, they should not be
referenced outside it.
The transport needs only to allocate memory on initialization and free
it on finalization.
The lifecycle for the two allocated structs is DBusTransport lifecycle
and at DBusTransport's finalization its connection is already
disconnected.
The assumption is that the transport owns a reference for any object the
two structs holds a reference for (particularly DBusConnection)

Bug: http://bugs.freedesktop.org/show_bug.cgi?id=39720
Reviewed-by: Simon McVittie <simon.mcvittie@collabora.co.uk>

diff --git a/dbus/dbus-auth-script.c b/dbus/dbus-auth-script.c
index 107c92b..d195dde 100644 (file)
--- a/dbus/dbus-auth-script.c
+++ b/dbus/dbus-auth-script.c
@@ -250,6 +250,7 @@ _dbus_auth_script_run (const DBusString *filename)
   dbus_bool_t retval;
   int line_no;
   DBusAuth *auth;
+  DBusAuthorization *authorization;
   DBusString from_auth;
   DBusAuthState state;
   DBusString context;
@@ -257,6 +258,7 @@ _dbus_auth_script_run (const DBusString *filename)
   
   retval = FALSE;
   auth = NULL;
+  authorization = NULL;
 
   _dbus_string_init_const (&guid, "5fa01f4202cd837709a3274ca0df9d00");
   _dbus_string_init_const (&context, "org_freedesktop_test");
@@ -374,24 +376,16 @@ _dbus_auth_script_run (const DBusString *filename)
               goto out;
             }
 
-          /* test ref/unref */
-          _dbus_auth_ref (auth);
-          _dbus_auth_unref (auth);
-
           creds = _dbus_credentials_new_from_current_process ();
           if (creds == NULL)
             {
               _dbus_warn ("no memory for credentials\n");
-              _dbus_auth_unref (auth);
-              auth = NULL;
               goto out;
             }
               
           if (!_dbus_auth_set_credentials (auth, creds))
             {
               _dbus_warn ("no memory for setting credentials\n");
-              _dbus_auth_unref (auth);
-              auth = NULL;
               _dbus_credentials_unref (creds);
               goto out;
             }
@@ -402,7 +396,6 @@ _dbus_auth_script_run (const DBusString *filename)
                _dbus_string_starts_with_c_str (&line, "SERVER_ANONYMOUS"))
         {
           DBusCredentials *creds;
-          DBusAuthorization *authorization;
           
           if (auth != NULL)
             {
@@ -423,32 +416,22 @@ _dbus_auth_script_run (const DBusString *filename)
             _dbus_authorization_set_allow_anonymous (authorization, TRUE);
 
           auth = _dbus_auth_server_new (&guid, authorization);
-          /* DBusAuth owns it, or finalized on OOM */
-          _dbus_authorization_unref (authorization);
           if (auth == NULL)
             {
               _dbus_warn ("no memory to create DBusAuth\n");
               goto out;
             }
 
-          /* test ref/unref */
-          _dbus_auth_ref (auth);
-          _dbus_auth_unref (auth);
-
           creds = _dbus_credentials_new_from_current_process ();
           if (creds == NULL)
             {
               _dbus_warn ("no memory for credentials\n");
-              _dbus_auth_unref (auth);
-              auth = NULL;
               goto out;
             }
               
           if (!_dbus_auth_set_credentials (auth, creds))
             {
               _dbus_warn ("no memory for setting credentials\n");
-              _dbus_auth_unref (auth);
-              auth = NULL;
               _dbus_credentials_unref (creds);
               goto out;
             }
@@ -806,7 +789,9 @@ _dbus_auth_script_run (const DBusString *filename)
   
  out:
   if (auth)
-    _dbus_auth_unref (auth);
+    _dbus_auth_free (auth);
+  if (authorization)
+    _dbus_authorization_free (authorization);
 
   _dbus_string_free (&file);
   _dbus_string_free (&line);

diff --git a/dbus/dbus-auth.c b/dbus/dbus-auth.c
index a218701..3da25ec 100644 (file)
--- a/dbus/dbus-auth.c
+++ b/dbus/dbus-auth.c
@@ -153,7 +153,6 @@ typedef struct
  */
 struct DBusAuth
 {
-  int refcount;           /**< reference count */
   const char *side;       /**< Client or server */
 
   DBusString incoming;    /**< Incoming data buffer */
@@ -346,8 +345,6 @@ _dbus_auth_new (int size)
   if (auth == NULL)
     return NULL;
   
-  auth->refcount = 1;
-  
   auth->keyring = NULL;
   auth->cookie_id = -1;
   
@@ -2262,7 +2259,7 @@ process_command (DBusAuth *auth)
  */
 DBusAuth*
 _dbus_auth_server_new (const DBusString *guid,
-    DBusAuthorization *authorization)
+                       DBusAuthorization *authorization)
 {
   DBusAuth *auth;
   DBusAuthServer *server_auth;
@@ -2290,7 +2287,7 @@ _dbus_auth_server_new (const DBusString *guid,
   server_auth = DBUS_AUTH_SERVER (auth);
 
   server_auth->guid = guid_copy;
-  server_auth->authorization = _dbus_authorization_ref (authorization);
+  server_auth->authorization = authorization;
 
   /* perhaps this should be per-mechanism with a lower
    * max
@@ -2333,7 +2330,7 @@ _dbus_auth_client_new (void)
    * mechanism */
   if (!send_auth (auth, &all_mechanisms[0]))
     {
-      _dbus_auth_unref (auth);
+      _dbus_auth_free (auth);
       return NULL;
     }
   
@@ -2341,67 +2338,45 @@ _dbus_auth_client_new (void)
 }
 
 /**
- * Increments the refcount of an auth object.
- *
- * @param auth the auth conversation
- * @returns the auth conversation
- */
-DBusAuth *
-_dbus_auth_ref (DBusAuth *auth)
-{
-  _dbus_assert (auth != NULL);
-  
-  auth->refcount += 1;
-  
-  return auth;
-}
-
-/**
- * Decrements the refcount of an auth object.
+ * Free memory allocated for an auth object.
  *
  * @param auth the auth conversation
  */
 void
-_dbus_auth_unref (DBusAuth *auth)
+_dbus_auth_free (DBusAuth *auth)
 {
   _dbus_assert (auth != NULL);
-  _dbus_assert (auth->refcount > 0);
 
-  auth->refcount -= 1;
-  if (auth->refcount == 0)
+  shutdown_mech (auth);
+
+  if (DBUS_AUTH_IS_CLIENT (auth))
     {
-      shutdown_mech (auth);
+      _dbus_string_free (& DBUS_AUTH_CLIENT (auth)->guid_from_server);
+      _dbus_list_clear (& DBUS_AUTH_CLIENT (auth)->mechs_to_try);
+    }
+  else
+    {
+      _dbus_assert (DBUS_AUTH_IS_SERVER (auth));
 
-      if (DBUS_AUTH_IS_CLIENT (auth))
-        {
-          _dbus_string_free (& DBUS_AUTH_CLIENT (auth)->guid_from_server);
-          _dbus_list_clear (& DBUS_AUTH_CLIENT (auth)->mechs_to_try);
-        }
-      else
-        {
-          _dbus_assert (DBUS_AUTH_IS_SERVER (auth));
+      _dbus_string_free (& DBUS_AUTH_SERVER (auth)->guid);
+    }
 
-          _dbus_string_free (& DBUS_AUTH_SERVER (auth)->guid);
-          _dbus_authorization_unref (DBUS_AUTH_SERVER (auth)->authorization);
-        }
+  if (auth->keyring)
+    _dbus_keyring_unref (auth->keyring);
 
-      if (auth->keyring)
-        _dbus_keyring_unref (auth->keyring);
+  _dbus_string_free (&auth->context);
+  _dbus_string_free (&auth->challenge);
+  _dbus_string_free (&auth->identity);
+  _dbus_string_free (&auth->incoming);
+  _dbus_string_free (&auth->outgoing);
 
-      _dbus_string_free (&auth->context);
-      _dbus_string_free (&auth->challenge);
-      _dbus_string_free (&auth->identity);
-      _dbus_string_free (&auth->incoming);
-      _dbus_string_free (&auth->outgoing);
+  dbus_free_string_array (auth->allowed_mechs);
 
-      dbus_free_string_array (auth->allowed_mechs);
+  _dbus_credentials_unref (auth->credentials);
+  _dbus_credentials_unref (auth->authenticated_identity);
+  _dbus_credentials_unref (auth->desired_identity);
 
-      _dbus_credentials_unref (auth->credentials);
-      _dbus_credentials_unref (auth->authenticated_identity);
-      _dbus_credentials_unref (auth->desired_identity);
-      
-      dbus_free (auth);
-    }
+  dbus_free (auth);
 }
 
 /**

diff --git a/dbus/dbus-auth.h b/dbus/dbus-auth.h
index 3f178a2..1cf0570 100644 (file)
--- a/dbus/dbus-auth.h
+++ b/dbus/dbus-auth.h
@@ -45,8 +45,7 @@ typedef enum
 DBusAuth*     _dbus_auth_server_new          (const DBusString       *guid,
                                               DBusAuthorization      *authorization);
 DBusAuth*     _dbus_auth_client_new          (void);
-DBusAuth*     _dbus_auth_ref                 (DBusAuth               *auth);
-void          _dbus_auth_unref               (DBusAuth               *auth);
+void          _dbus_auth_free                (DBusAuth               *auth);
 dbus_bool_t   _dbus_auth_set_mechanisms      (DBusAuth               *auth,
                                               const char            **mechanisms);
 DBusAuthState _dbus_auth_do_work             (DBusAuth               *auth);

diff --git a/dbus/dbus-authorization.c b/dbus/dbus-authorization.c
index 05a3aa8..3f0b966 100644 (file)
--- a/dbus/dbus-authorization.c
+++ b/dbus/dbus-authorization.c
@@ -5,8 +5,6 @@
 #include "dbus-connection-internal.h"
 
 struct DBusAuthorization {
-  int refcount;
-
   DBusConnection *connection;
 
   /* Authorization functions, used as callback by SASL (implemented by
@@ -26,58 +24,31 @@ struct DBusAuthorization {
 DBusAuthorization *
 _dbus_authorization_new (void)
 {
-  DBusAuthorization *ret;
-
-  ret = dbus_malloc0 (sizeof (DBusAuthorization));
-  if (ret == NULL)
-    {
-      _dbus_verbose ("OOM\n");
-      return NULL; /* OOM */
-    }
-
-  ret->refcount = 1;
-
-  return ret;
-}
-
-DBusAuthorization *
-_dbus_authorization_ref (DBusAuthorization *self)
-{
-  _dbus_assert (self != NULL);
-
-  self->refcount += 1;
-
-  return self;
+  /* it returns the allocated memory or NULL in case of OOM */
+  return dbus_malloc0 (sizeof (DBusAuthorization));
 }
 
 void
-_dbus_authorization_unref (DBusAuthorization *self)
+_dbus_authorization_free (DBusAuthorization *self)
 {
   _dbus_assert (self != NULL);
-  _dbus_assert (self->refcount > 0);
 
-  self->refcount -= 1;
+  if (self->unix_data && self->unix_data_free)
+    {
+      _dbus_verbose ("freeing unix authorization callback data\n");
+      (*self->unix_data_free) (self->unix_data);
+      self->unix_data = NULL;
+    }
 
-  if (self->refcount == 0)
+  if (self->windows_data && self->windows_data_free)
     {
-      _dbus_verbose ("last reference, finalizing\n");
-
-      if (self->unix_data && self->unix_data_free)
-        {
-          _dbus_verbose ("freeing unix authorization callback data\n");
-          (*self->unix_data_free) (self->unix_data);
-          self->unix_data = NULL;
-        }
-
-      if (self->windows_data && self->windows_data_free)
-        {
-          _dbus_verbose ("freeing windows authorization callback data\n");
-          (*self->windows_data_free) (self->windows_data);
-          self->windows_data = NULL;
-        }
-
-      dbus_free (self);
+      _dbus_verbose ("freeing windows authorization callback data\n");
+      (*self->windows_data_free) (self->windows_data);
+      self->windows_data = NULL;
     }
+
+  _dbus_verbose ("freeing memory for %p\n", self);
+  dbus_free (self);
 }
 
 /* Called by transport's set_connection with the connection locked */
@@ -85,6 +56,7 @@ void
 _dbus_authorization_set_connection (DBusAuthorization *self,
                                 DBusConnection *connection)
 {
+  _dbus_assert (self != NULL);
   _dbus_assert (connection != NULL);
   _dbus_assert (self->connection == NULL);
 
@@ -115,6 +87,8 @@ _dbus_authorization_set_unix_authorization_callback (DBusAuthorization
                                         void                     **old_data,
                                         DBusFreeFunction          *old_free_data_function)
 {
+  _dbus_assert (self != NULL);
+
   *old_data = self->unix_data;
   *old_free_data_function = self->unix_data_free;
 
@@ -148,6 +122,8 @@ _dbus_authorization_set_windows_authorization_callback (DBusAuthorization
                                            void                      **old_data,
                                            DBusFreeFunction           *old_free_data_function)
 {
+  _dbus_assert (self != NULL);
+
   *old_data = self->windows_data;
   *old_free_data_function = self->windows_data_free;
 
@@ -166,6 +142,7 @@ auth_via_unix_authorization_callback (DBusAuthorization *self,
 
   /* Dropping the lock here probably isn't that safe. */
 
+  _dbus_assert (self != NULL);
   _dbus_assert (auth_identity != NULL);
 
   uid = _dbus_credentials_get_unix_uid (auth_identity);
@@ -203,6 +180,7 @@ auth_via_windows_authorization_callback (DBusAuthorization *self,
 
   /* Dropping the lock here probably isn't that safe. */
 
+  _dbus_assert (self != NULL);
   _dbus_assert (auth_identity != NULL);
 
   windows_sid = _dbus_strdup (_dbus_credentials_get_windows_sid (auth_identity));
@@ -243,6 +221,7 @@ auth_via_default_rules (DBusAuthorization *self,
   DBusCredentials *our_identity;
   dbus_bool_t allow;
 
+  _dbus_assert (self != NULL);
   _dbus_assert (auth_identity != NULL);
 
   /* By default, connection is allowed if the client is 1) root or 2)
@@ -299,6 +278,8 @@ _dbus_authorization_do_authorization (DBusAuthorization *self,
 {
   dbus_bool_t allow;
 
+  _dbus_assert (self != NULL);
+
   /* maybe-FIXME: at this point we *should* have a connection set unless we
    * are in some test case, but we assert its presence only in some if's
    * branches since default_rules does not need one and is used in a test case
@@ -340,5 +321,7 @@ void
 _dbus_authorization_set_allow_anonymous (DBusAuthorization *self,
                                      dbus_bool_t value)
 {
+  _dbus_assert (self != NULL);
+
   self->allow_anonymous = value != FALSE;
 }

diff --git a/dbus/dbus-authorization.h b/dbus/dbus-authorization.h
index 8f7f1d4..eca81d8 100644 (file)
--- a/dbus/dbus-authorization.h
+++ b/dbus/dbus-authorization.h
@@ -9,8 +9,7 @@ typedef struct DBusAuthorization DBusAuthorization;
 DBusAuthorization *_dbus_authorization_new (void);
 void _dbus_authorization_set_connection (DBusAuthorization *self,
     DBusConnection *connection);
-DBusAuthorization * _dbus_authorization_ref (DBusAuthorization *self);
-void _dbus_authorization_unref (DBusAuthorization *self);
+void _dbus_authorization_free (DBusAuthorization *self);
 void _dbus_authorization_set_unix_authorization_callback (DBusAuthorization *self,
     DBusAllowUnixUserFunction function, void *data,
     DBusFreeFunction free_data_function, void **old_data,

diff --git a/dbus/dbus-transport.c b/dbus/dbus-transport.c
index 3a9cf84..db16574 100644 (file)
--- a/dbus/dbus-transport.c
+++ b/dbus/dbus-transport.c
@@ -134,7 +134,7 @@ _dbus_transport_init_base (DBusTransport             *transport,
   if (auth == NULL)
     {
       if (authorization != NULL)
-        _dbus_authorization_unref (authorization);
+        _dbus_authorization_free (authorization);
       _dbus_message_loader_unref (loader);
       return FALSE;
     }
@@ -142,9 +142,9 @@ _dbus_transport_init_base (DBusTransport             *transport,
   counter = _dbus_counter_new ();
   if (counter == NULL)
     {
-      _dbus_auth_unref (auth);
+      _dbus_auth_free (auth);
       if (authorization != NULL)
-        _dbus_authorization_unref (authorization);
+        _dbus_authorization_free (authorization);
       _dbus_message_loader_unref (loader);
       return FALSE;
     }  
@@ -153,9 +153,9 @@ _dbus_transport_init_base (DBusTransport             *transport,
   if (creds == NULL)
     {
       _dbus_counter_unref (counter);
-      _dbus_auth_unref (auth);
+      _dbus_auth_free (auth);
       if (authorization != NULL)
-        _dbus_authorization_unref (authorization);
+        _dbus_authorization_free (authorization);
       _dbus_message_loader_unref (loader);
       return FALSE;
     }
@@ -173,9 +173,9 @@ _dbus_transport_init_base (DBusTransport             *transport,
         {
           _dbus_credentials_unref (creds);
           _dbus_counter_unref (counter);
-          _dbus_auth_unref (auth);
+          _dbus_auth_free (auth);
           if (authorization != NULL)
-            _dbus_authorization_unref (authorization);
+            _dbus_authorization_free (authorization);
           _dbus_message_loader_unref (loader);
           return FALSE;
         }
@@ -237,9 +237,9 @@ _dbus_transport_finalize_base (DBusTransport *transport)
     _dbus_transport_disconnect (transport);
 
   _dbus_message_loader_unref (transport->loader);
-  _dbus_auth_unref (transport->auth);
+  _dbus_auth_free (transport->auth);
   if (transport->authorization)
-    _dbus_authorization_unref (transport->authorization);
+    _dbus_authorization_free (transport->authorization);
   _dbus_counter_set_notify (transport->live_messages,
                             0, 0, NULL, NULL);
   _dbus_counter_unref (transport->live_messages);