[pygobject: 1/2] pygobject-object: fix memory corruption around list of closures

[Christoph Reiter <creiter src gnome org>]

commit fd9e24a73acde7313fbf96c946f4ce8dad130b33
Author: Mikhail Fludkov <fludkov me gmail com>
Date:   Mon Jan 29 14:23:37 2018 +0100

    pygobject-object: fix memory corruption around list of closures
    
    https://gitlab.gnome.org/GNOME/pygobject/issues/158
    
    The memory corruption occurs because of the race while accessing
    PyGObjectData->closures list.
    
    Protect PyGObjectData->closures by GIL in pygobject_unwatch_closure.
    Despite the fact that we don't call any Python API in the function. We use
    GIL to be sure that PyGObjectData->closures list stays intact while
    GC iterating the list inside pygobject_traverse. Otherwise we can
    segfault while trying to call 'visit' function on an object that
    was just freed in pygobject_unwatch_closure.

 gi/pygobject-object.c |  5 +++++
 tests/test_signal.py  | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+)
---
diff --git a/gi/pygobject-object.c b/gi/pygobject-object.c
index 729bb4dc..304b4277 100644
--- a/gi/pygobject-object.c
+++ b/gi/pygobject-object.c
@@ -1037,7 +1037,12 @@ pygobject_unwatch_closure(gpointer data, GClosure *closure)
 {
     PyGObjectData *inst_data = data;
 
+    /* Despite no Python API is called the list inst_data->closures
+     * must be protected by GIL as it is used by GC in
+     * pygobject_traverse */
+    PyGILState_STATE state = PyGILState_Ensure();
     inst_data->closures = g_slist_remove (inst_data->closures, closure);
+    PyGILState_Release(state);
 }
 
 /**
diff --git a/tests/test_signal.py b/tests/test_signal.py
index b2f121a6..642708bf 100644
--- a/tests/test_signal.py
+++ b/tests/test_signal.py
@@ -4,12 +4,15 @@ import gc
 import unittest
 import sys
 import weakref
+import threading
+import time
 
 from gi.repository import GObject, GLib, Regress, Gio
 from gi import _signalhelper as signalhelper
 import testhelper
 from compathelper import _long
 from helper import capture_glib_warnings, capture_gi_deprecation_warnings
+from gi.module import repository as repo
 
 
 class C(GObject.GObject):
@@ -1219,6 +1222,53 @@ class TestIntrospectedSignals(unittest.TestCase):
         self.assertNotEqual(struct, held_struct)
 
 
+class TestIntrospectedSignalsIssue158(unittest.TestCase):
+    """
+    The test for https://gitlab.gnome.org/GNOME/pygobject/issues/158
+    """
+    _obj_sig_names = [sig.get_name() for sig in repo.find_by_name('Regress', 'TestObj').get_signals()]
+
+    def __init__(self, *args):
+        unittest.TestCase.__init__(self, *args)
+        self._gc_thread_stop = False
+
+    def _gc_thread(self):
+        while not self._gc_thread_stop:
+            gc.collect()
+            time.sleep(0.010)
+
+    def _callback(self, *args):
+        pass
+
+    def test_run(self):
+        """
+        Manually trigger GC from a different thread periodicaly
+        while the main thread keeps connecting/disconnecting to/from signals.
+
+        It takes a lot of time to reproduce the issue. It is possible to make it
+        fail reliably by changing the code of pygobject_unwatch_closure slightly from:
+          PyGObjectData *inst_data = data;
+          inst_data->closures = g_slist_remove (inst_data->closures, closure);
+        to
+          PyGObjectData *inst_data = data;
+          GSList *tmp = g_slist_remove (inst_data->closures, closure);
+          g_usleep(G_USEC_PER_SEC/10);
+          inst_data->closures = tmp;
+        """
+        obj = Regress.TestObj()
+        gc_thread = threading.Thread(target=self._gc_thread)
+        gc_thread.start()
+
+        for _ in range(8):
+            handlers = [obj.connect(sig, self._callback) for sig in self._obj_sig_names]
+            time.sleep(0.010)
+            while len(handlers) > 0:
+                obj.disconnect(handlers.pop())
+
+        self._gc_thread_stop = True
+        gc_thread.join()
+
+
 class _ConnectObjectTestBase(object):
     # Notes:
     #  - self.Object is overridden in sub-classes.