From 63f8294c453df0c39eee60288e161676dfaec9c2 Mon Sep 17 00:00:00 2001 From: Will Thompson Date: Thu, 20 Sep 2018 16:56:26 +0100 Subject: [PATCH 1/2] gkeyfile: remain usable after g_key_file_free() Previously, in the case where 'kf' has more than one ref, calling g_key_file_free(kf) would break it. For example, calling g_key_file_has_key(kf, ...) would hit the following assertion: g_hash_table_lookup: assertion 'hash_table != NULL' failed This is because g_key_file_free() calls g_key_file_clear() which sets self->groups and other fields to NULL; most lookup functions assume these fields are non-NULL. One fix would be to call g_key_file_init() right after g_key_file_clear() in g_key_file_free(). However, in the case where there are no other refs to the keyfile, this would mean allocating many new hash tables which will be immediately destroyed when g_key_file_unref() removes the last ref. Instead, inline the unref, and re-initialize the internal state when the keyfile is still alive. --- glib/gkeyfile.c | 6 +++++- glib/tests/keyfile.c | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/glib/gkeyfile.c b/glib/gkeyfile.c index ae3bbbc1d..4e9b53541 100644 --- a/glib/gkeyfile.c +++ b/glib/gkeyfile.c @@ -1191,7 +1191,11 @@ g_key_file_free (GKeyFile *key_file) g_return_if_fail (key_file != NULL); g_key_file_clear (key_file); - g_key_file_unref (key_file); + + if (g_atomic_int_dec_and_test (&key_file->ref_count)) + g_slice_free (GKeyFile, key_file); + else + g_key_file_init (key_file); } /** diff --git a/glib/tests/keyfile.c b/glib/tests/keyfile.c index 16f3b788b..9ee7d02c4 100644 --- a/glib/tests/keyfile.c +++ b/glib/tests/keyfile.c @@ -1726,6 +1726,42 @@ test_get_locale (void) g_key_file_free (kf); } +static void +test_free_when_not_last_ref (void) +{ + GKeyFile *kf; + GError *error = NULL; + const gchar *data = + "[Group]\n" + "Key=Value\n"; + + kf = load_data (data, G_KEY_FILE_NONE); + /* Add a second ref */ + g_key_file_ref (kf); + + /* Quick coherence check */ + g_assert_true (g_key_file_has_group (kf, "Group")); + g_assert_true (g_key_file_has_key (kf, "Group", "Key", &error)); + g_assert_no_error (error); + + /* Should clear all keys and groups, and remove one ref */ + g_key_file_free (kf); + + /* kf should still work */ + g_assert_false (g_key_file_has_group (kf, "Group")); + g_assert_false (g_key_file_has_key (kf, "Group", "Key", &error)); + check_error (&error, G_KEY_FILE_ERROR, G_KEY_FILE_ERROR_GROUP_NOT_FOUND); + g_clear_error (&error); + + g_key_file_load_from_data (kf, data, -1, G_KEY_FILE_NONE, &error); + g_assert_no_error (error); + + g_assert_true (g_key_file_has_group (kf, "Group")); + g_assert_true (g_key_file_has_key (kf, "Group", "Key", &error)); + + g_key_file_unref (kf); +} + int main (int argc, char *argv[]) { @@ -1771,6 +1807,7 @@ main (int argc, char *argv[]) g_test_add_func ("/keyfile/roundtrip", test_roundtrip); g_test_add_func ("/keyfile/bytes", test_bytes); g_test_add_func ("/keyfile/get-locale", test_get_locale); + g_test_add_func ("/keyfile/free-when-not-last-ref", test_free_when_not_last_ref); return g_test_run (); } From 5ca9eca632cca97c01f748cc613ef9acf9b6742e Mon Sep 17 00:00:00 2001 From: Will Thompson Date: Thu, 20 Sep 2018 16:26:55 +0100 Subject: [PATCH 2/2] gdesktopappinfo: add get_string_list() function The X-Flatpak-RenamedFrom key is used in .desktop files to identify past names for the desktop file. It is defined to be a list of strings. However, there was previously no correct way to retrieve a list of strings from the GKeyFile wrapped by GDesktopAppInfo, short of re-parsing the file with GKeyFile. Note that doing something like: g_strsplit (g_desktop_app_info_get_string (...), ";", -1) is not correct: the raw value "a\;b;" represents the one-element list ["a;b"], but g_key_file_get_string() rejects the sequence "\;", and so g_desktop_app_info_get_string() returns NULL in this case. (Of course, a .desktop file with a semicolon in its name is a pathological case.) Add g_desktop_app_info_get_string_list(), a trivial wrapper around g_key_file_get_string_list(), similar to g_desktop_app_info_get_string() and co. The change from g_key_file_free() to g_key_file_unref() in the test is needed because g_key_file_free() clears the contents of the keyfile. This is fine for all the fields which are eagerly loaded and copied into GDesktopAppInfo, but not when we want to access arbitrary stuff from the keyfile. --- docs/reference/gio/gio-sections.txt | 1 + gio/gdesktopappinfo.c | 27 +++++++++++++++++++++++++++ gio/gdesktopappinfo.h | 5 +++++ gio/tests/appinfo.c | 10 +++++++++- 4 files changed, 42 insertions(+), 1 deletion(-) diff --git a/docs/reference/gio/gio-sections.txt b/docs/reference/gio/gio-sections.txt index 3b7cc28e7..f7efd2bf4 100644 --- a/docs/reference/gio/gio-sections.txt +++ b/docs/reference/gio/gio-sections.txt @@ -1621,6 +1621,7 @@ g_desktop_app_info_set_desktop_env g_desktop_app_info_get_string g_desktop_app_info_get_locale_string g_desktop_app_info_get_boolean +g_desktop_app_info_get_string_list g_desktop_app_info_has_key GDesktopAppLaunchCallback g_desktop_app_info_launch_uris_as_manager diff --git a/gio/gdesktopappinfo.c b/gio/gdesktopappinfo.c index d0ffbace3..c55a0abe2 100644 --- a/gio/gdesktopappinfo.c +++ b/gio/gdesktopappinfo.c @@ -4534,6 +4534,33 @@ g_desktop_app_info_get_boolean (GDesktopAppInfo *info, G_KEY_FILE_DESKTOP_GROUP, key, NULL); } +/** + * g_desktop_app_info_get_string_list: + * @info: a #GDesktopAppInfo + * @key: the key to look up + * @length: (out) (optional): return location for the number of returned strings, or %NULL + * + * Looks up a string list value in the keyfile backing @info. + * + * The @key is looked up in the "Desktop Entry" group. + * + * Returns: (array zero-terminated=1 length=length) (element-type utf8) (transfer full): + * a %NULL-terminated string array or %NULL if the specified + * key cannot be found. The array should be freed with g_strfreev(). + * + * Since: 2.59.0 + */ +gchar ** +g_desktop_app_info_get_string_list (GDesktopAppInfo *info, + const char *key, + gsize *length) +{ + g_return_val_if_fail (G_IS_DESKTOP_APP_INFO (info), NULL); + + return g_key_file_get_string_list (info->keyfile, + G_KEY_FILE_DESKTOP_GROUP, key, length, NULL); +} + /** * g_desktop_app_info_has_key: * @info: a #GDesktopAppInfo diff --git a/gio/gdesktopappinfo.h b/gio/gdesktopappinfo.h index 86a3caa30..1254038a4 100644 --- a/gio/gdesktopappinfo.h +++ b/gio/gdesktopappinfo.h @@ -89,6 +89,11 @@ GLIB_AVAILABLE_IN_2_36 gboolean g_desktop_app_info_get_boolean (GDesktopAppInfo *info, const char *key); +GLIB_AVAILABLE_IN_2_60 +gchar ** g_desktop_app_info_get_string_list (GDesktopAppInfo *info, + const char *key, + gsize *length); + GLIB_AVAILABLE_IN_2_38 const gchar * const * g_desktop_app_info_list_actions (GDesktopAppInfo *info); diff --git a/gio/tests/appinfo.c b/gio/tests/appinfo.c index a52bc7094..e75c4abe7 100644 --- a/gio/tests/appinfo.c +++ b/gio/tests/appinfo.c @@ -512,6 +512,8 @@ test_from_keyfile (void) GKeyFile *kf; GError *error = NULL; const gchar *categories; + gchar **categories_list; + gsize categories_count; gchar **keywords; const gchar *file; const gchar *name; @@ -522,7 +524,7 @@ test_from_keyfile (void) g_key_file_load_from_file (kf, path, G_KEY_FILE_NONE, &error); g_assert_no_error (error); info = g_desktop_app_info_new_from_keyfile (kf); - g_key_file_free (kf); + g_key_file_unref (kf); g_assert (info != NULL); g_object_get (info, "filename", &file, NULL); @@ -532,6 +534,11 @@ test_from_keyfile (void) g_assert (file == NULL); categories = g_desktop_app_info_get_categories (info); g_assert_cmpstr (categories, ==, "GNOME;GTK;"); + categories_list = g_desktop_app_info_get_string_list (info, "Categories", &categories_count); + g_assert_cmpint (categories_count, ==, 2); + g_assert_cmpint (g_strv_length (categories_list), ==, 2); + g_assert_cmpstr (categories_list[0], ==, "GNOME"); + g_assert_cmpstr (categories_list[1], ==, "GTK"); keywords = (gchar **)g_desktop_app_info_get_keywords (info); g_assert_cmpint (g_strv_length (keywords), ==, 2); g_assert_cmpstr (keywords[0], ==, "keyword1"); @@ -540,6 +547,7 @@ test_from_keyfile (void) g_assert_cmpstr (name, ==, "generic-appinfo-test"); g_assert (!g_desktop_app_info_get_nodisplay (info)); + g_strfreev (categories_list); g_object_unref (info); }