GNOME Bugzilla – Bug 609101
[PATCH] assigning key binding to list of keys always causes crash
Last modified: 2012-02-08 14:03:18 UTC
Created attachment 153091 [details] [review] metacity-key-list-binding.patch Metacity is capable to assign a function to more than one key using "_list" gconf key name and string array list value. However any attempt to use this feature always causes a crash. How to reproduce: For example setting /apps/metacity/global_keybindings/panel_main_menu_list to string array [Super_L,Super_R] init_bindings() wants to initialize key bindings. Key panel_main_menu_list (guessingh that this value does not come from upstream but gconf2-branding-sled) ends with "_list" and it is processed specially by update_key_list_binding(key,list_val) update_key_list_binding(key,list_val) calls find_and_update_list_binding(key_bindings, name==key, value==list_val) It cuts the key suffix ("panel_main_menu") does some processing and calls: update_list_binding(key_bindings,value,type_of_value) Type of value is always META_LIST_OF_GCONFVALUE_STRINGS (well, strange - the function is static, called only from a single place, value can never have different value), the code reaches: pref_string = gconf_value_get_string (pref_iterator->data); It always fails (the value in question is string list, not string) and pref_string is NULL. NULL is always passed to meta_ui_parse_accelerator (pref_string==NULL, &keysym, &keycode, &mods) and it always crashes. Attached patch is the minimal patch. But it seems the code wants more changes: There is no use for the other of META_LIST_OF_STRINGS/META_LIST_OF_GCONFVALUE_STRINGS. Argument is always the same. The whole MetaStringListType has no use - only one value is actually actively used. Third argument of update_list_binding() is always the same and can be removed. update_key_list_binding() seems to be intermediate function that only adds one level of indirection with no reason. Maybe meta_ui_parse_accelerator() should check accel for NULL or use g_strcmp0. Well, and there is yet another NULL sanity check missing, but it seems to be safe with the current implementation: list_val can be NULL if the key has suffix "_list" but the value is is not a list.
Forgot the backtrace: Program received signal SIGSEGV, Segmentation fault. 0x000000000046312b in meta_ui_parse_accelerator (accel=0x0, keysym=0x7fffffffdce4, keycode=0x7fffffffdce0, mask=0x7fffffffdcdc) at ui/ui.c:768
+ Trace 220434
Created attachment 154165 [details] [review] Fix crash on startup with list bindings Just hit and debugged the same crash in the context of Mutter; I don't think that your patch is really right - in the case when the list key is changed on the fly, the list passed into update_key_binding() is a list of GConfValue, not a list of strings; so we actually need to keep the MetaStringListType and pass it down. If my understanding is correct, your patch will cause misbehavior or a crash when the _list key is changed on the fly while Metacity is running. You are right that the indirection in update_key_list_binding() / find_and_update_list_binding() is a bit silly, but I've left that untouched - it's not really doing any harm.
Review of attachment 153091 [details] [review]: Marking your patch needs-work based on my analysis above.
Attachment 154165 [details] pushed as 8875e73 - Fix crash on startup with list bindings
Your fix does not crash while changing the key list on the fly. But it is still not fully correct: - Change of foo_list on the fly has no effect. One has to change key foo to get new value of foo_list into effect.
Update: Only removal of foo_list has no immediate effect. Other changes (addition, change) seems to work.
Can we get Owen's patch committed to metacity? :-)
Pretty sure this is obsolete with the move to gsettings.