Fix crash in gil_scoped_acquire (#5828)

* Add a test reproducing the #5827 crash

Signed-off-by: Rostan Tabet <[email protected]>

* Fix #5827

Signed-off-by: Rostan Tabet <[email protected]>

* Rename PYBIND11_HAS_BARRIER and move it to common.h

Signed-off-by: Rostan Tabet <[email protected]>

* In test_thread.{cpp,py}, rename has_barrier

Signed-off-by: Rostan Tabet <[email protected]>

---------

Signed-off-by: Rostan Tabet <[email protected]>

Author: rostan-t

include/pybind11/detail/common.h

@@ -103,6 +103,10 @@
 #    define PYBIND11_DTOR_CONSTEXPR
 #endif
 
+#if defined(PYBIND11_CPP20) && defined(__has_include) && __has_include(<barrier>)
+#    define PYBIND11_HAS_STD_BARRIER 1
+#endif
+
 // Compiler version assertions
 #if defined(__INTEL_COMPILER)
 #    if __INTEL_COMPILER < 1800

include/pybind11/gil.h

@@ -120,7 +120,11 @@ class gil_scoped_acquire {
                 pybind11_fail("scoped_acquire::dec_ref(): internal error!");
             }
 #    endif
+            // Make sure that PyThreadState_Clear is not recursively called by finalizers.
+            // See issue #5827
+            ++tstate->gilstate_counter;
             PyThreadState_Clear(tstate);
+            --tstate->gilstate_counter;
             if (active) {
                 PyThreadState_DeleteCurrent();
             }

tests/test_scoped_critical_section.cpp

@@ -7,8 +7,7 @@
 #include <thread>
 #include <utility>
 
-#if defined(PYBIND11_CPP20) && defined(__has_include) && __has_include(<barrier>)
-#    define PYBIND11_HAS_BARRIER 1
+#if defined(PYBIND11_HAS_STD_BARRIER)
 #    include <barrier>
 #endif
 
@@ -39,7 +38,7 @@ class BoolWrapper {
     std::atomic_bool value_{false};
 };
 
-#if defined(PYBIND11_HAS_BARRIER)
+#if defined(PYBIND11_HAS_STD_BARRIER)
 
 // Modifying the C/C++ members of a Python object from multiple threads requires a critical section
 // to ensure thread safety and data integrity.
@@ -259,7 +258,7 @@ TEST_SUBMODULE(scoped_critical_section, m) {
     (void) BoolWrapperHandle.ptr(); // suppress unused variable warning
 
     m.attr("has_barrier") =
-#ifdef PYBIND11_HAS_BARRIER
+#ifdef PYBIND11_HAS_STD_BARRIER
         true;
 #else
         false;

tests/test_thread.cpp

@@ -15,6 +15,10 @@
 #include <chrono>
 #include <thread>
 
+#if defined(PYBIND11_HAS_STD_BARRIER)
+#    include <barrier>
+#endif
+
 namespace py = pybind11;
 
 namespace {
@@ -34,7 +38,6 @@ EmptyStruct SharedInstance;
 } // namespace
 
 TEST_SUBMODULE(thread, m) {
-
     py::class_<IntStruct>(m, "IntStruct").def(py::init([](const int i) { return IntStruct(i); }));
 
     // implicitly_convertible uses loader_life_support when an implicit
@@ -67,6 +70,39 @@ TEST_SUBMODULE(thread, m) {
     py::class_<EmptyStruct>(m, "EmptyStruct")
         .def_readonly_static("SharedInstance", &SharedInstance);
 
+#if defined(PYBIND11_HAS_STD_BARRIER)
+    // In the free-threaded build, during PyThreadState_Clear, removing the thread from the biased
+    // reference counting table may call destructors. Make sure that it doesn't crash.
+    m.def("test_pythread_state_clear_destructor", [](py::type cls) {
+        py::handle obj;
+
+        std::barrier barrier{2};
+        std::thread thread1{[&]() {
+            py::gil_scoped_acquire gil;
+            obj = cls().release();
+            barrier.arrive_and_wait();
+        }};
+        std::thread thread2{[&]() {
+            py::gil_scoped_acquire gil;
+            barrier.arrive_and_wait();
+            // ob_ref_shared becomes negative; transition to the queued state
+            obj.dec_ref();
+        }};
+
+        // jthread is not supported by Apple Clang
+        thread1.join();
+        thread2.join();
+    });
+#endif
+
+    m.attr("defined_PYBIND11_HAS_STD_BARRIER") =
+#ifdef PYBIND11_HAS_STD_BARRIER
+        true;
+#else
+        false;
+#endif
+    m.def("acquire_gil", []() { py::gil_scoped_acquire gil_acquired; });
+
     // NOTE: std::string_view also uses loader_life_support to ensure that
     // the string contents remain alive, but that's a C++ 17 feature.
 }

tests/test_thread.py

@@ -5,6 +5,7 @@
 
 import pytest
 
+import env
 from pybind11_tests import thread as m
 
 
@@ -66,3 +67,14 @@ def access_shared_instance():
         thread.start()
     for thread in threads:
         thread.join()
+
+
+@pytest.mark.skipif(sys.platform.startswith("emscripten"), reason="Requires threads")
+@pytest.mark.skipif(not m.defined_PYBIND11_HAS_STD_BARRIER, reason="no <barrier>")
+@pytest.mark.skipif(env.sys_is_gil_enabled(), reason="Deadlock with the GIL")
+def test_pythread_state_clear_destructor():
+    class Foo:
+        def __del__(self):
+            m.acquire_gil()
+
+    m.test_pythread_state_clear_destructor(Foo)