GNOME Bugzilla – Bug 688081
Change default void pointer marshaling to only allow integer values
Last modified: 2013-09-26 02:03:24 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.
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.
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.
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.
Created attachment 248536 [details] [review] Move PyGIDeprecationWarning to C for shared Python/C usage
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.
Created attachment 250031 [details] [review] Fix indentation for pyg_value_from_pyobject
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
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.
Created attachment 250034 [details] [review] Final deprecation of allowing PyObjects as void pointers
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 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 on attachment 250031 [details] [review] Fix indentation for pyg_value_from_pyobject Please feel free to push such trivial stuff without review.
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!
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
*** Bug 660056 has been marked as a duplicate of this bug. ***
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.