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 657120 - get_entries_for_keyval reliably segfaults with introspection
get_entries_for_keyval reliably segfaults with introspection
Status: RESOLVED FIXED
Product: pygobject
Classification: Bindings
Component: introspection
Git master
Other Linux
: Normal major
: ---
Assigned To: Nobody's working on this now (help wanted and appreciated)
Python bindings maintainers
Depends on:
Blocks: 626252 626254
 
 
Reported: 2011-08-22 22:05 UTC by Joanmarie Diggs (IRC: joanie)
Modified: 2011-08-26 16:56 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
backtrace (5.33 KB, application/octet-stream)
2011-08-22 22:05 UTC, Joanmarie Diggs (IRC: joanie)
  Details
fix inline struct array handling (3.16 KB, patch)
2011-08-23 18:22 UTC, johnp
committed Details | Review
Patch to again copy the in-line structs before freeing them. (1.53 KB, patch)
2011-08-24 22:42 UTC, Mike Gorse
committed Details | Review

Description Joanmarie Diggs (IRC: joanie) 2011-08-22 22:05:05 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.
Comment 1 johnp 2011-08-22 23:06:37 UTC
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.
Comment 2 johnp 2011-08-22 23:10:58 UTC
Walters, can you comment.  Is gjs determining how to decode an array differently?
Comment 3 Colin Walters 2011-08-23 01:35:14 UTC
(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.
Comment 4 Mike Gorse 2011-08-23 02:14:56 UTC
Fwiw, GdkKeymapKey is not boxed, so doing this would fix this particular crash.
Comment 5 johnp 2011-08-23 18:22:05 UTC
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
Comment 6 johnp 2011-08-23 18:22:47 UTC
please check to see if this works for you.  Thanks.
Comment 7 Mike Gorse 2011-08-23 18:46:23 UTC
It works for me, anyway (I no longer get the crash when running the test that Joanie filed).
Comment 8 Joanmarie Diggs (IRC: joanie) 2011-08-23 22:32:36 UTC
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!
Comment 9 Mike Gorse 2011-08-24 20:21:19 UTC
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.
Comment 10 Mike Gorse 2011-08-24 22:42:31 UTC
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.
Comment 11 johnp 2011-08-25 15:59:42 UTC
(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.
Comment 12 johnp 2011-08-25 17:32:58 UTC
Attachment 194522 [details] pushed as f38511f - fix inline struct array handling
Comment 13 Torsten Schoenfeld 2011-08-26 15:56:19 UTC
You could use g_type_info_is_pointer on the element type to check whether pointers or values are wanted.
Comment 14 johnp 2011-08-26 16:56:45 UTC
(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.