From 4f801636c777638c5335a120b9dafc810e4881dc Mon Sep 17 00:00:00 2001 From: Neil Schemenauer Date: Mon, 14 Oct 2019 12:14:58 -0700 Subject: [PATCH 1/4] bpo-38006: Add unit test for weakref clear bug --- Lib/test/test_gc.py | 68 +++++++++++++++++++++++++++++++++++++++ Modules/_testcapimodule.c | 52 ++++++++++++++++++++++++++++++ 2 files changed, 120 insertions(+) diff --git a/Lib/test/test_gc.py b/Lib/test/test_gc.py index fe9d6c2f766ccbe..803191fd6977909 100644 --- a/Lib/test/test_gc.py +++ b/Lib/test/test_gc.py @@ -22,6 +22,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 +964,69 @@ 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). + + wr_callback_was_run = False + def callback(wr): + # We cannot allow this callback to run. It could access objects + # with invalid states due to tp_clear already being run. Or, it + # could resurrecte objects that were previously considered trash. + nonlocal wr_callback_was_run + wr_callback_was_run = True + + 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() + # release references and create trash + del a, wr_cycle + gc.collect() + # if the callback runs, we have a bug + self.assertFalse(wr_callback_was_run) + + class GCCallbackTests(unittest.TestCase): def setUp(self): # Save gc state and disable it. diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index c009d254da8f1bc..3780f1a43c5ccf4 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -6538,6 +6538,51 @@ 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 +6780,13 @@ 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; } From 5fbc0875022ceed3594f59ead9f35e976100d44d Mon Sep 17 00:00:00 2001 From: Neil Schemenauer Date: Mon, 14 Oct 2019 15:59:50 -0700 Subject: [PATCH 2/4] Add braces as per modern style. --- Modules/_testcapimodule.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index 3780f1a43c5ccf4..a18a8e7553ef2d1 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -6550,11 +6550,13 @@ ContainerNoGC_new(PyTypeObject *type, PyObject *args, PyObject *kwargs) { PyObject *value; char *names[] = {"value", NULL}; - if (!PyArg_ParseTupleAndKeywords(args, kwargs, "O", names, &value)) + if (!PyArg_ParseTupleAndKeywords(args, kwargs, "O", names, &value)) { return NULL; + } PyObject *self = type->tp_alloc(type, 0); - if (self == NULL) + if (self == NULL) { return NULL; + } Py_INCREF(value); ((ContainerNoGCobject *)self)->value = value; return self; @@ -6780,8 +6782,9 @@ PyInit__testcapi(void) Py_DECREF(subclass_with_finalizer_bases); PyModule_AddObject(m, "HeapCTypeSubclassWithFinalizer", HeapCTypeSubclassWithFinalizer); - if (PyType_Ready(&ContainerNoGC_type) < 0) + if (PyType_Ready(&ContainerNoGC_type) < 0) { return NULL; + } Py_INCREF(&ContainerNoGC_type); if (PyModule_AddObject(m, "ContainerNoGC", (PyObject *) &ContainerNoGC_type) < 0) From 6535ca751c8a2e107197cf6ea7aa986849915a25 Mon Sep 17 00:00:00 2001 From: Neil Schemenauer Date: Mon, 14 Oct 2019 16:00:22 -0700 Subject: [PATCH 3/4] Use unittest.mock to simplify code. --- Lib/test/test_gc.py | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/Lib/test/test_gc.py b/Lib/test/test_gc.py index 803191fd6977909..f40712d3f385489 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, @@ -996,13 +997,7 @@ def test_trash_weakref_clear(self): # since the callback might tinker with objects that have already had # tp_clear called on them (leaving them in possibly invalid states). - wr_callback_was_run = False - def callback(wr): - # We cannot allow this callback to run. It could access objects - # with invalid states due to tp_clear already being run. Or, it - # could resurrecte objects that were previously considered trash. - nonlocal wr_callback_was_run - wr_callback_was_run = True + callback = unittest.mock.Mock() class A: __slots__ = ['a', 'y', 'wz'] @@ -1023,8 +1018,9 @@ class Z: # release references and create trash del a, wr_cycle gc.collect() - # if the callback runs, we have a bug - self.assertFalse(wr_callback_was_run) + # if called, it means there is a bug in the GC. The weakref should be + # cleared before Z dies. + callback.assert_not_called() class GCCallbackTests(unittest.TestCase): From f3e1b0e897229d60dc9451cecb99b3d7a437bfa0 Mon Sep 17 00:00:00 2001 From: Neil Schemenauer Date: Mon, 14 Oct 2019 16:11:25 -0700 Subject: [PATCH 4/4] avoid unexpected automatic running of GC --- Lib/test/test_gc.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Lib/test/test_gc.py b/Lib/test/test_gc.py index f40712d3f385489..fdb8752b66817d6 100644 --- a/Lib/test/test_gc.py +++ b/Lib/test/test_gc.py @@ -1015,12 +1015,14 @@ class Z: 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):