From 92aca4416a7930e5870b8d1a4016bae8140462ee Mon Sep 17 00:00:00 2001

From: Daniel Drake <dsd@laptop.org>

Date: Fri, 3 Jun 2011 16:59:15 +0100

Subject: [PATCH] Fix GC-related crash during PyGObject deallocation

Python-2.7.1's GC source has the following comment:

        /* Python's cyclic gc should never see an incoming refcount

         * of 0:  if something decref'ed to 0, it should have been

         * deallocated immediately at that time.

         * Possible cause (if the assert triggers):  a tp_dealloc

         * routine left a gc-aware object tracked during its teardown

         * phase, and did something-- or allowed something to happen --

         * that called back into Python.  gc can trigger then, and may

         * see the still-tracked dying object.  Before this assert

         * was added, such mistakes went on to allow gc to try to

         * delete the object again.  In a debug build, that caused

         * a mysterious segfault, when _Py_ForgetReference tried

         * to remove the object from the doubly-linked list of all

         * objects a second time.  In a release build, an actual

         * double deallocation occurred, which leads to corruption

         * of the allocator's internal bookkeeping pointers.  That's

         * so serious that maybe this should be a release-build

         * check instead of an assert?

         */

As shown in a backtrace at

https://bugzilla.redhat.com/show_bug.cgi?id=640972 , pygobject is making

this exact mistake. Before untracking its object, pygobject_dealloc

calls PyObject_ClearWeakRefs() which can call back into python, create

new allocations, and trigger the GC.

This is causing Sugar (based on pygobject2 + pygtk2 static bindings) to

crash on a regular basis while interacting with widgets or launching

applications.

Fix this by untracking the object early. Also fix the same issue spotted

in the GSource wrapper.

Thanks to Bernie Innocenti for initial diagnosis.

---

 glib/pygsource.c    |    6 ++++--

 gobject/pygobject.c |    8 +++++++-

 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/glib/pygsource.c b/glib/pygsource.c

index 684711e..d0176ab 100644

--- a/glib/pygsource.c

+++ b/glib/pygsource.c

@@ -387,10 +387,12 @@ pyg_source_clear(PyGSource *self)

 static void

 pyg_source_dealloc(PyGSource *self)

 {

-    PyObject_ClearWeakRefs((PyObject *)self);

-

+    /* Must be done first, so that there is no chance of Python's GC being

+     * called while tracking this half-deallocated object */

     PyObject_GC_UnTrack((PyObject *)self);

+    PyObject_ClearWeakRefs((PyObject *)self);

+

     pyg_source_clear(self);

     PyObject_GC_Del(self);

diff --git a/gobject/pygobject.c b/gobject/pygobject.c

index 0faf221..3193890 100644

--- a/gobject/pygobject.c

+++ b/gobject/pygobject.c

@@ -1028,8 +1028,14 @@ PYGLIB_DEFINE_TYPE("gobject.GObject", PyGObject_Type, PyGObject);

 static void

 pygobject_dealloc(PyGObject *self)

 {

-    PyObject_ClearWeakRefs((PyObject *)self);

+    /* Untrack must be done first. This is because followup calls such as

+     * ClearWeakRefs could call into Python and cause new allocations to

+     * happen, which could in turn could trigger the garbage collector,

+     * which would then get confused as it is tracking this half-deallocated

+     * object. */

     PyObject_GC_UnTrack((PyObject *)self);

+

+    PyObject_ClearWeakRefs((PyObject *)self);

       /* this forces inst_data->type to be updated, which could prove

        * important if a new wrapper has to be created and it is of a

        * unregistered type */

-- 

1.7.5.2