CMultiplyNested closure crash: Python 3.13 regression

[vstinner]

Test extracted from Cython tests/run/methodmangling_T5.py:

class CMultiplyNested:
    def f1(self):
        __arg = 1
        class D:
            def g(self, __arg):
                return __arg
        return D().g(_CMultiplyNested__arg=2)

    def f2(self):
        __arg = 1
        class D:
            def g(self, __arg):
                return __arg
        return D().g

if __name__ == "__main__":
    import faulthandler; faulthandler.enable()

    inst = CMultiplyNested()
    name = '_CMultiplyNested__arg'
    try:
        inst.f1()
    except TypeError:
        print("TypeError")

    closure = inst.f2()
    closure(_CMultiplyNested__arg=2)

Output:

$ ./python bug.py
TypeError
Fatal Python error: Segmentation fault

Current thread 0x00007fa713149740 (most recent call first):
  File "/home/vstinner/python/3.13/bug.py", line 27 in <module>
Erreur de segmentation (core dumped)

Linked PRs

• gh-121863: Immortalize names in code objects to avoid crash #121903
• [3.13] gh-121863: Immortalize names in code objects to avoid crash (GH-121903) #121904

[da-woods]

Just checking - the code here crashes when run with Python and you don't need to compile with Cython to get the bug?

[vstinner]

Regression introduced by commit 9769b7a.

cc @encukou

commit 9769b7ae064a0546a98cbcbec2561dbaba20cd23 (HEAD)
Author: Petr Viktorin <[email protected]>
Date:   Mon Jun 24 20:24:19 2024 +0200

    [3.13] gh-113993: Allow interned strings to be mortal, and fix related issues (GH-120520) (GH-120945)

[vstinner]

the code here crashes when run with Python and you don't need to compile with Cython to get the bug?

The reproducer doesn't need Cython at all. Only these few lines of Python code without any import.

[vstinner]

The bug can be worked around by reverting codeobject.c changes:

diff --git a/Objects/codeobject.c b/Objects/codeobject.c
index 7b1244a8d5f..55b512b3f1a 100644
--- a/Objects/codeobject.c
+++ b/Objects/codeobject.c
@@ -137,7 +137,6 @@ static PyObject *intern_one_constant(PyObject *op);
 static int
 intern_strings(PyObject *tuple)
 {
-    PyInterpreterState *interp = _PyInterpreterState_GET();
     Py_ssize_t i;
 
     for (i = PyTuple_GET_SIZE(tuple); --i >= 0; ) {
@@ -147,7 +146,7 @@ intern_strings(PyObject *tuple)
                             "non-string found in code slot");
             return -1;
         }
-        _PyUnicode_InternMortal(interp, &_PyTuple_ITEMS(tuple)[i]);
+        PyUnicode_InternInPlace(&_PyTuple_ITEMS(tuple)[i]);
     }
     return 0;
 }
@@ -158,13 +157,12 @@ intern_strings(PyObject *tuple)
 static int
 intern_constants(PyObject *tuple, int *modified)
 {
-    PyInterpreterState *interp = _PyInterpreterState_GET();
     for (Py_ssize_t i = PyTuple_GET_SIZE(tuple); --i >= 0; ) {
         PyObject *v = PyTuple_GET_ITEM(tuple, i);
         if (PyUnicode_CheckExact(v)) {
             if (should_intern_string(v)) {
                 PyObject *w = v;
-                _PyUnicode_InternMortal(interp, &v);
+                PyUnicode_InternInPlace(&v);
                 if (w != v) {
                     PyTuple_SET_ITEM(tuple, i, v);
                     if (modified) {
@@ -460,13 +458,12 @@ init_code(PyCodeObject *co, struct _PyCodeConstructor *con)
         con->stacksize = 1;
     }
 
-    PyInterpreterState *interp = _PyInterpreterState_GET();
     co->co_filename = Py_NewRef(con->filename);
     co->co_name = Py_NewRef(con->name);
     co->co_qualname = Py_NewRef(con->qualname);
-    _PyUnicode_InternMortal(interp, &co->co_filename);
-    _PyUnicode_InternMortal(interp, &co->co_name);
-    _PyUnicode_InternMortal(interp, &co->co_qualname);
+    PyUnicode_InternInPlace(&co->co_filename);
+    PyUnicode_InternInPlace(&co->co_name);
+    PyUnicode_InternInPlace(&co->co_qualname);
     co->co_flags = con->flags;
 
     co->co_firstlineno = con->firstlineno;
@@ -492,6 +489,7 @@ init_code(PyCodeObject *co, struct _PyCodeConstructor *con)
     co->co_framesize = nlocalsplus + con->stacksize + FRAME_SPECIALS_SIZE;
     co->co_ncellvars = ncellvars;
     co->co_nfreevars = nfreevars;
+    PyInterpreterState *interp = _PyInterpreterState_GET();
 #ifdef Py_GIL_DISABLED
     PyMutex_Lock(&interp->func_state.mutex);
 #endif

[encukou]

Thanks for the bisect!
Immortalizing in intern_strings (a.k.a. the first part of the diff) fixes this for me. That is used for names and localsplusnames; it should be OK to immortalize those (and accept that the memory for these strings will not be reclaimed if the code object is GCd). See discussion in #113993.
PR: #121903

[markshannon]

intern_strings(PyObject *tuple) looks suspect to me, not because of the interning, but because it modifies a tuple.
That is risky as other code might (rightly) assume that tuples are immutable.
Have you tried having it return a new tuple?

[encukou]

I'll investigate that. I'd still like to merge the quick partial revert for 3.13.0b4 today.

[encukou]

intern_strings can indeed modify existing tuples. It's not a 3.13 regression, so I filed a new issue: #121954

The regression is fixed, to the best of my knowledge.