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 787069 - gdk_keymap_get_entries_for_keyval() returns TRUE with n_keys=0 in Wayland
gdk_keymap_get_entries_for_keyval() returns TRUE with n_keys=0 in Wayland
Status: RESOLVED OBSOLETE
Product: gtk+
Classification: Platform
Component: Backend: Wayland
3.22.x
Other Linux
: Normal normal
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks:
 
 
Reported: 2017-08-31 09:59 UTC by Carlos Garcia Campos
Modified: 2018-05-02 19:02 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gdk: wayland, mir: fix return value of gdk_keymap_get_entries_for_keyval() (1.52 KB, patch)
2017-08-31 10:00 UTC, Carlos Garcia Campos
none Details | Review

Description Carlos Garcia Campos 2017-08-31 09:59:11 UTC
The documentation says that TRUE is returned if keys were found and returned and that's what all other backends do (except mir that is broken too). This is causing crashes in WebKit that checks only the return value, and assumes the returned array has at least one item. See WebKit bug https://bugs.webkit.org/show_bug.cgi?id=176154
Comment 1 Carlos Garcia Campos 2017-08-31 10:00:33 UTC
Created attachment 358839 [details] [review]
gdk: wayland, mir: fix return value of gdk_keymap_get_entries_for_keyval()
Comment 2 Christian Persch 2017-08-31 22:43:49 UTC
Existing code using this may do

if (gdk_keymap_get_entries_for_keyval(..., &keys, &n_keys)) {
  /* do something */
  g_free(keys);
}

(e.g. firefox does this).

The patch returns FALSE on n_keys == 0, but still keys != NULL, which means a leak on the code pattern above. Should also change to set return keys = NULL in this case, I think.
Comment 3 Carlos Garcia Campos 2017-09-01 06:45:11 UTC
(In reply to Christian Persch from comment #2)
> Existing code using this may do
> 
> if (gdk_keymap_get_entries_for_keyval(..., &keys, &n_keys)) {
>   /* do something */
>   g_free(keys);
> }
> 
> (e.g. firefox does this).
> 
> The patch returns FALSE on n_keys == 0, but still keys != NULL, which means
> a leak on the code pattern above. Should also change to set return keys =
> NULL in this case, I think.

I'm not sure that can happen, at least in the mir and wayland implementations, if n_keys == 0, then they GArray was created but never changed, so the internal pointer is still NULL. The GArray itself is freed, so I don't think there's any leak. We could still do the same X11 backend does to be extra sure:

  if (retval->len > 0)
    {
      *keys = (GdkKeymapKey*) retval->data;
      *n_keys = retval->len;
    }
  else
    {
      *keys = NULL;
      *n_keys = 0;
    }

  g_array_free (retval, retval->len > 0 ? FALSE : TRUE);

  return *n_keys > 0;
Comment 4 GNOME Infrastructure Team 2018-05-02 19:02:22 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gtk/issues/899.