diff options
author | Robert Ancell <robert.ancell@gmail.com> | 2017-03-30 20:04:37 +1300 |
---|---|---|
committer | Richard Hughes <richard@hughsie.com> | 2017-03-30 08:04:37 +0100 |
commit | fe3b9c00f4ea649874cab0a640d9d10654aec6f5 (patch) | |
tree | 2a120c2f82212df4482733416d85507ab173d9ad | |
parent | 1ed3ad771fff358f27729a1c0c7b13ac861b28b8 (diff) | |
download | appstream-glib-fe3b9c00f4ea649874cab0a640d9d10654aec6f5.tar.gz |
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.
-rw-r--r-- | libappstream-glib/as-self-test.c | 38 | ||||
-rw-r--r-- | 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, |