From 97108c163ee2d6c197dd805318fff6e06823952a Mon Sep 17 00:00:00 2001 From: sweeneyde Date: Sun, 17 Apr 2022 22:22:29 -0400 Subject: [PATCH 1/5] Don't clear required fields of function objects --- Lib/test/test_gc.py | 66 ++++++++++++++++++++++++++++++++++++++++++++ Objects/funcobject.c | 15 ++++++++-- 2 files changed, 78 insertions(+), 3 deletions(-) diff --git a/Lib/test/test_gc.py b/Lib/test/test_gc.py index ce04042679bbc2..2de8b744568e81 100644 --- a/Lib/test/test_gc.py +++ b/Lib/test/test_gc.py @@ -226,6 +226,72 @@ def test_function(self): gc.collect() 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=}") + """ + 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): diff --git a/Objects/funcobject.c b/Objects/funcobject.c index deacfd55dd2866..747cce5c797888 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,14 @@ func_clear(PyFunctionObject *op) Py_CLEAR(op->func_dict); Py_CLEAR(op->func_closure); Py_CLEAR(op->func_annotations); + /* Below are required attributes, so to keep everything in + * a consistent state, don't clear them. They're immutable, + * so they can't participate in cycles anyway. + * + * don't Py_CLEAR(op->func_code); + * don't Py_CLEAR(op->func_name); + * don't Py_CLEAR(op->func_qualname); + */ return 0; } @@ -701,6 +706,10 @@ func_dealloc(PyFunctionObject *op) PyObject_ClearWeakRefs((PyObject *) op); } (void)func_clear(op); + // These aren't cleared by func_clear(). + Py_XDECREF(op->func_code); + Py_XDECREF(op->func_name); + Py_XDECREF(op->func_qualname); PyObject_GC_Del(op); } From e1d8031d7fb5640adf5958e1f95bb2ed6c14a5f8 Mon Sep 17 00:00:00 2001 From: "blurb-it[bot]" <43283697+blurb-it[bot]@users.noreply.github.com> Date: Mon, 18 Apr 2022 02:45:42 +0000 Subject: [PATCH 2/5] =?UTF-8?q?=F0=9F=93=9C=F0=9F=A4=96=20Added=20by=20blu?= =?UTF-8?q?rb=5Fit.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../2022-04-18-02-45-40.gh-issue-91636.6DFdy_.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2022-04-18-02-45-40.gh-issue-91636.6DFdy_.rst 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. From 7e6df5f1a2949e94a3e1bae1571bb93b1884bee4 Mon Sep 17 00:00:00 2001 From: sweeneyde Date: Sun, 17 Apr 2022 23:10:27 -0400 Subject: [PATCH 3/5] whitespace --- Lib/test/test_gc.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/test/test_gc.py b/Lib/test/test_gc.py index 2de8b744568e81..5bf6b4cbc83955 100644 --- a/Lib/test/test_gc.py +++ b/Lib/test/test_gc.py @@ -226,7 +226,7 @@ def test_function(self): gc.collect() 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: From 6e06a20c0df96540af0c6deb6ffc1b82e4a67238 Mon Sep 17 00:00:00 2001 From: sweeneyde Date: Mon, 18 Apr 2022 23:26:34 -0400 Subject: [PATCH 4/5] replace strings with the empty string --- Lib/test/test_gc.py | 3 ++- Objects/funcobject.c | 15 +++++++-------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/Lib/test/test_gc.py b/Lib/test/test_gc.py index 5bf6b4cbc83955..dbbd67b4fc88a1 100644 --- a/Lib/test/test_gc.py +++ b/Lib/test/test_gc.py @@ -288,9 +288,10 @@ def func(): # 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.assertRegex(stdout, rb"""\A\s*func=\s*\Z""") self.assertFalse(stderr) @refcount_test diff --git a/Objects/funcobject.c b/Objects/funcobject.c index 747cce5c797888..d9b6f4af453eb7 100644 --- a/Objects/funcobject.c +++ b/Objects/funcobject.c @@ -687,14 +687,13 @@ func_clear(PyFunctionObject *op) Py_CLEAR(op->func_dict); Py_CLEAR(op->func_closure); Py_CLEAR(op->func_annotations); - /* Below are required attributes, so to keep everything in - * a consistent state, don't clear them. They're immutable, - * so they can't participate in cycles anyway. - * - * don't Py_CLEAR(op->func_code); - * don't Py_CLEAR(op->func_name); - * don't Py_CLEAR(op->func_qualname); - */ + // 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; } From 23b08e8d8d1c9b5327d657c46c7ff79764fe1da2 Mon Sep 17 00:00:00 2001 From: sweeneyde Date: Mon, 18 Apr 2022 23:42:57 -0400 Subject: [PATCH 5/5] XDECREF --> DECREF --- Objects/funcobject.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Objects/funcobject.c b/Objects/funcobject.c index d9b6f4af453eb7..1e0cfb7efb479a 100644 --- a/Objects/funcobject.c +++ b/Objects/funcobject.c @@ -706,9 +706,9 @@ func_dealloc(PyFunctionObject *op) } (void)func_clear(op); // These aren't cleared by func_clear(). - Py_XDECREF(op->func_code); - Py_XDECREF(op->func_name); - Py_XDECREF(op->func_qualname); + Py_DECREF(op->func_code); + Py_DECREF(op->func_name); + Py_DECREF(op->func_qualname); PyObject_GC_Del(op); }