[gnome-software] Fix thread safety issues in the plugins

[Giovanni Campagna <gcampagna src gnome org>]

commit 526284fa0dd21d5c281f98e8452461edd4ac0098
Author: Giovanni Campagna <gcampagna src gnome org>
Date:   Sat Aug 31 16:34:01 2013 +0200

    Fix thread safety issues in the plugins
    
    The plugins are all run in worker threads, so shared data structures
    must be protected with locks, and initialization must be protected
    with GOnce.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=707185

 src/plugins/gs-plugin-datadir-apps.c           |    8 ++++++
 src/plugins/gs-plugin-datadir-filename.c       |   25 ++++++++++++++----
 src/plugins/gs-plugin-desktopdb.c              |   32 ++++++++++++++++++-----
 src/plugins/gs-plugin-hardcoded-descriptions.c |    8 +++---
 src/plugins/gs-plugin-hardcoded-ratings.c      |   26 ++++++-------------
 src/plugins/gs-plugin-local-ratings.c          |   11 +++++---
 6 files changed, 71 insertions(+), 39 deletions(-)
---
diff --git a/src/plugins/gs-plugin-datadir-apps.c b/src/plugins/gs-plugin-datadir-apps.c
index 23a1aef..97a968f 100644
--- a/src/plugins/gs-plugin-datadir-apps.c
+++ b/src/plugins/gs-plugin-datadir-apps.c
@@ -25,6 +25,7 @@
 #include <gs-plugin.h>
 
 struct GsPluginPrivate {
+       GMutex                   plugin_mutex;
        GHashTable              *cache;
 };
 
@@ -70,6 +71,7 @@ gs_plugin_initialize (GsPlugin *plugin)
                                                     g_str_equal,
                                                     g_free,
                                                     (GDestroyNotify) gs_plugin_datadir_apps_cache_item_free);
+       g_mutex_init (&plugin->priv->plugin_mutex);
 }
 
 /**
@@ -88,6 +90,7 @@ void
 gs_plugin_destroy (GsPlugin *plugin)
 {
        g_hash_table_unref (plugin->priv->cache);
+       g_mutex_clear (&plugin->priv->plugin_mutex);
 }
 
 /**
@@ -130,11 +133,14 @@ gs_plugin_datadir_apps_extract_desktop_data (GsPlugin *plugin,
        GsPluginDataDirAppsCacheItem *cache_item;
 
        /* is in cache */
+       g_mutex_lock (&plugin->priv->plugin_mutex);
        cache_item = g_hash_table_lookup (plugin->priv->cache, desktop_file);
        if (cache_item != NULL) {
                gs_plugin_datadir_apps_set_from_cache_item (app, cache_item);
+               g_mutex_unlock (&plugin->priv->plugin_mutex);
                goto out;
        }
+       g_mutex_unlock (&plugin->priv->plugin_mutex);
 
        /* load desktop file */
        key_file = g_key_file_new ();
@@ -203,9 +209,11 @@ gs_plugin_datadir_apps_extract_desktop_data (GsPlugin *plugin,
 
        /* add to cache */
        gs_plugin_datadir_apps_set_from_cache_item (app, cache_item);
+       g_mutex_lock (&plugin->priv->plugin_mutex);
        g_hash_table_insert (plugin->priv->cache,
                             g_strdup (desktop_file),
                             cache_item);
+       g_mutex_unlock (&plugin->priv->plugin_mutex);
 out:
        if (key_file != NULL)
                g_key_file_unref (key_file);
diff --git a/src/plugins/gs-plugin-datadir-filename.c b/src/plugins/gs-plugin-datadir-filename.c
index fd4bca7..882e5b0 100644
--- a/src/plugins/gs-plugin-datadir-filename.c
+++ b/src/plugins/gs-plugin-datadir-filename.c
@@ -24,6 +24,7 @@
 #include <gs-plugin.h>
 
 struct GsPluginPrivate {
+       GMutex                   plugin_mutex;
        GHashTable              *cache;
 };
 
@@ -48,6 +49,7 @@ gs_plugin_initialize (GsPlugin *plugin)
                                                     g_str_equal,
                                                     g_free,
                                                     g_free);
+       g_mutex_init (&plugin->priv->plugin_mutex);
 }
 
 /**
@@ -66,17 +68,18 @@ void
 gs_plugin_destroy (GsPlugin *plugin)
 {
        g_hash_table_unref (plugin->priv->cache);
+       g_mutex_clear (&plugin->priv->plugin_mutex);
 }
 
 /**
  * gs_plugin_datadir_filename_find:
  */
-static const gchar *
+static gchar *
 gs_plugin_datadir_filename_find (GsPlugin *plugin,
                                 GsApp *app)
 {
        const gchar *id;
-       const gchar *path_tmp = NULL;
+       gchar *path_tmp = NULL;
        gboolean ret;
        gchar *path = NULL;
        const char * const *datadirs;
@@ -86,14 +89,19 @@ gs_plugin_datadir_filename_find (GsPlugin *plugin,
        id = gs_app_get_id (app);
        if (id == NULL)
                goto out;
+
+       g_mutex_lock (&plugin->priv->plugin_mutex);
        ret = g_hash_table_lookup_extended (plugin->priv->cache,
                                            id,
                                            NULL,
                                            (gpointer *) &path_tmp);
        if (ret) {
                g_debug ("found existing %s", id);
+               path_tmp = g_strdup (path_tmp);
+               g_mutex_unlock (&plugin->priv->plugin_mutex);
                goto out;
        }
+       g_mutex_unlock (&plugin->priv->plugin_mutex);
 
        /* find if the file exists */
        datadirs = g_get_system_data_dirs ();
@@ -102,18 +110,22 @@ gs_plugin_datadir_filename_find (GsPlugin *plugin,
                                        datadirs[i], gs_app_get_id (app));
                if (g_file_test (path, G_FILE_TEST_EXISTS)) {
                        path_tmp = g_strdup (path);
+                       g_mutex_lock (&plugin->priv->plugin_mutex);
                        g_hash_table_insert (plugin->priv->cache,
                                             g_strdup (id),
-                                            (gpointer) path_tmp);
+                                            g_strdup (path));
+                       g_mutex_unlock (&plugin->priv->plugin_mutex);
                        break;
                }
        }
 
        if (path_tmp == NULL) {
                /* add an empty key to the cache to avoid stat'ing again */
+               g_mutex_lock (&plugin->priv->plugin_mutex);
                g_hash_table_insert (plugin->priv->cache,
                                     g_strdup (id),
                                     NULL);
+               g_mutex_unlock (&plugin->priv->plugin_mutex);
        }
 out:
        g_free (path);
@@ -129,7 +141,7 @@ gs_plugin_refine (GsPlugin *plugin,
                  GCancellable *cancellable,
                  GError **error)
 {
-       const gchar *tmp;
+       gchar *tmp;
        GList *l;
        GsApp *app;
 
@@ -137,14 +149,15 @@ gs_plugin_refine (GsPlugin *plugin,
                app = GS_APP (l->data);
                if (gs_app_get_name (app) != NULL)
                        continue;
-               tmp = gs_app_get_metadata_item (app, "datadir-desktop-filename");
-               if (tmp != NULL)
+               if (gs_app_get_metadata_item (app, "datadir-desktop-filename") != NULL)
                        continue;
+
                tmp = gs_plugin_datadir_filename_find (plugin, app);
                if (tmp != NULL) {
                        gs_app_set_metadata (app,
                                             "datadir-desktop-filename",
                                             tmp);
+                       g_free (tmp);
                }
        }
        return TRUE;
diff --git a/src/plugins/gs-plugin-desktopdb.c b/src/plugins/gs-plugin-desktopdb.c
index 95b6176..ec80c3c 100644
--- a/src/plugins/gs-plugin-desktopdb.c
+++ b/src/plugins/gs-plugin-desktopdb.c
@@ -21,6 +21,8 @@
 
 #include <config.h>
 
+#include <string.h>
+
 #define I_KNOW_THE_PACKAGEKIT_GLIB2_API_IS_SUBJECT_TO_CHANGE
 #include <packagekit-glib2/packagekit.h>
 
@@ -28,7 +30,8 @@
 
 struct GsPluginPrivate {
        PkDesktop               *desktop;
-       gboolean                 loaded;
+       gsize                    loaded;
+       GMutex                   plugin_mutex;
        GHashTable              *cache;
 };
 
@@ -54,6 +57,7 @@ gs_plugin_initialize (GsPlugin *plugin)
                                                     g_str_equal,
                                                     g_free,
                                                     g_free);
+       g_mutex_init (&plugin->priv->plugin_mutex);
 }
 
 /**
@@ -73,6 +77,7 @@ gs_plugin_destroy (GsPlugin *plugin)
 {
        g_object_unref (plugin->priv->desktop);
        g_hash_table_unref (plugin->priv->cache);
+       g_mutex_clear (&plugin->priv->plugin_mutex);
 }
 
 /**
@@ -83,15 +88,18 @@ gs_plugin_desktopdb_set_metadata (GsPlugin *plugin,
                                  GsApp *app,
                                  const gchar *pkg_name)
 {
-       const gchar *desktop_file;
-       gchar *id = NULL;
+       gchar *desktop_file;
+       gchar *id = NULL, *dot;
        GError *error = NULL;
        GPtrArray *files = NULL;
 
        /* is in cache */
+       g_mutex_lock (&plugin->priv->plugin_mutex);
        desktop_file = g_hash_table_lookup (plugin->priv->cache, pkg_name);
-       if (desktop_file == NULL) {
+       desktop_file = g_strdup (desktop_file);
+       g_mutex_unlock (&plugin->priv->plugin_mutex);
 
+       if (desktop_file == NULL) {
                /* try to get the list of desktop files for this package */
                files = pk_desktop_get_shown_for_package (plugin->priv->desktop,
                                                          pkg_name,
@@ -108,24 +116,32 @@ gs_plugin_desktopdb_set_metadata (GsPlugin *plugin,
                }
 
                /* add just the first desktop file */
-               desktop_file = g_ptr_array_index (files, 0);
+               desktop_file = g_strdup (g_ptr_array_index (files, 0));
 
                /* add to the cache */
+               g_mutex_lock (&plugin->priv->plugin_mutex);
                g_hash_table_insert (plugin->priv->cache,
                                     g_strdup (pkg_name),
                                     g_strdup (desktop_file));
+               g_mutex_unlock (&plugin->priv->plugin_mutex);
+
        }
 
        /* also set the ID if it's missing */
        if (gs_app_get_id (app) == NULL) {
                id = g_path_get_basename (desktop_file);
-               g_strdelimit (id, ".", '\0');
+               dot = strrchr (id, '.');
+               if (dot)
+                       *dot = '\0';
+
                gs_app_set_id (app, id);
        }
 
        gs_app_set_metadata (app,
                             "datadir-desktop-filename",
                             desktop_file);
+       g_free (desktop_file);
+
 out:
        g_free (id);
        if (files != NULL)
@@ -147,8 +163,10 @@ gs_plugin_refine (GsPlugin *plugin,
        GsApp *app;
 
        /* not loaded yet */
-       if (!plugin->priv->loaded) {
+       if (g_once_init_enter (&plugin->priv->loaded)) {
                ret = pk_desktop_open_database (plugin->priv->desktop, error);
+               g_once_init_leave (&plugin->priv->loaded, TRUE);
+
                if (!ret)
                        goto out;
        }
diff --git a/src/plugins/gs-plugin-hardcoded-descriptions.c b/src/plugins/gs-plugin-hardcoded-descriptions.c
index 988786d..cd2a1b0 100644
--- a/src/plugins/gs-plugin-hardcoded-descriptions.c
+++ b/src/plugins/gs-plugin-hardcoded-descriptions.c
@@ -25,7 +25,7 @@
 
 struct GsPluginPrivate {
        GHashTable              *cache;
-       gboolean                 loaded;
+       gsize                    loaded;
 };
 
 /**
@@ -111,7 +111,6 @@ gs_plugin_hardcoded_descriptions_add (GsPlugin *plugin, GError **error)
                                     (gpointer) descriptions[i].desc);
        }
 
-       plugin->priv->loaded = TRUE;
        return TRUE;
 }
 
@@ -130,9 +129,10 @@ gs_plugin_refine (GsPlugin *plugin,
        GList *l;
        GsApp *app;
 
-       /* already loaded */
-       if (!plugin->priv->loaded) {
+       if (g_once_init_enter (&plugin->priv->loaded)) {
                ret = gs_plugin_hardcoded_descriptions_add (plugin, error);
+               g_once_init_leave (&plugin->priv->loaded, TRUE);
+
                if (!ret)
                        goto out;
        }
diff --git a/src/plugins/gs-plugin-hardcoded-ratings.c b/src/plugins/gs-plugin-hardcoded-ratings.c
index 0a1ff31..65de8e5 100644
--- a/src/plugins/gs-plugin-hardcoded-ratings.c
+++ b/src/plugins/gs-plugin-hardcoded-ratings.c
@@ -25,8 +25,7 @@
 
 struct GsPluginPrivate {
        GHashTable              *cache;
-       gboolean                 loaded;
-       GMutex                   mutex;
+       gsize                    loaded;
 };
 
 /**
@@ -51,9 +50,6 @@ gs_plugin_initialize (GsPlugin *plugin)
                                                     g_str_equal,
                                                     NULL,
                                                     NULL);
-
-       /* can only load from one thread */
-       g_mutex_init (&plugin->priv->mutex);
 }
 
 /**
@@ -72,7 +68,6 @@ void
 gs_plugin_destroy (GsPlugin *plugin)
 {
        g_hash_table_unref (plugin->priv->cache);
-       g_mutex_clear (&plugin->priv->mutex);
 }
 
 /**
@@ -693,12 +688,6 @@ gs_plugin_hardcoded_ratings_setup (GsPlugin *plugin, GError **error)
                { -1    ,NULL}
        };
 
-       /* protect */
-       g_mutex_lock (&plugin->priv->mutex);
-
-       if (plugin->priv->loaded)
-               goto out;
-
        /* add each one to a hash table */
        for (i = 0; ratings[i].id != NULL; i++) {
                g_hash_table_insert (plugin->priv->cache,
@@ -706,9 +695,6 @@ gs_plugin_hardcoded_ratings_setup (GsPlugin *plugin, GError **error)
                                     GINT_TO_POINTER (ratings[i].value));
        }
 
-       plugin->priv->loaded = TRUE;
-out:
-       g_mutex_unlock (&plugin->priv->mutex);
        return TRUE;
 }
 
@@ -728,9 +714,13 @@ gs_plugin_refine (GsPlugin *plugin,
        GsApp *app;
 
        /* already loaded */
-       ret = gs_plugin_hardcoded_ratings_setup (plugin, error);
-       if (!ret)
-               goto out;
+       if (g_once_init_enter (&plugin->priv->loaded)) {
+               ret = gs_plugin_hardcoded_ratings_setup (plugin, error);
+               g_once_init_leave (&plugin->priv->loaded, TRUE);
+
+               if (!ret)
+                       goto out;
+       }
 
        /* add any missing ratings data */
        for (l = list; l != NULL; l = l->next) {
diff --git a/src/plugins/gs-plugin-local-ratings.c b/src/plugins/gs-plugin-local-ratings.c
index 2818da0..0b2ca7a 100644
--- a/src/plugins/gs-plugin-local-ratings.c
+++ b/src/plugins/gs-plugin-local-ratings.c
@@ -28,7 +28,7 @@
 #include <gs-plugin.h>
 
 struct GsPluginPrivate {
-       gboolean                 loaded;
+       gsize                    loaded;
        gchar                   *db_path;
        sqlite3                 *db;
 };
@@ -132,7 +132,6 @@ gs_plugin_local_ratings_load_db (GsPlugin *plugin,
        }
 
        /* success */
-       plugin->priv->loaded = TRUE;
 out:
        return ret;
 }
@@ -182,8 +181,10 @@ gs_plugin_app_set_rating (GsPlugin *plugin,
        gint rc;
 
        /* already loaded */
-       if (!plugin->priv->loaded) {
+       if (g_once_init_enter (&plugin->priv->loaded)) {
                ret = gs_plugin_local_ratings_load_db (plugin, error);
+               g_once_init_leave (&plugin->priv->loaded, TRUE);
+
                if (!ret)
                        goto out;
        }
@@ -224,8 +225,10 @@ gs_plugin_refine (GsPlugin *plugin,
        GsApp *app;
 
        /* already loaded */
-       if (!plugin->priv->loaded) {
+       if (g_once_init_enter (&plugin->priv->loaded)) {
                ret = gs_plugin_local_ratings_load_db (plugin, error);
+               g_once_init_leave (&plugin->priv->loaded, TRUE);
+
                if (!ret)
                        goto out;
        }