Move the unref to below the multi-interpreter check.

include/pybind11/detail/common.h

@@ -381,8 +381,7 @@
 #define PYBIND11_CONCAT(first, second) first##second
 #define PYBIND11_ENSURE_INTERNALS_READY                                                           \
     {                                                                                             \
-        pybind11::detail::get_internals_pp_manager().unref();                                     \
-        pybind11::detail::get_internals();                                                        \
+        pybind11::detail::ensure_internals();                                                     \
     }
 
 #if !defined(GRAALVM_PYTHON)
@@ -471,7 +470,6 @@ PyModuleDef_Init should be treated like any other PyObject (so not shared across
         PYBIND11_CHECK_PYTHON_VERSION                                                             \
         PYBIND11_PRECOMPILED_CONFIG_GUARD                                                         \
         try {                                                                                     \
-            pybind11::detail::ensure_internals();                                                 \
             static ::pybind11::detail::slots_array mod_def_slots                                  \
                 = ::pybind11::detail::init_slots(                                                 \
                     &PYBIND11_CONCAT(pybind11_exec_, name), ##__VA_ARGS__);                       \
@@ -494,7 +492,7 @@ PyModuleDef_Init should be treated like any other PyObject (so not shared across
     static void PYBIND11_CONCAT(pybind11_init_, name)(::pybind11::module_ &);                     \
     int PYBIND11_CONCAT(pybind11_exec_, name)(PyObject * pm) {                                    \
         try {                                                                                     \
-            pybind11::detail::ensure_internals();                                                 \
+            PYBIND11_ENSURE_INTERNALS_READY                                                       \
             auto m = pybind11::reinterpret_borrow<::pybind11::module_>(pm);                       \
             if (!pybind11::detail::get_cached_module(m.attr("__spec__").attr("name"))) {          \
                 PYBIND11_CONCAT(pybind11_init_, name)(m);                                         \

include/pybind11/detail/internals-inl.h

@@ -207,12 +207,17 @@ PYBIND11_INLINE PyObject *get_local_internals_capsule() {
 }
 
 PYBIND11_INLINE void ensure_internals() {
-    pybind11::detail::get_internals_pp_manager().unref();
 #ifdef PYBIND11_HAS_SUBINTERPRETER_SUPPORT
     if (PyInterpreterState_Get() != PyInterpreterState_Main()) {
         has_seen_non_main_interpreter() = true;
     }
 #endif
+    /* In an embedded app the main interpreter could be finalized and re-initialized and a pybind11
+     * extension module would hold a pointer to a deleted internals.  The only way to prevent
+     * access to that is to re-fetch everything from the state dict here.  So we first null it out
+     * of our global copy and then fetch it (creating it if it does not already exist). */
+    pybind11::detail::get_internals_pp_manager().unref();
+    pybind11::detail::get_local_internals_pp_manager().unref();
     pybind11::detail::get_internals();
 }
 

include/pybind11/pybind11.h

@@ -689,7 +689,8 @@ PyObject *cached_create_module(PyObject *spec, PyModuleDef *);
 /// the sentinel (0) end slot.
 using slots_array = std::array<PyModuleDef_Slot, 5>;
 
-/// Initialize an array of slots based on the supplied exec slot and options.
+/** Initialize an array of slots based on the supplied exec slot and options. This code may not
+ * assume the GIL and may not use pybind11 internals because this runs early during module init. */
 template <typename... Options>
 inline slots_array init_slots(int (*exec_fn)(PyObject *), Options &&...options) noexcept {
     /* NOTE: slots_array MUST be large enough to hold all possible options.  If you add an option

tests/test_with_catch/external_module.cpp

@@ -6,26 +6,18 @@ namespace py = pybind11;
  * modules aren't preserved over a finalize/initialize.
  */
 
-namespace {
-// Compare unsafe_reset_internals_for_single_interpreter in
-// test_subinterpreter.cpp.
-void unsafe_reset_local_internals() {
-    // NOTE: This code is NOT SAFE unless the caller guarantees no other threads are alive
-    // NOTE: This code is tied to the precise implementation of the internals holder
-
-    py::detail::get_local_internals_pp_manager().unref();
-    py::detail::get_local_internals();
-}
-} // namespace
-
 PYBIND11_MODULE(external_module,
                 m,
                 py::mod_gil_not_used(),
                 py::multiple_interpreters::per_interpreter_gil()) {
-    // At least one test ("Single Subinterpreter") wants to reset
-    // internals. We have separate local internals because we are a
-    // separate DSO, so ours need to be reset too!
-    unsafe_reset_local_internals();
+    // A separate DSO must refresh its own local cache after an interpreter restart. Check before
+    // registering bindings, which would dereference a stale holder if initialization missed it.
+    auto *local_internals_pp = py::detail::get_local_internals_pp_manager().get_pp();
+    auto *local_internals_capsule = py::detail::get_local_internals_capsule();
+    if (!local_internals_capsule
+        || PyCapsule_GetPointer(local_internals_capsule, nullptr) != local_internals_pp) {
+        throw py::import_error("external_module has a stale local internals cache");
+    }
     class A {
     public:
         explicit A(int value) : v{value} {};

tests/test_with_catch/test_interpreter.cpp

@@ -104,6 +104,16 @@ PYBIND11_EMBEDDED_MODULE(throw_error_already_set, , py::multiple_interpreters::n
     d["missing"].cast<py::object>();
 }
 
+static int internals_init_module_executions = 0;
+
+PYBIND11_EMBEDDED_MODULE(internals_init_module,
+                         m,
+                         py::mod_gil_not_used(),
+                         py::multiple_interpreters::per_interpreter_gil()) {
+    ++internals_init_module_executions;
+    m.def("answer", []() { return 42; });
+}
+
 TEST_CASE("PYTHONPATH is used to update sys.path") {
     // The setup for this TEST_CASE is in catch.cpp!
     auto sys_path = py::str(py::module_::import("sys").attr("path")).cast<std::string>();
@@ -293,7 +303,38 @@ TEST_CASE("Add program dir to path using PyConfig") {
     py::initialize_interpreter();
 }
 
-TEST_CASE("Restart the interpreter") {
+TEST_CASE("Internals are initialized when a module executes", "[internals_init]") {
+    // Use a fresh interpreter so that another module cannot have populated its internals.
+    py::finalize_interpreter();
+    py::initialize_interpreter();
+    internals_init_module_executions = 0;
+    REQUIRE_FALSE(has_state_dict_internals_obj());
+    REQUIRE(py::detail::get_local_internals_capsule() == nullptr);
+
+    auto *definition = PyInit_internals_init_module();
+    REQUIRE(definition != nullptr);
+    REQUIRE_FALSE(has_state_dict_internals_obj());
+    REQUIRE(py::detail::get_local_internals_capsule() == nullptr);
+
+    auto spec = py::module_::import("importlib.util")
+                    .attr("spec_from_loader")("internals_init_module", py::none());
+    auto *module_def = reinterpret_cast<PyModuleDef *>(definition);
+    auto module
+        = py::reinterpret_steal<py::module_>(PyModule_FromDefAndSpec(module_def, spec.ptr()));
+    REQUIRE(module);
+    module.attr("__spec__") = spec;
+    REQUIRE_FALSE(has_state_dict_internals_obj());
+    REQUIRE(py::detail::get_local_internals_capsule() == nullptr);
+    REQUIRE(internals_init_module_executions == 0);
+
+    REQUIRE(PyModule_ExecDef(module.ptr(), module_def) == 0);
+    REQUIRE(has_state_dict_internals_obj());
+    REQUIRE(py::detail::get_local_internals_capsule() != nullptr);
+    REQUIRE(internals_init_module_executions == 1);
+    REQUIRE(module.attr("answer")().cast<int>() == 42);
+}
+
+TEST_CASE("Restart the interpreter", "[internals_init]") {
     // Verify pre-restart state.
     REQUIRE(py::module_::import("widget_module").attr("add")(1, 2).cast<int>() == 3);
     REQUIRE(has_state_dict_internals_obj());
@@ -319,6 +360,8 @@ TEST_CASE("Restart the interpreter") {
     REQUIRE(get_details_as_uintptr() != 0);
     REQUIRE(get_details_as_uintptr()
             == py::module_::import("external_module").attr("internals_at")().cast<uintptr_t>());
+    REQUIRE(py::module_::import("external_module").attr("A")(321).attr("value").cast<int>()
+            == 321);
 
     // Make sure that an interpreter with no get_internals() created until finalize still gets the
     // internals destroyed

tests/test_with_catch/test_subinterpreter.cpp

@@ -91,6 +91,83 @@ TEST_CASE("Internals cache retries after a failed lookup") {
     }
 }
 
+TEST_CASE("First subinterpreter initialization preserves the singleton cache",
+          "[internals_init]") {
+    // No worker threads or other subinterpreters may be alive while resetting the cache mode.
+    unsafe_reset_internals_for_single_interpreter();
+    auto &manager = py::detail::get_internals_pp_manager();
+    auto *main_pp = manager.get_pp();
+
+    struct reset_guard {
+        PyThreadState *main_tstate;
+        PyThreadState *sub_tstate = nullptr;
+        PyObject *state_dict = nullptr;
+        PyObject *capsule = nullptr;
+
+        explicit reset_guard(PyThreadState *main) : main_tstate(main) {}
+
+        void end_subinterpreter() {
+            if (sub_tstate == nullptr) {
+                return;
+            }
+            // Also handle assertions that fail before ensure_internals() enables TLS caching.
+            py::detail::has_seen_non_main_interpreter() = true;
+            py::detail::get_internals_pp_manager().get_pp();
+            py::detail::get_local_internals_pp_manager().get_pp();
+            Py_EndInterpreter(sub_tstate);
+            sub_tstate = nullptr;
+            // As in py::subinterpreter's destructor, free holders after interpreter shutdown.
+            py::detail::get_internals_pp_manager().destroy();
+            py::detail::get_local_internals_pp_manager().destroy();
+            PyThreadState_Swap(main_tstate);
+        }
+
+        ~reset_guard() {
+            end_subinterpreter();
+            if (capsule != nullptr) {
+                PyDict_SetItemString(state_dict, PYBIND11_INTERNALS_ID, capsule);
+                Py_DECREF(capsule);
+            }
+            unsafe_reset_internals_for_single_interpreter();
+        }
+    } reset{PyThreadState_Get()};
+
+    REQUIRE_FALSE(py::detail::has_seen_non_main_interpreter());
+    PyInterpreterConfig config{};
+    config.allow_threads = 1;
+    config.check_multi_interp_extensions = 1;
+    config.gil = PyInterpreterConfig_OWN_GIL;
+    // py::subinterpreter::create() would set the flag itself, masking the initialization order.
+    auto status = Py_NewInterpreterFromConfig(&reset.sub_tstate, &config);
+    REQUIRE_FALSE(PyStatus_Exception(status));
+    REQUIRE_FALSE(py::detail::has_seen_non_main_interpreter());
+
+    // This must be the first pybind11 operation in the new interpreter.
+    py::detail::ensure_internals();
+    REQUIRE(py::detail::has_seen_non_main_interpreter());
+    REQUIRE(manager.get_pp() != main_pp);
+    reset.end_subinterpreter();
+
+    // All subinterpreters are gone: clear TLS before selecting the singleton cache again.
+    manager.unref();
+    py::detail::get_local_internals_pp_manager().unref();
+    py::detail::has_seen_non_main_interpreter() = false;
+
+    reset.state_dict = PyInterpreterState_GetDict(PyInterpreterState_Get());
+    REQUIRE(reset.state_dict != nullptr);
+    reset.capsule = Py_XNewRef(PyDict_GetItemString(reset.state_dict, PYBIND11_INTERNALS_ID));
+    REQUIRE(reset.capsule != nullptr);
+    REQUIRE(PyDict_DelItemString(reset.state_dict, PYBIND11_INTERNALS_ID) == 0);
+
+    // Hide the capsule: otherwise an incorrectly cleared singleton could just fetch it again.
+    auto *cached_pp = manager.get_pp();
+    bool preserved_singleton = cached_pp == main_pp;
+    if (!preserved_singleton) {
+        manager.destroy(); // Free the empty holder created by the failed cache lookup.
+    }
+    REQUIRE(preserved_singleton);
+}
+
 py::object &get_dict_type_object() {
     PYBIND11_CONSTINIT static py::gil_safe_call_once_and_store<py::object> storage;
     return storage