[Python-checkins] bpo-38006: Add unit test for weakref clear bug (GH-16788)

[Neil Schemenauer]

https://github.com/python/cpython/commit/392a13bb9346331b087bcd8bb1b37072c126abee
commit: 392a13bb9346331b087bcd8bb1b37072c126abee
branch: master
author: Neil Schemenauer <nas-github at arctrix.com>
committer: GitHub <noreply at github.com>
date: 2019-10-15T20:56:48-07:00
summary:

bpo-38006: Add unit test for weakref clear bug (GH-16788)

files:
M Lib/test/test_gc.py
M Modules/_testcapimodule.c

diff --git a/Lib/test/test_gc.py b/Lib/test/test_gc.py
index fe9d6c2f766cc..fdb8752b66817 100644
--- a/Lib/test/test_gc.py
+++ b/Lib/test/test_gc.py
@@ -1,4 +1,5 @@
 import unittest
+import unittest.mock
 from test.support import (verbose, refcount_test, run_unittest,
                           strip_python_stderr, cpython_only, start_threads,
                           temp_dir, requires_type_collecting, TESTFN, unlink,
@@ -22,6 +23,11 @@ def __new__(cls, *args, **kwargs):
                 raise TypeError('requires _testcapi.with_tp_del')
         return C
 
+try:
+    from _testcapi import ContainerNoGC
+except ImportError:
+    ContainerNoGC = None
+
 ### Support code
 ###############################################################################
 
@@ -959,6 +965,66 @@ def getstats():
 
         gc.enable()
 
+    @unittest.skipIf(ContainerNoGC is None,
+                     'requires ContainerNoGC extension type')
+    def test_trash_weakref_clear(self):
+        # Test that trash weakrefs are properly cleared (bpo-38006).
+        #
+        # Structure we are creating:
+        #
+        #   Z <- Y <- A--+--> WZ -> C
+        #             ^  |
+        #             +--+
+        # where:
+        #   WZ is a weakref to Z with callback C
+        #   Y doesn't implement tp_traverse
+        #   A contains a reference to itself, Y and WZ
+        #
+        # A, Y, Z, WZ are all trash.  The GC doesn't know that Z is trash
+        # because Y does not implement tp_traverse.  To show the bug, WZ needs
+        # to live long enough so that Z is deallocated before it.  Then, if
+        # gcmodule is buggy, when Z is being deallocated, C will run.
+        #
+        # To ensure WZ lives long enough, we put it in a second reference
+        # cycle.  That trick only works due to the ordering of the GC prev/next
+        # linked lists.  So, this test is a bit fragile.
+        #
+        # The bug reported in bpo-38006 is caused because the GC did not
+        # clear WZ before starting the process of calling tp_clear on the
+        # trash.  Normally, handle_weakrefs() would find the weakref via Z and
+        # clear it.  However, since the GC cannot find Z, WR is not cleared and
+        # it can execute during delete_garbage().  That can lead to disaster
+        # since the callback might tinker with objects that have already had
+        # tp_clear called on them (leaving them in possibly invalid states).
+
+        callback = unittest.mock.Mock()
+
+        class A:
+            __slots__ = ['a', 'y', 'wz']
+
+        class Z:
+            pass
+
+        # setup required object graph, as described above
+        a = A()
+        a.a = a
+        a.y = ContainerNoGC(Z())
+        a.wz = weakref.ref(a.y.value, callback)
+        # create second cycle to keep WZ alive longer
+        wr_cycle = [a.wz]
+        wr_cycle.append(wr_cycle)
+        # ensure trash unrelated to this test is gone
+        gc.collect()
+        gc.disable()
+        # release references and create trash
+        del a, wr_cycle
+        gc.collect()
+        # if called, it means there is a bug in the GC.  The weakref should be
+        # cleared before Z dies.
+        callback.assert_not_called()
+        gc.enable()
+
+
 class GCCallbackTests(unittest.TestCase):
     def setUp(self):
         # Save gc state and disable it.
diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c
index c009d254da8f1..a18a8e7553ef2 100644
--- a/Modules/_testcapimodule.c
+++ b/Modules/_testcapimodule.c
@@ -6538,6 +6538,53 @@ static PyTypeObject MethStatic_Type = {
         "Class with static methods to test calling conventions"),
 };
 
+/* ContainerNoGC -- a simple container without GC methods */
+
+typedef struct {
+    PyObject_HEAD
+    PyObject *value;
+} ContainerNoGCobject;
+
+static PyObject *
+ContainerNoGC_new(PyTypeObject *type, PyObject *args, PyObject *kwargs)
+{
+    PyObject *value;
+    char *names[] = {"value", NULL};
+    if (!PyArg_ParseTupleAndKeywords(args, kwargs, "O", names, &value)) {
+        return NULL;
+    }
+    PyObject *self = type->tp_alloc(type, 0);
+    if (self == NULL) {
+        return NULL;
+    }
+    Py_INCREF(value);
+    ((ContainerNoGCobject *)self)->value = value;
+    return self;
+}
+
+static void
+ContainerNoGC_dealloc(ContainerNoGCobject *self)
+{
+    Py_DECREF(self->value);
+    Py_TYPE(self)->tp_free((PyObject *)self);
+}
+
+static PyMemberDef ContainerNoGC_members[] = {
+    {"value", T_OBJECT, offsetof(ContainerNoGCobject, value), READONLY,
+     PyDoc_STR("a container value for test purposes")},
+    {0}
+};
+
+static PyTypeObject ContainerNoGC_type = {
+    PyVarObject_HEAD_INIT(NULL, 0)
+    "_testcapi.ContainerNoGC",
+    sizeof(ContainerNoGCobject),
+    .tp_dealloc = (destructor)ContainerNoGC_dealloc,
+    .tp_flags = Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE,
+    .tp_members = ContainerNoGC_members,
+    .tp_new = ContainerNoGC_new,
+};
+
 
 static struct PyModuleDef _testcapimodule = {
     PyModuleDef_HEAD_INIT,
@@ -6735,6 +6782,14 @@ PyInit__testcapi(void)
     Py_DECREF(subclass_with_finalizer_bases);
     PyModule_AddObject(m, "HeapCTypeSubclassWithFinalizer", HeapCTypeSubclassWithFinalizer);
 
+    if (PyType_Ready(&ContainerNoGC_type) < 0) {
+        return NULL;
+    }
+    Py_INCREF(&ContainerNoGC_type);
+    if (PyModule_AddObject(m, "ContainerNoGC",
+                           (PyObject *) &ContainerNoGC_type) < 0)
+        return NULL;
+
     PyState_AddModule(m, &_testcapimodule);
     return m;
 }