GNOME Bugzilla – Bug 791337
Crash opening URIs with g_desktop_app_info_launch_uris_with_spawn()
Last modified: 2017-12-08 13:51:56 UTC
We just observed in Endless what looks to be a bug that manifests itself when trying to open directories with the Visual Studio flatpak (VS installs itself as a handler for inode/directory) via g_app_info_launch_default_for_uri(), which results in a crash deep in expand_macro(): ``` Thread 6 "pool" received signal SIGSEGV, Segmentation fault.
+ Trace 238233
Thread 139784534091520 (LWP 17936)
In this particular case the crash happened from the xdg-desktop-portal, but doesn't seem to be flatpak-specific. What I think it's going on is that expand_macro() takes a pointer to a GList** as a parameter, with the clear intention of treating it as an out parameter whose content will override[1], and that will cause trouble if this is called after initiating the request from the portal[2], as the GList* passed from there is not expected to be modified. I'm still not 100% sure why this was a problem with Visual Studio and not with Nautilus, but I think the key might be in g_desktop_app_info_launch_uris_internal(): ``` static gboolean g_desktop_app_info_launch_uris_internal (GAppInfo *appinfo, GList *uris, [...] GError **error) { [...] if (session_bus && info->app_id) g_desktop_app_info_launch_uris_with_dbus (info, session_bus, uris, launch_context); else success = g_desktop_app_info_launch_uris_with_spawn (info, session_bus, info->exec, uris, launch_context, spawn_flags, user_setup, user_setup_data, pid_callback, pid_callback_data, error); [...] } ``` According to the backtrace, when the segfault happens the logic takes the else branch of this if, which is the only place where g_desktop_app_info_launch_uris_with_spawn() is called passing the list of uris (the other single place passes NULL instead[3], so this is the only code path that could expose the issue, since g_desktop_app_info_launch_uris_with_spawn() would then call expand_application_parameters(), which is where the original GList* is "converted" into a GList**[4]. So, it all boils down to what I think it's a bug in GLib that seems only to be exposed in some scenarios that perhaps are not that common (!session_bus || !info->app_id), and perhaps that's why it was not detected before. [1] https://git.gnome.org/browse/glib/tree/gio/gdesktopappinfo.c#n2392 [2] https://github.com/flatpak/xdg-desktop-portal/blob/master/src/open-uri.c#L206 [3] https://git.gnome.org/browse/glib/tree/gio/gdesktopappinfo.c#n4596 [4] https://git.gnome.org/browse/glib/tree/gio/gdesktopappinfo.c#n2682
(In reply to Mario Sánchez Prada from comment #0) > [...] > In this particular case the crash happened from the xdg-desktop-portal, but doesn't seem to be flatpak-specific. Actually, I take that back. There was a bug in the portal, which was sending a GList of NULLs to GIO, and that's wrong from the client side too (fixed in https://github.com/flatpak/xdg-desktop-portal/issues/134). Regardless of that, GLib still needs fixing since such a situation shouldn't crash the application, but I'm afraid that my analysis above is wrong: While passing a reference to the function's input parameter is wrong, it's not what's crashing the application. The problem lies in that the NULL being passed by the portal was causing an invalid read in expand_macro(), and that's the real reason behind the crash. I've been working on the fix for this in the context of Endless [1], so I will be uploading the patches in a few minutes. [1] https://github.com/endlessm/glib/commits/T20318
Created attachment 365240 [details] [review] gdesktopappinfo: Pass a copy of the URIs list to expand_application_parameters() This list will be modified in-place when calling expand_macro(), so pass a copy of it instead the original pointer, that is supposed to be an input parameter only for g_desktop_app_info_launch_uris_with_spawn().
Created attachment 365241 [details] [review] gdesktopappinfo: Gracefully handle NULL URIs when passed to expand_macro() If an application calls g_app_info_launch_uris() with a GList that includes NULL values in some of its data members, and GIO ends up internally calling g_desktop_app_info_launch_uris_with_spawn() for whatever reason (e.g. no D-Bus session available), expand_macro() will crash due to the invalid data. As this is considered a programmer error, use g_return_val_if_fail() in those situations to prevent the crash from happening, but printing a warning anyway.
Created attachment 365242 [details] [review] gio/tests/appinfo: New test for launch with "appId-less" applications New test to make sure we exercise the code paths in gdesktopappinfo.c that get triggered when g_desktop_app_info_launch_uris_with_spawn() is used (i.e. unknown app ID, no session bus), both for when either a single URI or multiple ones are expected by the application.
Review of attachment 365240 [details] [review]: Yes.
Review of attachment 365241 [details] [review]: Yes!
Review of attachment 365242 [details] [review]: YES!
Fantastic, thanks. I’ll also push to glib-2-54. Attachment 365240 [details] pushed as fbed9c8 - gdesktopappinfo: Pass a copy of the URIs list to expand_application_parameters() Attachment 365241 [details] pushed as d501bd0 - gdesktopappinfo: Gracefully handle NULL URIs when passed to expand_macro() Attachment 365242 [details] pushed as 637a298 - gio/tests/appinfo: New test for launch with "appId-less" applications
Backported to glib-2-54: 620c50ba1 (HEAD -> glib-2-54, origin/glib-2-54) gio/tests/appinfo: New test for launch with "appId-less" applications 75681a877 gdesktopappinfo: Gracefully handle NULL URIs when passed to expand_macro() d42115b01 gdesktopappinfo: Pass a copy of the URIs list to expand_application_parameters()