Make sure `detail::get_internals` acquires the GIL before making Python calls. (#1836)

This is only necessary if `get_internals` is called for the first time in a given module when the running thread is in a GIL-released state.

Fixes #1364

include/pybind11/detail/internals.h

@@ -171,6 +171,14 @@ PYBIND11_NOINLINE inline internals &get_internals() {
     if (internals_pp && *internals_pp)
         return **internals_pp;
 
+    // Ensure that the GIL is held since we will need to make Python calls.
+    // Cannot use py::gil_scoped_acquire here since that constructor calls get_internals.
+    struct gil_scoped_acquire {
+        gil_scoped_acquire() : state (PyGILState_Ensure()) {}
+        ~gil_scoped_acquire() { PyGILState_Release(state); }
+        const PyGILState_STATE state;
+    } gil;
+
     constexpr auto *id = PYBIND11_INTERNALS_ID;
     auto builtins = handle(PyEval_GetBuiltins());
     if (builtins.contains(id) && isinstance<capsule>(builtins[id])) {

tests/CMakeLists.txt

@@ -83,6 +83,10 @@ set(PYBIND11_CROSS_MODULE_TESTS
   test_stl_binders.py
 )
 
+set(PYBIND11_CROSS_MODULE_GIL_TESTS
+  test_gil_scoped.py
+)
+
 # Check if Eigen is available; if not, remove from PYBIND11_TEST_FILES (but
 # keep it in PYBIND11_PYTEST_FILES, so that we get the "eigen is not installed"
 # skip message).
@@ -150,6 +154,14 @@ foreach(t ${PYBIND11_CROSS_MODULE_TESTS})
   endif()
 endforeach()
 
+foreach(t ${PYBIND11_CROSS_MODULE_GIL_TESTS})
+  list(FIND PYBIND11_PYTEST_FILES ${t} i)
+  if (i GREATER -1)
+    list(APPEND test_targets cross_module_gil_utils)
+    break()
+  endif()
+endforeach()
+
 set(testdir ${CMAKE_CURRENT_SOURCE_DIR})
 foreach(target ${test_targets})
   set(test_files ${PYBIND11_TEST_FILES})

tests/cross_module_gil_utils.cpp

+/*
+    tests/cross_module_gil_utils.cpp -- tools for acquiring GIL from a different module
+
+    Copyright (c) 2019 Google LLC
+
+    All rights reserved. Use of this source code is governed by a
+    BSD-style license that can be found in the LICENSE file.
+*/
+#include <pybind11/pybind11.h>
+#include <cstdint>
+
+// This file mimics a DSO that makes pybind11 calls but does not define a
+// PYBIND11_MODULE. The purpose is to test that such a DSO can create a
+// py::gil_scoped_acquire when the running thread is in a GIL-released state.
+//
+// Note that we define a Python module here for convenience, but in general
+// this need not be the case. The typical scenario would be a DSO that implements
+// shared logic used internally by multiple pybind11 modules.
+
+namespace {
+
+namespace py = pybind11;
+void gil_acquire() { py::gil_scoped_acquire gil; }
+
+constexpr char kModuleName[] = "cross_module_gil_utils";
+
+#if PY_MAJOR_VERSION >= 3
+struct PyModuleDef moduledef = {
+    PyModuleDef_HEAD_INIT,
+    kModuleName,
+    NULL,
+    0,
+    NULL,
+    NULL,
+    NULL,
+    NULL,
+    NULL
+};
+#else
+PyMethodDef module_methods[] = {
+    {NULL, NULL, 0, NULL}
+};
+#endif
+
+}  // namespace
+
+extern "C" PYBIND11_EXPORT
+#if PY_MAJOR_VERSION >= 3
+PyObject* PyInit_cross_module_gil_utils()
+#else
+void initcross_module_gil_utils()
+#endif
+{
+
+    PyObject* m =
+#if PY_MAJOR_VERSION >= 3
+        PyModule_Create(&moduledef);
+#else
+        Py_InitModule(kModuleName, module_methods);
+#endif
+
+    if (m != NULL) {
+        static_assert(
+            sizeof(&gil_acquire) == sizeof(void*),
+            "Function pointer must have the same size as void*");
+        PyModule_AddObject(m, "gil_acquire_funcaddr",
+                           PyLong_FromVoidPtr(reinterpret_cast<void*>(&gil_acquire)));
+    }
+
+#if PY_MAJOR_VERSION >= 3
+    return m;
+#endif
+}

tests/test_gil_scoped.cpp

@@ -41,4 +41,12 @@ TEST_SUBMODULE(gil_scoped, m) {
           [](VirtClass &virt) { virt.virtual_func(); });
     m.def("test_callback_pure_virtual_func",
           [](VirtClass &virt) { virt.pure_virtual_func(); });
+    m.def("test_cross_module_gil",
+          []() {
+              auto cm = py::module::import("cross_module_gil_utils");
+              auto gil_acquire = reinterpret_cast<void (*)()>(
+                  PyLong_AsVoidPtr(cm.attr("gil_acquire_funcaddr").ptr()));
+              py::gil_scoped_release gil_release;
+              gil_acquire();
+          });
 }

tests/test_gil_scoped.py

@@ -78,3 +78,8 @@ def test_python_to_cpp_to_python_from_process():
     This test is for completion, but it was never an issue.
     """
     assert _run_in_process(_python_to_cpp_to_python) == 0
+
+
+def test_cross_module_gil():
+    """Makes sure that the GIL can be acquired by another module from a GIL-released state."""
+    m.test_cross_module_gil()  # Should not raise a SIGSEGV