GNOME Bugzilla – Bug 657120
get_entries_for_keyval reliably segfaults with introspection
Last modified: 2011-08-26 16:56:45 UTC
Created attachment 194436 [details] backtrace Fresh install of Fedora Rawhide, pygobject from master, along with 2.28.6 built with introspection disabled. $ python >>> from gi.repository import Gdk >>> keymap = Gdk.Keymap.get_default() >>> keyval = Gdk.keyval_from_name('KP_Divide') >>> success, entries = keymap.get_entries_for_keyval(keyval) Segmentation fault The impact of this is that Orca hangs if introspection is used and a key is pressed.
This is an issue with the lack of ability to tell if a C array contains pointers to a struct (struct **) or the if the structs are interned as part of the array (struct *). In the case of out values this would be struct *** and struct ** respectively. In this case the entries in the keymap are returned as an array of interned stucts. You can see the commit that broke this interface: http://git.gnome.org/browse/pygobject/commit/?id=c9d9ffd0380878792cbdb13dec4e53be897e5fbc We need a way of determining if we should be decoding a pointer or an interned struct. The form: item_arg.v_pointer = array_->data + i * item_size; should be used to decode an interned struct and: item_arg.v_pointer = g_array_index (array_, gpointer, i); should be used to to decode a pointer to a struct.
Walters, can you comment. Is gjs determining how to decode an array differently?
(In reply to comment #2) > Walters, can you comment. Is gjs determining how to decode an array > differently? I think no one is calling this from gjs. As far as arrays and depth - g-i doesn't presently tell in the .gir now, and we don't have any annotations. Note we don't even have any rule that says how to free passed arrays, though one could probably just assume g_free(). However, I would apply the following rules: 1) If the element type is boxed, GObject, or string, it's not a flat array 2) If the element type is a non-boxed structure or union, it is flat 3) If the element type is anything else (integer, float) it is flat It's basically broken for code to return non-flat arrays in case 2) because you don't know how to free the elements.
Fwiw, GdkKeymapKey is not boxed, so doing this would fix this particular crash.
Created attachment 194522 [details] [review] fix inline struct array handling * we now assume any non-boxed structs are inline in an array since there is no way to check in GI and this is the most common use for an array of non-boxed structs
please check to see if this works for you. Thanks.
It works for me, anyway (I no longer get the crash when running the test that Joanie filed).
Well, the good news is that the segfault is gone. The bad news is that I'm now once again seeing bug 653588 which, in terms of Orca's migration to introspection is just as toxic I'm afraid. Sorry!
The old code had a g_malloc and memcpy to copy the struct, and the patch removes that bit of code. I don't know if you intended to remove it, but _pygi_marshal_free_out_array eventually frees the array data for transfer full, and the struct was never copied.
Created attachment 194660 [details] [review] Patch to again copy the in-line structs before freeing them. I believe this is the correct behavior; apply on top of the last patch.
(In reply to comment #10) > Created an attachment (id=194660) [details] [review] > Patch to again copy the in-line structs before freeing them. > > I believe this is the correct behavior; apply on top of the last patch. You are correct. I'll commit these patches today. Thanks.
Attachment 194522 [details] pushed as f38511f - fix inline struct array handling
You could use g_type_info_is_pointer on the element type to check whether pointers or values are wanted.
(In reply to comment #13) > You could use g_type_info_is_pointer on the element type to check whether > pointers or values are wanted. is_pointer doesn't do what you think it does. In fact is_pointer is poorly defined.