From 3f4fdb0f6976865f5b72d371421589fab2b63998 Mon Sep 17 00:00:00 2001 From: Niels De Graef Date: Fri, 27 May 2022 11:12:32 +0200 Subject: [PATCH] Replace some SecretSync with a sync implementation The idea behind `SecretSync` is a nice thing: it allows use to re-use our async implementations of methods for the synchronous versions. The big problem with it, is that it makes debugging issues much harder (especially with issues related to freezes) since things can seem to be stuck in `poll()` somewhere. Even though it adds quite a bit of code, I think it makes sense to replace some instances with a proper synchronous implementation. Note that we don't do this for all usages of `SecretSync` though, as some things will need some kind of main loop interaction anyway (for example, when waiting for a portal's Response signal) Another nice advantage is that it's easier to follow the logic in the sync code than the async version. --- libsecret/secret-collection.c | 23 +++---- libsecret/secret-item.c | 122 +++++++++++++++++++++------------ libsecret/secret-paths.c | 123 +++++++++++++++++++++++++++------- libsecret/secret-private.h | 10 +++ libsecret/secret-service.c | 21 ++---- libsecret/secret-session.c | 59 ++++++++++++++++ 6 files changed, 261 insertions(+), 97 deletions(-) diff --git a/libsecret/secret-collection.c b/libsecret/secret-collection.c index 190e2c3..7c5172a 100644 --- a/libsecret/secret-collection.c +++ b/libsecret/secret-collection.c @@ -1705,26 +1705,19 @@ secret_collection_delete_sync (SecretCollection *self, GCancellable *cancellable, GError **error) { - SecretSync *sync; - gboolean ret; + const char *object_path; g_return_val_if_fail (SECRET_IS_COLLECTION (self), FALSE); g_return_val_if_fail (cancellable == NULL || G_IS_CANCELLABLE (cancellable), FALSE); g_return_val_if_fail (error == NULL || *error == NULL, FALSE); - sync = _secret_sync_new (); - g_main_context_push_thread_default (sync->context); - - secret_collection_delete (self, cancellable, _secret_sync_on_result, sync); - - g_main_loop_run (sync->loop); - - ret = secret_collection_delete_finish (self, sync->result, error); - - g_main_context_pop_thread_default (sync->context); - _secret_sync_free (sync); - - return ret; + object_path = g_dbus_proxy_get_object_path (G_DBUS_PROXY (self)); + if (!_secret_service_delete_path_sync (self->pv->service, object_path, FALSE, + cancellable, error)) { + _secret_util_strip_remote_error (error); + return FALSE; + } + return TRUE; } /** diff --git a/libsecret/secret-item.c b/libsecret/secret-item.c index 21b8ecb..baa9476 100644 --- a/libsecret/secret-item.c +++ b/libsecret/secret-item.c @@ -1411,6 +1411,42 @@ on_loads_secrets_session (GObject *source, g_clear_object (&task); } +static LoadsClosure * +load_secrets_prepare (GList *items) +{ + LoadsClosure *ret; + GPtrArray *paths; + + ret = g_new0 (LoadsClosure, 1); + ret->items = g_hash_table_new_full (g_str_hash, g_str_equal, + g_free, g_object_unref); + + paths = g_ptr_array_new (); + for (GList *l = items; l != NULL; l = g_list_next (l)) { + const char *path; + + if (secret_item_get_locked (l->data)) + continue; + + if (ret->service == NULL) { + ret->service = secret_item_get_service (l->data); + if (ret->service) + g_object_ref (ret->service); + } + + path = g_dbus_proxy_get_object_path (l->data); + g_hash_table_insert (ret->items, g_strdup (path), g_object_ref (l->data)); + g_ptr_array_add (paths, (void *) path); + } + + ret->in = g_variant_new_objv ((const char * const *)paths->pdata, paths->len); + g_variant_ref_sink (ret->in); + + g_ptr_array_free (paths, TRUE); + + return ret; +} + /** * secret_item_load_secrets: * @items: (element-type Secret.Item): the items to retrieve secrets for @@ -1430,43 +1466,17 @@ secret_item_load_secrets (GList *items, GAsyncReadyCallback callback, gpointer user_data) { - GTask *task; LoadsClosure *loads; - GPtrArray *paths; - const gchar *path; - GList *l; + GTask *task; + for (GList *l = items; l != NULL; l = g_list_next (l)) + g_return_if_fail (SECRET_IS_ITEM (l->data)); g_return_if_fail (cancellable == NULL || G_IS_CANCELLABLE (cancellable)); - for (l = items; l != NULL; l = g_list_next (l)) - g_return_if_fail (SECRET_IS_ITEM (l->data)); + loads = load_secrets_prepare (items); task = g_task_new (NULL, cancellable, callback, user_data); g_task_set_source_tag (task, secret_item_load_secrets); - loads = g_new0 (LoadsClosure, 1); - loads->items = g_hash_table_new_full (g_str_hash, g_str_equal, - g_free, g_object_unref); - - paths = g_ptr_array_new (); - for (l = items; l != NULL; l = g_list_next (l)) { - if (secret_item_get_locked (l->data)) - continue; - - if (loads->service == NULL) { - loads->service = secret_item_get_service (l->data); - if (loads->service) - g_object_ref (loads->service); - } - - path = g_dbus_proxy_get_object_path (l->data); - g_hash_table_insert (loads->items, g_strdup (path), g_object_ref (l->data)); - g_ptr_array_add (paths, (gpointer)path); - } - - loads->in = g_variant_new_objv ((const gchar * const *)paths->pdata, paths->len); - g_variant_ref_sink (loads->in); - - g_ptr_array_free (paths, TRUE); g_task_set_task_data (task, loads, loads_closure_free); if (loads->service) { @@ -1528,29 +1538,53 @@ secret_item_load_secrets_sync (GList *items, GCancellable *cancellable, GError **error) { - SecretSync *sync; - gboolean ret; - GList *l; - - for (l = items; l != NULL; l = g_list_next (l)) - g_return_val_if_fail (SECRET_IS_ITEM (l->data), FALSE); + gboolean ret = TRUE; + LoadsClosure *loads; + const char *session_path; + GVariant *response; g_return_val_if_fail (cancellable == NULL || G_IS_CANCELLABLE (cancellable), FALSE); + for (GList *l = items; l != NULL; l = g_list_next (l)) + g_return_val_if_fail (SECRET_IS_ITEM (l->data), FALSE); g_return_val_if_fail (error == NULL || *error == NULL, FALSE); - sync = _secret_sync_new (); - g_main_context_push_thread_default (sync->context); + loads = load_secrets_prepare (items); + ret = secret_service_ensure_session_sync (loads->service, cancellable, error); + if (!ret) { + goto out; + } - secret_item_load_secrets (items, cancellable, - _secret_sync_on_result, sync); + session_path = secret_service_get_session_dbus_path (loads->service); + response = g_dbus_proxy_call_sync (G_DBUS_PROXY (loads->service), "GetSecrets", + g_variant_new ("(@aoo)", loads->in, session_path), + G_DBUS_CALL_FLAGS_NO_AUTO_START, -1, + cancellable, error); - g_main_loop_run (sync->loop); + if (response != NULL) { + GHashTable *with_paths; + GHashTableIter iter; + const char *path; + SecretValue *value; + SecretItem *item; - ret = secret_item_load_secrets_finish (sync->result, error); + with_paths = _secret_service_decode_get_secrets_all (loads->service, response); + g_return_val_if_fail (with_paths != NULL, FALSE); - g_main_context_pop_thread_default (sync->context); - _secret_sync_free (sync); + g_hash_table_iter_init (&iter, with_paths); + while (g_hash_table_iter_next (&iter, (void *) &path, (void *) &value)) { + item = g_hash_table_lookup (loads->items, path); + if (item != NULL) + _secret_item_set_cached_secret (item, value); + } + g_hash_table_unref (with_paths); + } + +out: + if (response != NULL) + g_variant_unref (response); + _secret_util_strip_remote_error (error); + loads_closure_free (loads); return ret; } diff --git a/libsecret/secret-paths.c b/libsecret/secret-paths.c index f837bb2..16b446c 100644 --- a/libsecret/secret-paths.c +++ b/libsecret/secret-paths.c @@ -432,28 +432,37 @@ secret_collection_search_for_dbus_paths_sync (SecretCollection *collection, GCancellable *cancellable, GError **error) { - SecretSync *sync; - gchar **paths; + const char *schema_name = NULL; + GVariant *response = NULL; + char **paths = NULL; g_return_val_if_fail (SECRET_IS_COLLECTION (collection), NULL); g_return_val_if_fail (attributes != NULL, NULL); g_return_val_if_fail (cancellable == NULL || G_IS_CANCELLABLE (cancellable), NULL); g_return_val_if_fail (error == NULL || *error == NULL, NULL); - sync = _secret_sync_new (); - g_main_context_push_thread_default (sync->context); + /* Warnings raised already */ + if (schema != NULL && !_secret_attributes_validate (schema, attributes, G_STRFUNC, TRUE)) + return NULL; - secret_collection_search_for_dbus_paths (collection, schema, attributes, cancellable, - _secret_sync_on_result, sync); + if (schema != NULL && !(schema->flags & SECRET_SCHEMA_DONT_MATCH_NAME)) + schema_name = schema->name; - g_main_loop_run (sync->loop); - paths = secret_collection_search_for_dbus_paths_finish (collection, sync->result, error); + response = + g_dbus_proxy_call_sync (G_DBUS_PROXY (collection), "SearchItems", + g_variant_new ("(@a{ss})", + _secret_attributes_to_variant (attributes, schema_name)), + G_DBUS_CALL_FLAGS_NONE, -1, cancellable, error); + if (response == NULL) { + _secret_util_strip_remote_error (error); + return FALSE; + } - g_main_context_pop_thread_default (sync->context); - _secret_sync_free (sync); + g_variant_get (response, "(^ao)", &paths); + g_variant_unref (response); - return paths; + return g_steal_pointer (&paths); } /** @@ -831,28 +840,37 @@ secret_service_get_secret_for_dbus_path_sync (SecretService *self, GCancellable *cancellable, GError **error) { - SecretSync *sync; - SecretValue *value; + const char *session = NULL; + GVariant *response = NULL; + SecretValue *ret = NULL; g_return_val_if_fail (SECRET_IS_SERVICE (self), NULL); g_return_val_if_fail (item_path != NULL, NULL); g_return_val_if_fail (cancellable == NULL || G_IS_CANCELLABLE (cancellable), NULL); g_return_val_if_fail (error == NULL || *error == NULL, NULL); - sync = _secret_sync_new (); - g_main_context_push_thread_default (sync->context); + if (!secret_service_ensure_session_sync (self, cancellable, error)) { + _secret_util_strip_remote_error (error); + return NULL; + } - secret_service_get_secret_for_dbus_path (self, item_path, cancellable, - _secret_sync_on_result, sync); + session = secret_service_get_session_dbus_path (self); + response = + g_dbus_proxy_call_sync (G_DBUS_PROXY (self), "GetSecrets", + g_variant_new ("(@aoo)", + g_variant_new_objv (&item_path, 1), + session), + G_DBUS_CALL_FLAGS_NO_AUTO_START, -1, + cancellable, error); + if (response) { + ret = _secret_service_decode_get_secrets_first (self, response); + } else { + _secret_util_strip_remote_error (error); + } - g_main_loop_run (sync->loop); + g_clear_pointer (&response, g_variant_unref); - value = secret_service_get_secret_for_dbus_path_finish (self, sync->result, error); - - g_main_context_pop_thread_default (sync->context); - _secret_sync_free (sync); - - return value; + return ret; } /** @@ -1527,6 +1545,63 @@ _secret_service_delete_path_finish (SecretService *self, return TRUE; } +gboolean +_secret_service_delete_path_sync (SecretService *self, + const char *object_path, + gboolean is_an_item, + GCancellable *cancellable, + GError **error) +{ + GError *err = NULL; + GVariant *prompt_variant = NULL; + gboolean ret = TRUE; + + g_return_val_if_fail (SECRET_IS_SERVICE (self), FALSE); + g_return_val_if_fail (object_path != NULL, FALSE); + g_return_val_if_fail (cancellable == NULL || G_IS_CANCELLABLE (cancellable), FALSE); + + prompt_variant = + g_dbus_connection_call_sync (g_dbus_proxy_get_connection (G_DBUS_PROXY (self)), + g_dbus_proxy_get_name (G_DBUS_PROXY (self)), + object_path, + is_an_item ? SECRET_ITEM_INTERFACE : SECRET_COLLECTION_INTERFACE, + "Delete", g_variant_new ("()"), G_VARIANT_TYPE ("(o)"), + G_DBUS_CALL_FLAGS_NO_AUTO_START, -1, + cancellable, + &err); + if (err == NULL) { + const char *prompt_path; + + g_variant_get (prompt_variant, "(&o)", &prompt_path); + + /* show a prompt if needed */ + if (!_secret_util_empty_path (prompt_path)) { + SecretPrompt *prompt = NULL; + GVariant *prompt_result = NULL; + + prompt = _secret_prompt_instance (self, prompt_path); + prompt_result = secret_service_prompt_sync (self, + prompt, + cancellable, + NULL, + &err); + ret = (err != NULL); + g_clear_pointer (&prompt_result, g_variant_unref); + g_clear_object (&prompt); + } + + } else { + ret = FALSE; + } + + g_clear_pointer (&prompt_variant, g_variant_unref); + if (err) { + _secret_util_strip_remote_error (&err); + g_propagate_error (error, err); + } + return ret; +} + /** * secret_service_delete_item_dbus_path: (skip) * @self: the secret service diff --git a/libsecret/secret-private.h b/libsecret/secret-private.h index 87633f6..852a37d 100644 --- a/libsecret/secret-private.h +++ b/libsecret/secret-private.h @@ -140,6 +140,12 @@ gboolean _secret_service_delete_path_finish (SecretService *se GAsyncResult *result, GError **error); +gboolean _secret_service_delete_path_sync (SecretService *self, + const char *object_path, + gboolean is_an_item, + GCancellable *cancellable, + GError **error); + void _secret_service_search_for_paths_variant (SecretService *self, GVariant *attributes, GCancellable *cancellable, @@ -198,6 +204,10 @@ void _secret_session_open (SecretService *se gboolean _secret_session_open_finish (GAsyncResult *result, GError **error); +gboolean _secret_session_open_sync (SecretService *service, + GCancellable *cancellable, + GError **error); + GVariant * _secret_session_encode_secret (SecretSession *session, SecretValue *value); diff --git a/libsecret/secret-service.c b/libsecret/secret-service.c index 63024a7..e480c48 100644 --- a/libsecret/secret-service.c +++ b/libsecret/secret-service.c @@ -1544,27 +1544,20 @@ secret_service_ensure_session_sync (SecretService *self, GCancellable *cancellable, GError **error) { - SecretSync *sync; - gboolean ret; + SecretSession *session; g_return_val_if_fail (SECRET_IS_SERVICE (self), FALSE); g_return_val_if_fail (cancellable == NULL || G_IS_CANCELLABLE (cancellable), FALSE); g_return_val_if_fail (error == NULL || *error == NULL, FALSE); - sync = _secret_sync_new (); - g_main_context_push_thread_default (sync->context); + g_mutex_lock (&self->pv->mutex); + session = self->pv->session; + g_mutex_unlock (&self->pv->mutex); - secret_service_ensure_session (self, cancellable, - _secret_sync_on_result, sync); + if (session != NULL) + return TRUE; - g_main_loop_run (sync->loop); - - ret = secret_service_ensure_session_finish (self, sync->result, error); - - g_main_context_pop_thread_default (sync->context); - _secret_sync_free (sync); - - return ret; + return _secret_session_open_sync (self, cancellable, error); } static SecretCollection * diff --git a/libsecret/secret-session.c b/libsecret/secret-session.c index 8b52e58..bb72480 100644 --- a/libsecret/secret-session.c +++ b/libsecret/secret-session.c @@ -363,6 +363,65 @@ _secret_session_open_finish (GAsyncResult *result, return TRUE; } +gboolean +_secret_session_open_sync (SecretService *service, + GCancellable *cancellable, + GError **error) +{ + SecretSession *session = NULL; + GVariant *params; + GVariant *result = NULL; + + session = g_new0 (SecretSession, 1); + +#ifdef WITH_CRYPTO + params = request_open_session_aes (session); + result = g_dbus_proxy_call_sync (G_DBUS_PROXY (service), "OpenSession", + params, + G_DBUS_CALL_FLAGS_NONE, -1, + cancellable, error); + if (result != NULL) { + if (response_open_session_aes (session, result)) { + _secret_service_take_session (service, g_steal_pointer (&session)); + goto out; + } else { + g_set_error (error, SECRET_ERROR, SECRET_ERROR_PROTOCOL, + _("Couldn’t communicate with the secret storage")); + goto out; + } + } else { + /* AES session not supported, request a plain session */ + if (g_error_matches (*error, G_DBUS_ERROR, G_DBUS_ERROR_NOT_SUPPORTED)) { + g_clear_error (error); + /* Fall through */ + } else { + goto out; + } + } +#endif + + params = request_open_session_plain (session); + result = g_dbus_proxy_call_sync (G_DBUS_PROXY (service), "OpenSession", + params, + G_DBUS_CALL_FLAGS_NONE, -1, + cancellable, error); + if (result != NULL) { + if (response_open_session_plain (session, result)) { + _secret_service_take_session (service, g_steal_pointer (&session)); + goto out; + } else { + g_set_error (error, SECRET_ERROR, SECRET_ERROR_PROTOCOL, + _("Couldn’t communicate with the secret storage")); + goto out; + } + } + +out: + _secret_util_strip_remote_error (error); + g_clear_object (&session); + return result != NULL; +} + #ifdef WITH_CRYPTO static gboolean