Crash in freethreading when acquiring/releasing the GIL in a finalizer

[da-woods]

Bug report

Bug description:

bug.cpp

#include <thread>
#include <cstdio>
#include <latch>
#include <Python.h>

PyObject* callInDestructor(PyObject *self, PyObject *args) {
    auto state = PyGILState_Ensure();
    printf("In destructor\n");
    PyGILState_Release(state);
    Py_RETURN_NONE;
}

PyObject *doStuff(PyObject *self, PyObject *cls) {
    PyObject *o;
    Py_BEGIN_ALLOW_THREADS  // I'm not really sure why this is needed...
    {
        std::latch l1(1);
        std::latch l2(1);
        std::latch l3(1);
        auto thread1 = std::jthread([&](){
            l1.wait();
            auto state = PyGILState_Ensure();
            o = PyObject_CallNoArgs(cls);
            l2.count_down();
            // printf("0\n");
            l3.wait();
            PyGILState_Release(state);
            printf("thread1 end\n");
        });
        auto thread2 = std::jthread([&](){
            l1.count_down();
            auto state = PyGILState_Ensure();
            l2.wait();
            Py_XDECREF(o);
            l3.count_down();
            PyGILState_Release(state);
            printf("thread2 end\n");
        });
    }
    Py_END_ALLOW_THREADS
    Py_RETURN_NONE;
}

static PyMethodDef methods[] = {
    {"doStuff",  doStuff, METH_O, "Demonstrate error."},
    {"callInDestructor", callInDestructor, METH_NOARGS, "destruct"},
    {NULL, NULL, 0, NULL}        /* Sentinel */
};

static struct PyModuleDef moduleDef = {
    PyModuleDef_HEAD_INIT,
    "bug",   /* name of module */
    NULL,
    -1,       /* size of per-interpreter state of the module,
                 or -1 if the module keeps state in global variables. */
    methods
};

PyMODINIT_FUNC
PyInit_bug(void)
{
    PyObject *m;

    m = PyModule_Create(&moduleDef);
    if (m == NULL)
        return NULL;

    return m;
}

setupbug.py

from setuptools import Extension, setup

setup(
    ext_modules=[
        Extension(
            name="bug",
            sources=["bug.cpp"],
            language="C++",
            extra_compile_args=['-std=c++20'],
            extra_link_args=['-lstdc++'],
        ),
    ]
)

testbug.py

import bug

class C:
    def __del__(self):
        bug.callInDestructor()

bug.doStuff(C)

Build with python setupbug.py build_ext --inplace

Run with python -Xgil=0 testbug.py

I'm a little unclear on exactly what's going wrong here, but essentially it's destroying the C object from within merge_queued_objects(&brc->local_objects_to_merge);, callInDestruction() calls PyGILState_Ensure and PyGILState_Release (which is unnecessary because it already has the GIL, but should be fine anyway), and it's around here that it crashes with a segmentation fault.

If I remove the GIL handing from callInDestruction then it doesn't crash for me.

This is a cut-down version of something I've seen in Cython cython/cython#6214 (comment) with a very similar crash but happening in a slightly different way.

CPython versions tested on:

3.13

Operating systems tested on:

Linux

Linked PRs

• gh-119585: Fix crash involving PyGILState_Release() and PyThreadState_Clear() #119753
• [3.13] gh-119585: Fix crash involving PyGILState_Release() and PyThreadState_Clear() (GH-119753) #119859
• [3.12] gh-119585: Fix crash involving PyGILState_Release() and PyThreadState_Clear() (GH-119753) #119861

[colesbury]

Thanks -- I was able to reproduce the issue easily with your repro. The problem is roughly:

• PyGILState_Release() calls PyThreadState_Clear()
• PyThreadState_Clear() calls some destructors
• The destructors call PyGILState_Ensure() and PyGILState_Release(), but the original PyGILState_Release() is started, but not quite finished so it leads to an invalid state.

PyThreadState_Clear() can call destructors in the default build too, but it happens more often in the free-threaded build.

I think the fix is relatively simple: don't decrement the gilstate_counter until after we call PyThreadState_Clear():

cpython/Python/pystate.c

Line 2801 in a150679

--tstate->gilstate_counter;

[da-woods]

@colesbury Thanks for the quick PR to fix it.

I'm closing the issue since I'm assuming it was left open as an oversight and not because there's more to do. But please correct that if I'm wrong.