GNOME Bugzilla – Bug 669415
Use Py_XDECREF to unref marshalled errors in pyglib_error_check
Last modified: 2012-02-06 12:56:50 UTC
Currently, Py_DECREF is used to unref the python exception that has been marshalled from a GError **. However, when there is no error set, the marshalled value is NULL. Py_DECREF does not handle this and the result is a SEGV. Using Py_XDECREF instead solves this problem.
Created attachment 206838 [details] [review] Use Py_XDECREF to unref marshalled errors in pyglib_error_check
Created attachment 206854 [details] [review] patch
Created attachment 206855 [details] [review] patch
Actually my pygobject unit test does not pass: the callback is not invoked: either I am missing something really obvious in my test or the test shows a further bug (though at least with Jesse's patch the crash is gone).
Adding in CC the authors of the patch that added support for GErrors in callbacks
Paolo, unfortunately I cannot help with understanding why the callback is invoked. But there is indeed an error in the current code, which probably happened when merging my code with Will's. When I submitted my patch, the pyglib_error_check() function was: ============== gboolean pyglib_error_check(GError **error) { PyGILState_STATE state; PyObject *exc_type; PyObject *exc_instance; PyObject *d; g_return_val_if_fail(error != NULL, FALSE); if (*error == NULL) return FALSE; state = pyglib_gil_state_ensure(); ... ============== Notice how it would return FALSE if the GError contents are NULL. The current master has instead: ============== gboolean pyglib_error_check(GError **error) { PyGILState_STATE state; PyObject *exc_instance; g_return_val_if_fail(error != NULL, FALSE); state = pyglib_gil_state_ensure(); exc_instance = pyglib_error_marshal (error); PyErr_SetObject(_PyGLib_API->gerror_exception, exc_instance); Py_DECREF(exc_instance); g_clear_error(error); ============== The new pyglib_error_marshal() function returns a NULL object when the GError contents are NULL, so calling PyDECREF() on it indeed causes problems. I don't know much about python-gobject (or the Python/C API), so I cannot say whether the line PyErr_SetObject(_PyGLib_API->gerror_exception, exc_instance); can cause some unwanted side effects when exc_instance is NULL. Maybe it's a good idea to put back the "if (*error == NULL) return FALSE" shortcut at the top of pyglib_error_check as well?
Indeed, this was a regression from http://git.gnome.org/browse/pygobject/commit/?id=adcfe96d49b09bc. I committed http://git.gnome.org/browse/pygobject/commit/?id=a41984780ee49dcf02c718ca1be87bba747472e5 which I believe should fix this. Can you please verify? Thanks!
Comment on attachment 206854 [details] [review] patch Paolo's new test case nicely reproduces this, I pushed the patches to g-i and pygobject. Thanks!
*** Bug 668862 has been marked as a duplicate of this bug. ***