diff --git a/include/pybind11/detail/common.h b/include/pybind11/detail/common.h index dc2b7cee12..142c23c772 100644 --- a/include/pybind11/detail/common.h +++ b/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); \ diff --git a/include/pybind11/detail/internals-inl.h b/include/pybind11/detail/internals-inl.h index ea1998435a..7d93347c5f 100644 --- a/include/pybind11/detail/internals-inl.h +++ b/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(); } diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index cca6491d12..b90028b0c2 100644 --- a/include/pybind11/pybind11.h +++ b/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; -/// 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 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 diff --git a/tests/test_with_catch/external_module.cpp b/tests/test_with_catch/external_module.cpp index 933ee3a6ff..c08e401b44 100644 --- a/tests/test_with_catch/external_module.cpp +++ b/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} {}; diff --git a/tests/test_with_catch/test_interpreter.cpp b/tests/test_with_catch/test_interpreter.cpp index e39f51c274..2e5f7a153f 100644 --- a/tests/test_with_catch/test_interpreter.cpp +++ b/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(); } +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(); @@ -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(definition); + auto module + = py::reinterpret_steal(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() == 42); +} + +TEST_CASE("Restart the interpreter", "[internals_init]") { // Verify pre-restart state. REQUIRE(py::module_::import("widget_module").attr("add")(1, 2).cast() == 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()); + REQUIRE(py::module_::import("external_module").attr("A")(321).attr("value").cast() + == 321); // Make sure that an interpreter with no get_internals() created until finalize still gets the // internals destroyed diff --git a/tests/test_with_catch/test_subinterpreter.cpp b/tests/test_with_catch/test_subinterpreter.cpp index 52b18aa6b7..9a4b90b642 100644 --- a/tests/test_with_catch/test_subinterpreter.cpp +++ b/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 storage; return storage