GNOME Bugzilla – Bug 670418
gmenu_tree_load_sync() returns FALSE without setting error, leading to gnome-shell crash
Last modified: 2015-02-27 04:05:40 UTC
Created attachment 208020 [details] gnome-shell crash backtrace A Gentoo user reported a segmentation fault in gnome-shell-3.2.2.1 (see https://bugs.gentoo.org/show_bug.cgi?id=404475). The backtrace from the segfault (attached) showed a crash in on_apps_tree_changed_cb() in gnome-shell's src/shell-app-system.c. The segmentation fault occurred in the following code: GError *error = NULL; [...] if (!gmenu_tree_load_sync (self->priv->apps_tree, &error)) { g_warning ("Failed to load apps: %s", error->message); return; } because error was NULL even though the gmenu_tree_load_sync() call had returned FALSE. gnome-menus needs to follow the usual error-handling convention for glib-based libraries, and to ensure that gmenu_tree_load_sync() always sets the error parameter on failure.
I am the gentoo user who originally reported the bug; please let me know if you need any more information.
There's no guarantee that the GError will be set. If it's set, then it's guaranteed that there was a failure, but that's all. This can happen because of a g_return_if_fail(), for instance. So moving to gnome-shell since it shouldn't assume the GError is set. (that being said, if someone can identify a code path where gnome-menus could set the GError, I'm happy to add something there)
Created attachment 208024 [details] [review] app-system: Don't assume that gmenu_tree_load_sync() sets error The function my return FALSE without setting the GError, so don't assume it is set to prevent a crash in that case. While at it, free the GError we were leaking before.
Created attachment 208025 [details] [review] patch for gnome-shell (In reply to comment #2) > There's no guarantee that the GError will be set. If it's set, then it's > guaranteed that there was a failure, but that's all. > > This can happen because of a g_return_if_fail(), for instance. I always believed that the implicit convention was that the *only* situation where GError might not be set on failure is in case of API misuse (e.g. when a function does g_return_if_fail() because an argument violates documented requirements). But if gnome-menus developers think that this really is the intended behavior, here's a patch for gnome-shell to guard against a NULL error.
Comment on attachment 208025 [details] [review] patch for gnome-shell Obsolete; Florian's patch is better :)
Review of attachment 208024 [details] [review]: Small typo: "The function my return FALSE" ^^ Looks good to me.
Attachment 208024 [details] pushed as b990ed2 - app-system: Don't assume that gmenu_tree_load_sync() sets error
(In reply to comment #4) > Created an attachment (id=208025) [details] [review] > patch for gnome-shell > > (In reply to comment #2) > > There's no guarantee that the GError will be set. If it's set, then it's > > guaranteed that there was a failure, but that's all. > > > > This can happen because of a g_return_if_fail(), for instance. > > I always believed that the implicit convention was that the *only* situation > where GError might not be set on failure is in case of API misuse (e.g. when a > function does g_return_if_fail() because an argument violates documented > requirements). This is certainly the case - if you have a function that is gboolean do_something(args...., GError *error); If passed valid arguments then it must set the GError if and only if it returns FALSE. If it g_return_if_fail()'s then all bets are off.
OK, first of all I find it amazing that in this discussion apparently no one bothered to open up $EDITOR and check "is there a bug in gnome-menus"? Anyways I've done so and if there is one, I'm not seeing it. There aren't that many code paths that gmenu_tree_load_sync() calls that throw, and they all look correct. But I think this patch is wrong. We shouldn't be trying to work around g_return_if_fail(). Oh except - what if the reporter had an old version? I ran: "git log -S GError" and the first thing that pops up is: commit 13920c91868f77a964b4498b00072563608f583e Author: Colin Walters <walters@verbum.org> Date: Sun Apr 17 09:51:58 2011 -0400 Further propagate GError for gmenu_tree_load_sync() We had some GError use internally; clean things up so we propagate it more consistently to the top of gmenu_tree_load_sync(). https://bugzilla.gnome.org/show_bug.cgi?id=647968
(In reply to comment #9) > OK, first of all I find it amazing that in this discussion apparently no one > bothered to open up $EDITOR and check "is there a bug in gnome-menus"? For the record, I did and didn't find anything wrong either. Which is why I assumed some other g_return_if_fail() issue that could be caused by something outside gnome-menus.
Nevermind, that's right in the middle of a branch merge, so unless the reporter is running a 10 month old random git snapshot, not likely the problem. I think we'd need the reporter to step through with gdb.
We no longer use GMenu, so no point in keeping this bug around.