qi.pyguard: Fixes segfault at Python interpreter finalization due to GIL bad manipulation. #47167

A segfault occurs during the finalization of the Python interpreter when an
object has a destructor that uses `GILRelease`. The constructor of `GILRelease`
effectively releases the GIL but the destructor disables its acquisition
because the interpreter is finalizing. When the call returns to the
interpreter, the GIL stays released and this leads to crashes or errors when
the interpreter tries to make Python calls.

This patch rewrites the `GILRelease` type so that during finalization, the
object does nothing (therefore the GIL is not released).

Author: Vincent Palancher

cmake/add_tests.cmake

@@ -49,18 +49,32 @@ target_link_libraries(test_qipython
           qi.interface
           cxx11
           gmock)
-# Unfortunately, in some of our toolchains, gtest/gmock headers are found in the qi-framework
-# package, which comes first in the include paths order given to the compiler. This causes the
-# compiler to use those headers instead of the ones we got from a `FetchContent` of the googletest
-# repository.
-target_include_directories(test_qipython
-  BEFORE # Force this path to come first in the list of include paths.
-  PRIVATE $<TARGET_PROPERTY:gmock,INTERFACE_INCLUDE_DIRECTORIES>)
-enable_warnings(test_qipython)
-set_build_rpath_to_qipython_dependencies(test_qipython)
+
+add_executable(test_qipython_local_interpreter)
+target_sources(test_qipython_local_interpreter
+  PRIVATE tests/test_qipython_local_interpreter.cpp)
+target_link_libraries(test_qipython_local_interpreter
+  PRIVATE Python::Python
+          pybind11
+          qi_python_objects
+          qi.interface
+          cxx11
+          gmock)
 
 set(_sdk_prefix "${CMAKE_BINARY_DIR}/sdk")
-gtest_discover_tests(test_qipython EXTRA_ARGS --qi-sdk-prefix ${_sdk_prefix})
+foreach(test_target IN ITEMS test_qipython
+                             test_qipython_local_interpreter)
+  # Unfortunately, in some of our toolchains, gtest/gmock headers are found in the qi-framework
+  # package, which comes first in the include paths order given to the compiler. This causes the
+  # compiler to use those headers instead of the ones we got from a `FetchContent` of the googletest
+  # repository.
+  target_include_directories(${test_target}
+    BEFORE # Force this path to come first in the list of include paths.
+    PRIVATE $<TARGET_PROPERTY:gmock,INTERFACE_INCLUDE_DIRECTORIES>)
+  enable_warnings(${test_target})
+  set_build_rpath_to_qipython_dependencies(${test_target})
+  gtest_discover_tests(${test_target} EXTRA_ARGS --qi-sdk-prefix ${_sdk_prefix})
+endforeach()
 
 if(NOT Python_Interpreter_FOUND)
   message(WARNING "tests: a compatible Python Interpreter was NOT found, Python tests are DISABLED.")

qipython/pyguard.hpp

@@ -143,59 +143,57 @@ void pybind11GuardDisarm(G& guard)
 /// RAII utility type that guarantees that the GIL is locked for the scope of
 /// the lifetime of the object.
 ///
+/// Objects of this type (or objects composed of them) must not be kept alive
+/// after the hand is given back to the interpreter.
+///
 /// This type is re-entrant.
 ///
-/// postcondition: `GILAcquire acq;` establishes `currentThreadHoldsGil()`
+/// postcondition: `GILAcquire acq;` establishes
+/// `(interpreterIsFinalizing() && *interpreterIsFinalizing()) || currentThreadHoldsGil()`
 struct GILAcquire
 {
   inline GILAcquire()
   {
+    const auto optIsFinalizing = interpreterIsFinalizing();
+    const auto definitelyFinalizing = optIsFinalizing && *optIsFinalizing;
     // `gil_scoped_acquire` is re-entrant by itself, so we don't need to check
     // whether or not the GIL is already held by the current thread.
-    QI_ASSERT(currentThreadHoldsGil());
+    if (!definitelyFinalizing)
+      _acq.emplace();
+    QI_ASSERT(definitelyFinalizing || currentThreadHoldsGil());
   }
 
   GILAcquire(const GILAcquire&) = delete;
   GILAcquire& operator=(const GILAcquire&) = delete;
 
-  inline ~GILAcquire()
-  {
-    const auto optIsFinalizing = interpreterIsFinalizing();
-    const auto definitelyFinalizing = optIsFinalizing && *optIsFinalizing;
-    if (definitelyFinalizing)
-      detail::pybind11GuardDisarm(_acq);
-  }
-
 private:
-  pybind11::gil_scoped_acquire _acq;
+  boost::optional<pybind11::gil_scoped_acquire> _acq;
 };
 
 /// RAII utility type that guarantees that the GIL is unlocked for the scope of
 /// the lifetime of the object.
 ///
+/// Objects of this type (or objects composed of them) must not be kept alive
+/// after the hand is given back to the interpreter.
+///
 /// This type is re-entrant.
 ///
-/// postcondition: `GILRelease rel;` establishes `!currentThreadHoldsGil()`
+/// postcondition: `GILRelease rel;` establishes
+/// `(interpreterIsFinalizing() && *interpreterIsFinalizing()) || !currentThreadHoldsGil()`
 struct GILRelease
 {
   inline GILRelease()
   {
-    if (currentThreadHoldsGil())
+    const auto optIsFinalizing = interpreterIsFinalizing();
+    const auto definitelyFinalizing = optIsFinalizing && *optIsFinalizing;
+    if (!definitelyFinalizing && currentThreadHoldsGil())
       _release.emplace();
-    QI_ASSERT(!currentThreadHoldsGil());
+    QI_ASSERT(definitelyFinalizing || !currentThreadHoldsGil());
   }
 
   GILRelease(const GILRelease&) = delete;
   GILRelease& operator=(const GILRelease&) = delete;
 
-  inline ~GILRelease()
-  {
-    const auto optIsFinalizing = interpreterIsFinalizing();
-    const auto definitelyFinalizing = optIsFinalizing && *optIsFinalizing;
-    if (_release && definitelyFinalizing)
-      detail::pybind11GuardDisarm(*_release);
-  }
-
 private:
   boost::optional<pybind11::gil_scoped_release> _release;
 };

tests/test_qipython_local_interpreter.cpp

@@ -0,0 +1,41 @@
+#include <gtest/gtest.h>
+#include <qi/application.hpp>
+#include <qi/property.hpp>
+#include <qi/session.hpp>
+#include <qipython/common.hpp>
+#include <qipython/pyguard.hpp>
+#include <qipython/pyobject.hpp>
+#include <qipython/pysession.hpp>
+#include <qipython/pyexport.hpp>
+#include <pybind11/pybind11.h>
+#include <pybind11/embed.h>
+
+PYBIND11_EMBEDDED_MODULE(test_local_interpreter, m) {
+  struct ObjectDtorOutsideGIL
+  {
+    ~ObjectDtorOutsideGIL()
+    {
+      qi::py::GILRelease _rel;
+      // nothing.
+    }
+
+  };
+  pybind11::class_<ObjectDtorOutsideGIL>(m, "ObjectDtorOutsideGIL")
+    .def(pybind11::init([]{
+      return std::make_unique<ObjectDtorOutsideGIL>();
+    }));
+}
+
+TEST(InterpreterFinalize, DoesNotSegfaultGarbageObjectDtorOutsideGIL)
+{
+  pybind11::scoped_interpreter interp;
+  pybind11::globals()["qitli"] = pybind11::module::import("test_local_interpreter");
+  pybind11::exec("obj = qitli.ObjectDtorOutsideGIL()");
+}
+
+int main(int argc, char **argv)
+{
+  ::testing::InitGoogleTest(&argc, argv);
+  qi::Application app(argc, argv);
+  return RUN_ALL_TESTS();
+}