diff --git a/Lib/test/test_gc.py b/Lib/test/test_gc.py index ce04042679bbc2..dbbd67b4fc88a1 100644 --- a/Lib/test/test_gc.py +++ b/Lib/test/test_gc.py @@ -227,6 +227,73 @@ def test_function(self): del d self.assertEqual(gc.collect(), 2) + def test_function_tp_clear_leaves_consistent_state(self): + # https://github.com/python/cpython/issues/91636 + code = """if 1: + + import gc + import weakref + + class LateFin: + __slots__ = ('ref',) + + def __del__(self): + + # 8. Now `latefin`'s finalizer is called. Here we + # obtain a reference to `func`, which is currently + # undergoing `tp_clear`. + global func + func = self.ref() + + class Cyclic(tuple): + __slots__ = () + + # 4. The finalizers of all garbage objects are called. In + # this case this is only us as `func` doesn't have a + # finalizer. + def __del__(self): + + # 5. Create a weakref to `func` now. If we had created + # it earlier, it would have been cleared by the + # garbage collector before calling the finalizers. + self[1].ref = weakref.ref(self[0]) + + # 6. Drop the global reference to `latefin`. The only + # remaining reference is the one we have. + global latefin + del latefin + + # 7. Now `func` is `tp_clear`-ed. This drops the last + # reference to `Cyclic`, which gets `tp_dealloc`-ed. + # This drops the last reference to `latefin`. + + latefin = LateFin() + def func(): + pass + cyc = tuple.__new__(Cyclic, (func, latefin)) + + # 1. Create a reference cycle of `cyc` and `func`. + func.__module__ = cyc + + # 2. Make the cycle unreachable, but keep the global reference + # to `latefin` so that it isn't detected as garbage. This + # way its finalizer will not be called immediately. + del func, cyc + + # 3. Invoke garbage collection, + # which will find `cyc` and `func` as garbage. + gc.collect() + + # 9. Previously, this would crash because `func_qualname` + # had been NULL-ed out by func_clear(). + print(f"{func=}") + """ + # We're mostly just checking that this doesn't crash. + rc, stdout, stderr = assert_python_ok("-c", code) + self.assertEqual(rc, 0) + self.assertRegex(stdout, rb"""\A\s*func=\s*\Z""") + self.assertFalse(stderr) + @refcount_test def test_frame(self): def f(): diff --git a/Misc/NEWS.d/next/Core and Builtins/2022-04-18-02-45-40.gh-issue-91636.6DFdy_.rst b/Misc/NEWS.d/next/Core and Builtins/2022-04-18-02-45-40.gh-issue-91636.6DFdy_.rst new file mode 100644 index 00000000000000..663339bafb79e1 --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2022-04-18-02-45-40.gh-issue-91636.6DFdy_.rst @@ -0,0 +1 @@ +Fixed a crash in a garbage-collection edge-case, in which a ``PyFunction_Type.tp_clear`` function could leave a python function object in an inconsistent state. diff --git a/Objects/funcobject.c b/Objects/funcobject.c index deacfd55dd2866..1e0cfb7efb479a 100644 --- a/Objects/funcobject.c +++ b/Objects/funcobject.c @@ -678,11 +678,8 @@ static int func_clear(PyFunctionObject *op) { op->func_version = 0; - Py_CLEAR(op->func_code); Py_CLEAR(op->func_globals); Py_CLEAR(op->func_builtins); - Py_CLEAR(op->func_name); - Py_CLEAR(op->func_qualname); Py_CLEAR(op->func_module); Py_CLEAR(op->func_defaults); Py_CLEAR(op->func_kwdefaults); @@ -690,6 +687,13 @@ func_clear(PyFunctionObject *op) Py_CLEAR(op->func_dict); Py_CLEAR(op->func_closure); Py_CLEAR(op->func_annotations); + // Don't Py_CLEAR(op->func_code), since code is always required + // to be non-NULL. Similarly, name and qualname shouldn't be NULL. + // However, name and qualname could be str subclasses, so they + // could have reference cycles. The solution is to replace them + // with a genuinely immutable string. + Py_SETREF(op->func_name, Py_NewRef(&_Py_STR(empty))); + Py_SETREF(op->func_qualname, Py_NewRef(&_Py_STR(empty))); return 0; } @@ -701,6 +705,10 @@ func_dealloc(PyFunctionObject *op) PyObject_ClearWeakRefs((PyObject *) op); } (void)func_clear(op); + // These aren't cleared by func_clear(). + Py_DECREF(op->func_code); + Py_DECREF(op->func_name); + Py_DECREF(op->func_qualname); PyObject_GC_Del(op); }