GNOME Bugzilla – Bug 755664
Memory issues: dangling pointers, leaks
Last modified: 2015-09-28 18:32:07 UTC
It has been discussed thoroughly on IRC but I think it needs adding to bugzilla. There are some memory issues in gnome-software. 1. A severe dangling pointer here: https://git.gnome.org/browse/gnome-software/tree/src/gs-plugin-loader.c#n2274 Whenever gs_plugin_loader_run_refine() or gs_plugin_loader_list_dedupe() is called the list should be constructed with gs_plugin_add_app(), never with g_list_prepend(). The dedupe code assumes that a list holds a reference for each its item and it should unreference an item whenever it recognizes it as a duplicate and removes. gs_plugin_add_app() ensures that a reference counter is increased for each added item. If you add an item with g_list_prepend() it is unreferenced if it is deduplicated. It does not cause the crash immediately but it leads to a crash in some other place, usually in a notification code which holds more references to the app object. The crash occurs only if: - the deduplication actually happens (eg., when the app object already exists somewhere in the g-s application, it may be already installed or featured, and we install/remove an app which we have found via the search mechanism), - if the ref counter reaches zero, this happens if the deduplication happens multiple times (if you install/remove the app multiple times). This makes the bug extremely difficult to reproduce but I believe this is the reason of multiple automatic bug reports in RH bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1144420 https://bugzilla.redhat.com/show_bug.cgi?id=1158870 https://bugzilla.redhat.com/show_bug.cgi?id=1161799 https://bugzilla.redhat.com/show_bug.cgi?id=1203834 https://bugzilla.redhat.com/show_bug.cgi?id=1209090 https://bugzilla.redhat.com/show_bug.cgi?id=1213605 https://bugzilla.redhat.com/show_bug.cgi?id=1233637 https://bugzilla.redhat.com/show_bug.cgi?id=1251799 https://bugzilla.redhat.com/show_bug.cgi?id=1253519 https://bugzilla.redhat.com/show_bug.cgi?id=1256043 and probably more. The simple solution is to use gs_plugin_add_app() instead of g_list_prepend()/append() in this place and probably in more places. The more advanced solution is to implement GsAppList which has already been introduced here https://git.gnome.org/browse/gnome-software/diff/src/gs-plugin.h?id=80611f4911fb37332e27334b25fb4f95d8ac6b86 and make sure it completely replaces GList. This would ensure that this bug does not return in future. 2. There is a pattern used here: https://git.gnome.org/browse/gnome-software/tree/src/gs-shell-details.c#n1021 and in multiple other places. The code: void gs_whatever_set_app (GsWhatever *self, GsApp *app) { if (self->app != NULL) g_object_unref (self->app); self->app = g_object_ref (app); ... usually works fine but will produce a dangling pointer if self->app is the same as app and this is its last reference. Probably it has not yet caused any problem in g-s but this pattern should be discouraged and avoided, especially when we have other memory problems. Either a new app should be refed before the old app is unrefed, or the function should return immediately if app==self->app, or a helper function g_set_object() should be used. 3. A series of memory leaks in gs-plugin-dummy.c: https://git.gnome.org/browse/gnome-software/tree/src/plugins/gs-plugin-dummy.c#n135 is an example of an app being allocated but never freed. gs_plugin_add_app() adds another reference and I believe this one will be eventually unrefed but the original one does not. The same pattern repeats multiple times in this file. One may doubt if this file is ever used but Richard said it is used in memory check, and it's indeed a memory leak.
Created attachment 312227 [details] [review] Fixes case 1: dangling pointer when installing/removing an app This is the simplest solution to the case 1. I have reviewed other occurrences of g_list_prepend()/g_list_append() and although some of them could be improved and replaced with gs_plugin_add_app() they do not produce any unbalanced ref/unref. So only this one is urgent.
Created attachment 312231 [details] [review] Fixes case 2: use g_set_object() instead of g_object_unref()/g_object_ref() sequence In case of gs-shell-details.c it fixes some other problems which I have just noticed: - gs_shell_details_progress_changed_cb() was missing when setting the app by the file name, this made the progress not update when installing an app from an rpm; - signals were not disconnected when the app was no longer set to the shell page; - gs_shell_details_set_app() checks for the garbage data as the arguments.
Created attachment 312232 [details] [review] Fixes case 3: plugs a memory leak in the dummy plugin This one is trivial when compared to the previous ones. :)
Review of attachment 312227 [details] [review]: Thanks, looks good to me.
Review of attachment 312232 [details] [review]: Looks good to me.
Review of attachment 312231 [details] [review]: Can you split notify::progress changes out to a separate patch please? ::: src/gs-app-row.c @@ +359,3 @@ + if (priv->app) + g_signal_handlers_disconnect_by_func (priv->app, gs_app_row_notify_props_changed_cb, app_row); + g_set_object (&priv->app, app); This shouldn't be needed here. gs_app_row_set_app() is supposed to be only ever called once per a GsAppRow lifetime and there's no need to do signal disconnection and priv->app clearing. I'd rather go with an assert(priv->app == NULL) instead to make it obvious that it only supports setting the app once.
Created attachment 312273 [details] [review] Case 2 reviewed: use g_set_object() instead of g_object_unref()/g_object_ref() sequence This patch also disconnects the no longer used signal handlers in gs-shell-details.c and checks for garbage values in gs_shell_details_set_app() but no longer does the things which are not related here: does not add a missing "notify::progress" callback and does not fix gs_app_row_set_app() which is not intended to be called multiple times.
Thanks, pushed the last one as well. I ended up splitting it up in a slightly different way to make it easier to port portions of it to Fedora 21 that doesn't have g_set_object.