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 669415 - Use Py_XDECREF to unref marshalled errors in pyglib_error_check
Use Py_XDECREF to unref marshalled errors in pyglib_error_check
Product: pygobject
Classification: Bindings
Component: general
Other All
: Normal normal
: ---
Assigned To: Nobody's working on this now (help wanted and appreciated)
Python bindings maintainers
: 668862 (view as bug list)
Depends on:
Reported: 2012-02-05 15:31 UTC by
Modified: 2012-02-06 12:56 UTC
See Also:
GNOME target: ---
GNOME version: ---

Use Py_XDECREF to unref marshalled errors in pyglib_error_check (829 bytes, patch)
2012-02-05 15:31 UTC,
none Details | Review
patch (2.69 KB, patch)
2012-02-05 18:00 UTC, Paolo Borelli
committed Details | Review
patch (1.14 KB, patch)
2012-02-05 18:00 UTC, Paolo Borelli
committed Details | Review

Description 2012-02-05 15:31:29 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.
Comment 1 2012-02-05 15:31:31 UTC
Created attachment 206838 [details] [review]
Use Py_XDECREF to unref marshalled errors in pyglib_error_check
Comment 2 Paolo Borelli 2012-02-05 18:00:18 UTC
Created attachment 206854 [details] [review]
Comment 3 Paolo Borelli 2012-02-05 18:00:44 UTC
Created attachment 206855 [details] [review]
Comment 4 Paolo Borelli 2012-02-05 21:16:57 UTC
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).
Comment 5 Paolo Borelli 2012-02-05 21:26:56 UTC
Adding in CC the authors of the patch that added support for GErrors in callbacks
Comment 6 Mardy 2012-02-06 07:47:17 UTC
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:

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:

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);

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?
Comment 7 Martin Pitt 2012-02-06 08:37:19 UTC
Indeed, this was a regression from

I committed

which I believe should fix this. Can you please verify?

Comment 8 Martin Pitt 2012-02-06 08:42:25 UTC
Comment on attachment 206854 [details] [review]

Paolo's new test case nicely reproduces this, I pushed the patches to g-i and pygobject. Thanks!
Comment 9 Christian Fredrik Kalager Schaller 2012-02-06 12:56:50 UTC
*** Bug 668862 has been marked as a duplicate of this bug. ***