From 21009af72d8ee615336eae7527e674f2984f9d5f Mon Sep 17 00:00:00 2001 From: Cosimo Alfarano Date: Fri, 7 Oct 2011 16:46:44 +0100 Subject: [PATCH 7/8] 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) --- dbus/dbus-auth-script.c | 25 +++----------- dbus/dbus-auth.c | 81 +++++++++++++++----------------------------- dbus/dbus-auth.h | 5 +-- dbus/dbus-authorization.c | 72 +++++++++++++++------------------------- dbus/dbus-authorization.h | 3 +- dbus/dbus-transport.c | 18 +++++----- 6 files changed, 72 insertions(+), 132 deletions(-) diff --git a/dbus/dbus-auth-script.c b/dbus/dbus-auth-script.c index 6bf98e3..b9b28e6 100644 --- 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 d12a1eb..b57255c 100644 --- 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; @@ -2279,7 +2276,7 @@ process_command (DBusAuth *auth) */ DBusAuth* _dbus_auth_server_new (const DBusString *guid, - DBusAuthorization *authorization) + DBusAuthorization *authorization) { DBusAuth *auth; DBusAuthServer *server_auth; @@ -2307,7 +2304,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 @@ -2350,7 +2347,7 @@ _dbus_auth_client_new (void) * mechanism */ if (!send_auth (auth, &all_mechanisms[0])) { - _dbus_auth_unref (auth); + _dbus_auth_free (auth); return NULL; } @@ -2358,67 +2355,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 a6577f9..f1e6a9a 100644 --- a/dbus/dbus-auth.h +++ b/dbus/dbus-auth.h @@ -43,10 +43,9 @@ typedef enum } DBusAuthState; DBusAuth* _dbus_auth_server_new (const DBusString *guid, - DBusAuthorization *authorization); + 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 3c43989..4444575 100644 --- a/dbus/dbus-authorization.c +++ b/dbus/dbus-authorization.c @@ -29,8 +29,6 @@ #include "dbus-connection-internal.h" struct DBusAuthorization { - int refcount; - DBusConnection *connection; /* User'sauthorization functions, used as callback by SASL (implemented by @@ -50,58 +48,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 */ @@ -109,6 +80,7 @@ void _dbus_authorization_set_connection (DBusAuthorization *self, DBusConnection *connection) { + _dbus_assert (self != NULL); _dbus_assert (connection != NULL); _dbus_assert (self->connection == NULL); @@ -139,6 +111,8 @@ _dbus_authorization_set_unix_authorization_callback (DBusAuthorization *s 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; @@ -172,6 +146,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; @@ -190,6 +166,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); _dbus_assert (self->connection != NULL); @@ -273,6 +250,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 either @@ -331,6 +309,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 @@ -360,5 +340,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 504a9c6..9c27119 100644 --- a/dbus/dbus-authorization.h +++ b/dbus/dbus-authorization.h @@ -33,8 +33,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, diff --git a/dbus/dbus-transport.c b/dbus/dbus-transport.c index cc21786..f427ad3 100644 --- a/dbus/dbus-transport.c +++ b/dbus/dbus-transport.c @@ -135,7 +135,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; } @@ -143,9 +143,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; } @@ -154,9 +154,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; } @@ -174,9 +174,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; } @@ -238,9 +238,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); -- 1.7.6.3