Skip to content

keybindings: Add refcounting to fix use-after-free for key handlers

Keyu Tao requested to merge taoky/mutter:fix-keybinding-use-after-free into main

When meta_prefs_remove_keybinding() is called by meta_display_remove_keybinding(), it would call queue_changed() and let prefs changed callback listener run later with a lower priority. However, this brings a race condition issue when binding's name and handler could be invalid for a short time, and it could crash mutter when there are keypresses triggering related handlers.

The commit fixes this by emit changed (call listener) immediately inside meta_prefs_remove_keybinding().

A new GHashTable is added in keybindings.c, which uses handler as key and a GHashTable of all related bindings as value. Before freeing handler in meta_display_remove_keybinding(), its related bindings within keys->key_bindings and keys->key_bindings_index should be removed to avoid use after free bug under race condition.

The new GHashTable is used to avoid a full iteration of the two key bindings hashtables.

Two new fields: ref_count and removed, are added to MetaKeyHandler, and it would be freed only if the ref count has reached 0. When handler is removed from key_handlers GHashTable, key_handler_destroy() would mark removed as TRUE, and do an unref. handler->removed is checked in get_keybinding, and binding with handler removed would not be used.

Also in MetaKeyBinding, it now has the ownership of the name field, to avoid it being freed before logging. Create or copy a binding would do a ref inc for handler, and free one would unref handler.

Fixes gnome-shell#1870 (closed).

Edited by Keyu Tao

Merge request reports