summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorRobert Ancell <robert.ancell@gmail.com>2017-03-30 20:04:37 +1300
committerRichard Hughes <richard@hughsie.com>2017-03-30 08:04:37 +0100
commitfe3b9c00f4ea649874cab0a640d9d10654aec6f5 (patch)
tree2a120c2f82212df4482733416d85507ab173d9ad
parent1ed3ad771fff358f27729a1c0c7b13ac861b28b8 (diff)
downloadappstream-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.c38
-rw-r--r--libappstream-glib/as-store.c12
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,