From 7f6d7229d8812d985d544cf5dd3636865c5abc81 Mon Sep 17 00:00:00 2001 From: Cosimo Alfarano Date: Fri, 23 Aug 2013 02:12:46 +0200 Subject: [PATCH] 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 --- dbus/dbus-auth-script.c | 25 +++------------ dbus/dbus-auth.c | 81 ++++++++++++++++------------------------------- dbus/dbus-auth.h | 3 +- dbus/dbus-authorization.c | 73 ++++++++++++++++-------------------------- dbus/dbus-authorization.h | 3 +- dbus/dbus-transport.c | 18 +++++------ 6 files changed, 72 insertions(+), 131 deletions(-) diff --git a/dbus/dbus-auth-script.c b/dbus/dbus-auth-script.c index 107c92b..d195dde 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 a218701..3da25ec 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; @@ -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 --- 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 --- 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 --- 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 --- 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); -- 2.7.4