[Python-checkins] gh-91636: Don't clear required fields of function objects (GH-91651)

[sweeneyde]

https://github.com/python/cpython/commit/f2b4e458b3327130e46edb4efe8e1847de09efc5
commit: f2b4e458b3327130e46edb4efe8e1847de09efc5
branch: main
author: Dennis Sweeney <36520290+sweeneyde at users.noreply.github.com>
committer: sweeneyde <36520290+sweeneyde at users.noreply.github.com>
date: 2022-04-21T02:06:35-04:00
summary:

gh-91636: Don't clear required fields of function objects (GH-91651)

files:
A Misc/NEWS.d/next/Core and Builtins/2022-04-18-02-45-40.gh-issue-91636.6DFdy_.rst
M Lib/test/test_gc.py
M Objects/funcobject.c

diff --git a/Lib/test/test_gc.py b/Lib/test/test_gc.py
index ce04042679bbc..dbbd67b4fc88a 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=<function  at \S+>\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 0000000000000..663339bafb79e
--- /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 deacfd55dd286..1e0cfb7efb479 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);
 }