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 703662 - Marshaling a list of GValues causes segfaults
Marshaling a list of GValues causes segfaults
Status: RESOLVED FIXED
Product: pygobject
Classification: Bindings
Component: general
3.8.x
Other Linux
: Normal normal
: GNOME 3.10
Assigned To: Nobody's working on this now (help wanted and appreciated)
Python bindings maintainers
Depends on: 693402
Blocks:
 
 
Reported: 2013-07-05 15:06 UTC by Christoph Reiter (lazka)
Modified: 2013-10-08 01:20 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Christoph Reiter (lazka) 2013-07-05 15:06:57 UTC
Since 3.8.3 the following segfaults:

###############

from gi.repository import Gtk, GObject

model = Gtk.ListStore(GObject.TYPE_PYOBJECT)
value = GObject.Value(GObject.TYPE_PYOBJECT)
value.set_boxed(object())
model.insert_with_valuesv(0, [0], [value])
value.set_boxed(object())
model.insert_with_valuesv(0, [0], [value])

###############

I guess related to https://bugzilla.gnome.org/show_bug.cgi?id=701058
Comment 1 Simon Feltman 2013-07-05 17:14:34 UTC
Verified. Do you know if this ever worked in prior versions of PyGObject, i.e. 3.4 ?
Comment 2 Christoph Reiter (lazka) 2013-07-05 17:55:08 UTC
It has worked since 3.2
Comment 3 Christoph Reiter (lazka) 2013-07-06 09:24:07 UTC
Ok, correction. The above doesn't work with 3.2 and 3.4 (3.6 untested) and works with 3.8.

It works in my code for older versions... no idea why..

I was using it because it reduces the insert time by 30%.
Comment 4 Simon Feltman 2013-07-06 11:46:00 UTC
That makes more sense :) Also note that there was never a 3.6 release because PyGObject skipped from 3.4 to 3.8 to match GNOME versioning. When reading the code it looks like this never worked except in 3.8 where it working is a side effect of the commit which broke the GValue passed by reference:

https://git.gnome.org/browse/pygobject/commit/?id=9e47afe459df942d9f

My gut reaction is we can't safely do much for 3.8 except back out the fix for bug 701058. I can fiddle around with a hack, but this part of the code looks like a minefield. To fix this correctly we will need to re-work the cachers to contain more information (which I think is out of scope for the 3.8 branch).

Specifically the problem is the marshaling cleanup for GValue grabs the incoming Python arg type from the invoke state. Meaning for arrays, the type check "py_object_type != G_TYPE_VALUE" will always succeed because it is checking the array type, not the item type, and prematurely free the GValue which is owned by Python:

https://git.gnome.org/browse/pygobject/tree/gi/pygi-marshal-cleanup.c?id=3.8.0#n262

This is called by the "from_py_cleanup" variable here:

https://git.gnome.org/browse/pygobject/tree/gi/pygi-marshal-from-py.c?id=3.8.0#n1011

A hack might be accomplished by doing a similar type check found in the cleanup code but in the array marshaling code, but this time really using the Python array item for the test, not the array, and conditionally call "from_py_cleanup". I hate to add more hacks here but this might just work.

A better solution would be to add a new parameter to marshaling cleanup functions. This would contain the Python object of what was marshaled. So in the normal case it would just be passed the Python function arg. But in the recursive array case, it would be the item in the array.

All this needs more eyes looking at it though.
Comment 5 Christoph Reiter (lazka) 2013-07-06 11:57:42 UTC
Thanks for looking into this. I would say leave the 3.8 branch as is..

Does this mean, reusing GValues is generally save except in arrays and the following is valid?

##############

from gi.repository import Gtk, GObject

model = Gtk.ListStore(GObject.TYPE_PYOBJECT)
value = GObject.Value(GObject.TYPE_PYOBJECT)
value.set_boxed(object())
model.set_value(model.insert(0), 0, value)
value.set_boxed(object())
model.set_value(model.insert(0), 0, value)

############
Comment 6 Christoph Reiter (lazka) 2013-07-06 12:17:30 UTC
Never mind.. that's even slower than recreating new GValues.
Comment 7 Simon Feltman 2013-07-06 13:02:12 UTC
(In reply to comment #5)
> Does this mean, reusing GValues is generally save except in arrays and the
> following is valid?

Yes, that should be safe, the bug is only with arrays containing GValue's. As a side note, I recall GValue's holding a boxed had some issues but might be fixed now, see bug 688081#c2

Another option could be to pass an array of Python values directly to insert_with_valuesv as it will automatically marshal them into GValue's:

model.insert_with_valuesv(0, [0], [object()])

This is the case where the cleanup code described in comment #4 is actually working correctly. This is also faster because the execution path is all C for creating the GValue and setting the boxed.

In [4]: %timeit model.insert_with_valuesv(0, [0], [object()])
100000 loops, best of 3: 16.8 us per loop

In [3]: %timeit model.insert_with_valuesv(0, [0], [GObject.Value(GObject.TYPE_PYOBJECT, object())])
10000 loops, best of 3: 45.5 us per loop

Actually it is a bit unclear to me why set_value and like functions are overridden in Python at all. It seems like there is already code in C for auto coercing Python values for functions which take GValue arguments. Contributions to figuring this out are very welcome :)
Comment 8 Christoph Reiter (lazka) 2013-07-06 13:14:49 UTC
Simply passing the python value would for example fail in case of integers:

>> model = Gtk.ListStore(GObject.TYPE_PYOBJECT)
>> model.insert_with_valuesv(0, [0], [2**70])
RuntimeError: Item 0: PyObject conversion to GValue failed
>> model.insert_with_valuesv(0, [0], [42])
Unable to convert from gint to PyObject
Comment 9 Simon Feltman 2013-10-07 02:40:11 UTC
I've added a patch to bug 693402 which fixes this.