From: Cosimo Alfarano Date: Fri, 23 Aug 2013 00:12:46 +0000 (+0200) Subject: Remove refcounting from DBusAuth and DBusAuthorization X-Git-Tag: dbus-1.7.6~109 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=7f6d7229d8812d985d544cf5dd3636865c5abc81;p=thirdparty%2Fdbus.git 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 --- diff --git a/dbus/dbus-auth-script.c b/dbus/dbus-auth-script.c index 107c92b23..d195dde50 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 a21870167..3da25ec51 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 3f178a227..1cf0570ab 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 05a3aa878..3f0b9660d 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 8f7f1d439..eca81d80a 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 3a9cf84b4..db16574a1 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);