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 688081 - Change default void pointer marshaling to only allow integer values
Change default void pointer marshaling to only allow integer values
Status: RESOLVED FIXED
Product: pygobject
Classification: Bindings
Component: introspection
unspecified
Other Linux
: Normal normal
: GNOME 3.10
Assigned To: Nobody's working on this now (help wanted and appreciated)
Python bindings maintainers
: 660056 (view as bug list)
Depends on:
Blocks: 666636 692918 693405 694857 699435
 
 
Reported: 2012-11-11 08:47 UTC by Simon Feltman
Modified: 2013-09-26 02:03 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add deprecation warning for marshaling arbitrary objects as pointers (2.56 KB, patch)
2013-07-06 21:20 UTC, Simon Feltman
none Details | Review
Move PyGIDeprecationWarning to C for shared Python/C usage (3.29 KB, patch)
2013-07-06 21:21 UTC, Simon Feltman
committed Details | Review
Add deprecation warning for marshaling arbitrary objects as pointers (2.56 KB, patch)
2013-07-06 21:21 UTC, Simon Feltman
committed Details | Review
Fix indentation for pyg_value_from_pyobject (29.52 KB, patch)
2013-07-24 12:11 UTC, Simon Feltman
committed Details | Review
Refactor pyg_value_from_pyobject into two functions (9.53 KB, patch)
2013-07-24 12:11 UTC, Simon Feltman
committed Details | Review
Override GValue.set/get_boxed with static C marshaler (3.17 KB, patch)
2013-07-24 12:11 UTC, Simon Feltman
committed Details | Review
Final deprecation of allowing PyObjects as void pointers (3.67 KB, patch)
2013-07-24 12:15 UTC, Simon Feltman
committed Details | Review

Description Simon Feltman 2012-11-11 08:47:28 UTC
This is a similar problem as described in bug 683599 but applied to generic argument marshaling. A void pointer argument described in gi is by default marshaled from python using the pointer to the python object itself. This does not seem like a good generic default as it is basically unknowable to gi what the pointer is supposed to be. This leads to crashers like bug 633927.

To be on the safe side we should only support integer values as arguments for void pointers generically as was the solution to bug 683599. This way overrides which know about what the pointer is supposed to mean can set this value explicitly.

From what I recall, cases like user data for callbacks are already special cased in pygi to use explicit structures holding pointers to the callback and user data python object. So these should continue to work.

The related code is gi/pygi-marshal-from-py.c:_pygi_marshal_from_py_void

This marshaler should raise an exception if the python argument is not an integer and use the integer value as the actual argument going to C.
Comment 1 Simon Feltman 2012-11-11 22:30:01 UTC
After testing a deprecation warning in _pygi_marshal_from_py_void I found only one occurance of its usage in the unittests. TreeModel.append accepts any arbitrary python object and uses it as the parameter to GValue.set_boxed. Although I don't fully understand boxing yet, this seems like it might be an abuse of the method as we are not actually setting a boxed but a python object pointer. This exposes potential dangers as the tree model might be trying to manager this pointer as a boxed type.
Comment 2 Simon Feltman 2012-11-12 03:05:27 UTC
It turns out the static bindings are creating a new boxed type for python object pointers in gi/_gobject/pygobject.c:pygobject_object_register_types. This seems to be working fairly well and I was only able to find one ref leak when using pyobjects held within a GValue, and it looks like it can be easily fixed (bug 688137)

We should special case overrides for GObject.Value.get/set_boxed and actually wrap the python objects in a python boxed wrapper that is passed to the get/set_boxed. This will also allow for generic usage of any boxed type for the argument (not just TYPE_PYOBJECT). And also allow for deprecation of the generic pyobject pointer marshaling as a gpointer argument.
Comment 3 Simon Feltman 2013-07-06 21:20:25 UTC
Created attachment 248535 [details] [review]
Add deprecation warning for marshaling arbitrary objects as pointers

Add deprecation warning for marshaling arbitrary objects to/from void
pointers with the exception of integers, PyCapsules, and None.
Comment 4 Simon Feltman 2013-07-06 21:21:08 UTC
Created attachment 248536 [details] [review]
Move PyGIDeprecationWarning to C for shared Python/C usage
Comment 5 Simon Feltman 2013-07-06 21:21:58 UTC
Created attachment 248537 [details] [review]
Add deprecation warning for marshaling arbitrary objects as pointers

Add deprecation warning for marshaling arbitrary objects to/from void
pointers with the exception of integers, PyCapsules, and None.
Comment 6 Simon Feltman 2013-07-24 12:11:01 UTC
Created attachment 250031 [details] [review]
Fix indentation for pyg_value_from_pyobject
Comment 7 Simon Feltman 2013-07-24 12:11:04 UTC
Created attachment 250032 [details] [review]
Refactor pyg_value_from_pyobject into two functions

Break pyg_value_from_pyobject into two functions. One which keeps
Python exceptions queued (pyg_value_from_pyobject_with_error) and
one which clears them (pyg_value_from_pyobject). This allows for
re-use for code which want to keep the errors around
Comment 8 Simon Feltman 2013-07-24 12:11:07 UTC
Created attachment 250033 [details] [review]
Override GValue.set/get_boxed with static C marshaler

Override boxed type get/set methods on GValue to use the static C
GValue marshaler. This works around the inability of the introspection
version of these methods to know what the held GValue type is.
With this, all boxed types will now marshal properly with GValues as
their storage.
Comment 9 Simon Feltman 2013-07-24 12:15:21 UTC
Created attachment 250034 [details] [review]
Final deprecation of allowing PyObjects as void pointers
Comment 10 Martin Pitt 2013-07-25 13:09:47 UTC
Comment on attachment 250033 [details] [review]
Override GValue.set/get_boxed with static C marshaler

LGTM (this is the requirement for bug 694857, right?)
Comment 11 Martin Pitt 2013-07-25 13:15:55 UTC
Comment on attachment 248537 [details] [review]
Add deprecation warning for marshaling arbitrary objects as pointers

Thanks for this! Let's get this in ASAP so that people start seeing it and fix their software.
Comment 12 Martin Pitt 2013-07-25 13:17:24 UTC
Comment on attachment 250031 [details] [review]
Fix indentation for pyg_value_from_pyobject

Please feel free to push such trivial stuff without review.
Comment 13 Martin Pitt 2013-07-25 13:20:21 UTC
Comment on attachment 250034 [details] [review]
Final deprecation of allowing PyObjects as void pointers

This looks fine, but we should stash this for at least 3.11.x, so that we allow developers to fix their software.

Thanks!
Comment 14 Simon Feltman 2013-07-25 20:01:23 UTC
Attachment 248536 [details] pushed as 077aefe - Move PyGIDeprecationWarning to C for shared Python/C usage
Attachment 248537 [details] pushed as 6a29d9b - Add deprecation warning for marshaling arbitrary objects as pointers
Attachment 250031 [details] pushed as 84e91a9 - Fix indentation for pyg_value_from_pyobject
Attachment 250032 [details] pushed as 2cff482 - Refactor pyg_value_from_pyobject into two functions
Attachment 250033 [details] pushed as dd43a1e - Override GValue.set/get_boxed with static C marshaler
Comment 15 Simon Feltman 2013-09-13 23:43:36 UTC
*** Bug 660056 has been marked as a duplicate of this bug. ***
Comment 16 Simon Feltman 2013-09-26 02:03:16 UTC
Committed final removal in 3.11. This shouldn't break any uses of void
pointers that are not already broken and actually allows passing NULL where
valid (instead of the address of Py_None or Py 0. Either because the C API
would have no idea that the pointer is a PyObject or because ref management
would be fragile at best.