After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 791337 - Crash opening URIs with g_desktop_app_info_launch_uris_with_spawn()
Crash opening URIs with g_desktop_app_info_launch_uris_with_spawn()
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: gio
unspecified
Other Linux
: High critical
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2017-12-07 12:11 UTC by Mario Sánchez Prada
Modified: 2017-12-08 13:51 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gdesktopappinfo: Pass a copy of the URIs list to expand_application_parameters() (2.50 KB, patch)
2017-12-08 12:51 UTC, Mario Sánchez Prada
committed Details | Review
gdesktopappinfo: Gracefully handle NULL URIs when passed to expand_macro() (3.85 KB, patch)
2017-12-08 12:52 UTC, Mario Sánchez Prada
committed Details | Review
gio/tests/appinfo: New test for launch with "appId-less" applications (4.78 KB, patch)
2017-12-08 12:52 UTC, Mario Sánchez Prada
committed Details | Review

Description Mario Sánchez Prada 2017-12-07 12:11:40 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.

Thread 139784534091520 (LWP 17936)

  • #0 __strchr_sse2
    at ../sysdeps/x86_64/multiarch/../strchr.S line 32
  • #1 expand_macro
    at ../../../../gio/gdesktopappinfo.c line 2331
  • #2 expand_application_parameters
    at ../../../../gio/gdesktopappinfo.c line 2421
  • #3 g_desktop_app_info_launch_uris_with_spawn
    at ../../../../gio/gdesktopappinfo.c line 2682
  • #4 g_desktop_app_info_launch_uris_internal
    at ../../../../gio/gdesktopappinfo.c line 2923
  • #5 g_desktop_app_info_launch_uris
    at ../../../../gio/gdesktopappinfo.c line 2946
  • #6 launch_application_with_uri
  • #7 handle_open_in_thread_func
    at src/open-uri.c line 545
  • #8 g_task_thread_pool_thread
    at ../../../../gio/gtask.c line 1328
  • #9 g_thread_pool_thread_proxy
    at ../../../../glib/gthreadpool.c line 307
  • #10 g_thread_proxy
    at ../../../../glib/gthread.c line 784
  • #11 start_thread
    at pthread_create.c line 333
  • #12 clone
    at ../sysdeps/unix/sysv/linux/x86_64/clone.S line 97

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
Comment 1 Mario Sánchez Prada 2017-12-08 12:50:44 UTC
(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
Comment 2 Mario Sánchez Prada 2017-12-08 12:51:49 UTC
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().
Comment 3 Mario Sánchez Prada 2017-12-08 12:52:01 UTC
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.
Comment 4 Mario Sánchez Prada 2017-12-08 12:52:11 UTC
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.
Comment 5 Philip Withnall 2017-12-08 13:46:47 UTC
Review of attachment 365240 [details] [review]:

Yes.
Comment 6 Philip Withnall 2017-12-08 13:46:52 UTC
Review of attachment 365241 [details] [review]:

Yes!
Comment 7 Philip Withnall 2017-12-08 13:46:56 UTC
Review of attachment 365242 [details] [review]:

YES!
Comment 8 Philip Withnall 2017-12-08 13:47:49 UTC
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
Comment 9 Philip Withnall 2017-12-08 13:51:56 UTC
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()