From 2252008c22abf69dd3d1c8942369017e89161fde Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Thu, 18 Jun 2020 13:17:46 +0100 Subject: act-user-manager: Fix various refcounting bugs The refcounting of `ActUser` instances was a bit jumbled and unclear, and seemed to contain several bugs. In particular, `act_user_manager_get_user_by_id()` was behaving as `(transfer full)` when it was documented as `(transfer none)`. Try and tidy the refcounting up, to a certain extent. There may still be issues left. Signed-off-by: Philip Withnall --- src/libaccountsservice/act-user-manager.c | 55 +++++++++++++++---------------- 1 file changed, 27 insertions(+), 28 deletions(-) diff --git a/src/libaccountsservice/act-user-manager.c b/src/libaccountsservice/act-user-manager.c index 4f945e9..f32a06b 100644 --- a/src/libaccountsservice/act-user-manager.c +++ b/src/libaccountsservice/act-user-manager.c @@ -153,7 +153,7 @@ typedef struct { GHashTable *normal_users_by_name; GHashTable *system_users_by_name; - GHashTable *users_by_object_path; + GHashTable *users_by_object_path; /* (element-type utf8 ActUser) (owned) */ GHashTable *sessions; GDBusConnection *connection; AccountsAccounts *accounts_proxy; @@ -161,8 +161,8 @@ typedef struct ActUserManagerSeat seat; GSList *new_sessions; - GSList *new_users; - GSList *new_users_inhibiting_load; + GSList *new_users; /* (element-type ActUser) (owned) */ + GSList *new_users_inhibiting_load; /* (element-type ActUser) (unowned) */ GSList *fetch_user_requests; GSList *exclude_usernames; @@ -696,6 +696,7 @@ set_has_multiple_users (ActUserManager *manager, } } +/* (transfer full) */ static ActUser * create_new_user (ActUserManager *manager) { @@ -822,6 +823,7 @@ update_user (ActUserManager *manager, } } +/* (transfer none) */ static ActUser * lookup_user_by_name (ActUserManager *manager, const char *username) @@ -911,6 +913,7 @@ out: } } +/* (transfer none) */ static ActUser * find_new_user_with_object_path (ActUserManager *manager, const char *object_path) @@ -931,6 +934,7 @@ find_new_user_with_object_path (ActUserManager *manager, return NULL; } +/* (transfer full) */ static ActUser * add_new_user_for_object_path (const char *object_path, ActUserManager *manager) @@ -943,7 +947,7 @@ add_new_user_for_object_path (const char *object_path, if (user != NULL) { g_debug ("ActUserManager: tracking existing %s with object path %s", describe_user (user), object_path); - return user; + return g_object_ref (user); } user = find_new_user_with_object_path (manager, object_path); @@ -951,7 +955,7 @@ add_new_user_for_object_path (const char *object_path, if (user != NULL) { g_debug ("ActUserManager: tracking existing (but very recently added) %s with object path %s", describe_user (user), object_path); - return user; + return g_object_ref (user); } g_debug ("ActUserManager: tracking new user with object path %s", object_path); @@ -1224,7 +1228,7 @@ load_user_paths (ActUserManager *manager, g_debug ("ActUserManager: ListCachedUsers finished, will set loaded property after list is fully loaded"); for (i = 0; user_paths[i] != NULL; i++) { - ActUser *user; + g_autoptr(ActUser) user = NULL; user = add_new_user_for_object_path (user_paths[i], manager); if (!priv->is_loaded) { @@ -1826,6 +1830,8 @@ act_user_manager_get_user (ActUserManager *manager, if (priv->accounts_proxy != NULL) { fetch_user_with_username_from_accounts_service (manager, user, username); } + + g_object_unref (user); /* remains in the cache */ } return user; @@ -1836,7 +1842,7 @@ load_user (ActUserManager *manager, const char *username) { ActUserManagerPrivate *priv = act_user_manager_get_instance_private (manager); - ActUser *user; + g_autoptr(ActUser) user = NULL; g_autoptr(GError) error = NULL; char *object_path = NULL; gboolean user_found; @@ -1849,6 +1855,8 @@ load_user (ActUserManager *manager, if (user == NULL) { g_debug ("ActUserManager: trying to track new user with username %s", username); user = create_new_user (manager); + } else { + user = g_object_ref (user); } user_found = accounts_accounts_call_find_user_by_name_sync (priv->accounts_proxy, @@ -1898,7 +1906,7 @@ act_user_manager_get_user_by_id (ActUserManager *manager, user = g_hash_table_lookup (priv->users_by_object_path, object_path); if (user != NULL) { - return g_object_ref (user); + return user; } else { g_debug ("ActUserManager: trying to track new user with uid %lu", (gulong) id); user = create_new_user (manager); @@ -1906,6 +1914,8 @@ act_user_manager_get_user_by_id (ActUserManager *manager, if (priv->accounts_proxy != NULL) { fetch_user_with_id_from_accounts_service (manager, user, id); } + + g_object_unref (user); /* remains in the cache */ } return user; @@ -2543,7 +2553,6 @@ act_user_manager_create_user (ActUserManager *manager, GError *local_error = NULL; gboolean res; g_autofree gchar *path = NULL; - ActUser *user; g_debug ("ActUserManager: Creating user '%s', '%s', %d", username, fullname, accounttype); @@ -2564,9 +2573,7 @@ act_user_manager_create_user (ActUserManager *manager, return NULL; } - user = add_new_user_for_object_path (path, manager); - - return user; + return add_new_user_for_object_path (path, manager); } static void @@ -2654,7 +2661,6 @@ act_user_manager_create_user_finish (ActUserManager *manager, { ActUserManagerPrivate *priv = act_user_manager_get_instance_private (manager); GAsyncResult *inner_result; - ActUser *user = NULL; g_autofree gchar *path = NULL; GError *remote_error = NULL; @@ -2663,17 +2669,14 @@ act_user_manager_create_user_finish (ActUserManager *manager, return NULL; } - if (accounts_accounts_call_create_user_finish (priv->accounts_proxy, - &path, inner_result, &remote_error)) { - user = add_new_user_for_object_path (path, manager); - } - - if (remote_error) { + if (!accounts_accounts_call_create_user_finish (priv->accounts_proxy, + &path, inner_result, &remote_error)) { g_dbus_error_strip_remote_error (remote_error); g_propagate_error (error, remote_error); + return NULL; } - return user; + return add_new_user_for_object_path (path, manager); } /** @@ -2783,7 +2786,6 @@ act_user_manager_cache_user_finish (ActUserManager *manager, { ActUserManagerPrivate *priv = act_user_manager_get_instance_private (manager); GAsyncResult *inner_result; - ActUser *user = NULL; g_autofree gchar *path = NULL; GError *remote_error = NULL; @@ -2792,17 +2794,14 @@ act_user_manager_cache_user_finish (ActUserManager *manager, return NULL; } - if (accounts_accounts_call_cache_user_finish (priv->accounts_proxy, - &path, inner_result, &remote_error)) { - user = add_new_user_for_object_path (path, manager); - } - - if (remote_error) { + if (!accounts_accounts_call_cache_user_finish (priv->accounts_proxy, + &path, inner_result, &remote_error)) { g_dbus_error_strip_remote_error (remote_error); g_propagate_error (error, remote_error); + return NULL; } - return user; + return add_new_user_for_object_path (path, manager); } /** -- cgit v1.2.3