From fe3b9c00f4ea649874cab0a640d9d10654aec6f5 Mon Sep 17 00:00:00 2001 From: Robert Ancell Date: Thu, 30 Mar 2017 20:04:37 +1300 Subject: Copy hash table keys from AsApp to avoid them being used after they've been freed The hash tables currently reference the keys in the AsApp object, however this fails in two cases: 1. If you insert two apps with the same ID into the same table the second app replaces the first but the key is from the original one (confirmed in glib source code). The first app is unreffed leaving the table to contain an invalid key (or may occur later if a reference is held elsewhere). 2. There are two tables that contain GPtrArrays of apps. If AsApp in the array that was used as the key is unreffed then the hash table again contains a freed key. I'm assuming these keys weren't copied to save memory, if that is important then the keys will have to be reference counted or atoms used. --- libappstream-glib/as-self-test.c | 38 ++++++++++++++++++++++++++++++++++++++ libappstream-glib/as-store.c | 12 ++++++------ 2 files changed, 44 insertions(+), 6 deletions(-) diff --git a/libappstream-glib/as-self-test.c b/libappstream-glib/as-self-test.c index 220fc2d..e1000f5 100644 --- a/libappstream-glib/as-self-test.c +++ b/libappstream-glib/as-self-test.c @@ -5126,6 +5126,43 @@ as_test_store_merge_replace_func (void) g_assert (!as_app_has_category (app2, "Family")); } +static void +as_test_store_merge_then_replace_func (void) +{ + g_autoptr (AsApp) app1 = NULL; + g_autoptr (AsApp) app2 = NULL; + g_autoptr (AsApp) app3 = NULL; + g_autoptr (AsStore) store = NULL; + + store = as_store_new (); + + /* this test case checks that a memory error using app names as keys is fixed */ + + /* add app */ + app1 = as_app_new (); + as_app_set_id (app1, "org.gnome.Software.desktop"); + _as_app_add_format_kind (app1, AS_FORMAT_KIND_DESKTOP); + as_app_set_priority (app1, 0); + as_store_add_app (store, app1); + g_clear_object (&app1); + + /* add app that merges with the first */ + app2 = as_app_new (); + as_app_set_id (app2, "org.gnome.Software.desktop"); + _as_app_add_format_kind (app2, AS_FORMAT_KIND_DESKTOP); + as_app_set_priority (app2, 0); + as_store_add_app (store, app2); + g_clear_object (&app2); + + /* add app that replaces the second */ + app3 = as_app_new (); + as_app_set_id (app3, "org.gnome.Software.desktop"); + _as_app_add_format_kind (app3, AS_FORMAT_KIND_DESKTOP); + as_app_set_priority (app3, 1); + as_store_add_app (store, app3); + g_clear_object (&app3); +} + /* shows the unique-id globbing functions at work */ static void as_test_utils_unique_id_hash_func (void) @@ -5313,6 +5350,7 @@ main (int argc, char **argv) g_test_add_func ("/AppStream/store{unique}", as_test_store_unique_func); g_test_add_func ("/AppStream/store{merge}", as_test_store_merge_func); g_test_add_func ("/AppStream/store{merge-replace}", as_test_store_merge_replace_func); + g_test_add_func ("/AppStream/store{merge-then-replace}", as_test_store_merge_then_replace_func); g_test_add_func ("/AppStream/store{empty}", as_test_store_empty_func); if (g_test_slow ()) { g_test_add_func ("/AppStream/store{auto-reload-dir}", as_test_store_auto_reload_dir_func); diff --git a/libappstream-glib/as-store.c b/libappstream-glib/as-store.c index da793fb..f5b793a 100644 --- a/libappstream-glib/as-store.c +++ b/libappstream-glib/as-store.c @@ -1078,7 +1078,7 @@ as_store_add_app (AsStore *store, AsApp *app) if (apps == NULL) { apps = g_ptr_array_new_with_free_func ((GDestroyNotify) g_object_unref); g_hash_table_insert (priv->hash_merge_id, - (gpointer) as_app_get_id (app), + g_strdup (as_app_get_id (app)), apps); } g_debug ("added %s merge component: %s", @@ -1260,7 +1260,7 @@ as_store_add_app (AsStore *store, AsApp *app) if (apps == NULL) { apps = g_ptr_array_new_with_free_func ((GDestroyNotify) g_object_unref); g_hash_table_insert (priv->hash_id, - (gpointer) as_app_get_id (app), + g_strdup (as_app_get_id (app)), apps); } g_ptr_array_add (apps, g_object_ref (app)); @@ -1268,7 +1268,7 @@ as_store_add_app (AsStore *store, AsApp *app) /* success, add to array */ g_ptr_array_add (priv->array, g_object_ref (app)); g_hash_table_insert (priv->hash_unique_id, - (gpointer) as_app_get_unique_id (app), + g_strdup (as_app_get_unique_id (app)), g_object_ref (app)); pkgnames = as_app_get_pkgnames (app); for (i = 0; i < pkgnames->len; i++) { @@ -3472,15 +3472,15 @@ as_store_init (AsStore *store) NULL); priv->hash_id = g_hash_table_new_full (g_str_hash, g_str_equal, - NULL, + g_free, (GDestroyNotify) g_ptr_array_unref); priv->hash_merge_id = g_hash_table_new_full (g_str_hash, g_str_equal, - NULL, + g_free, (GDestroyNotify) g_ptr_array_unref); priv->hash_unique_id = g_hash_table_new_full (g_str_hash, g_str_equal, - NULL, + g_free, g_object_unref); priv->hash_pkgname = g_hash_table_new_full (g_str_hash, g_str_equal, -- cgit v1.2.1