diff --git a/include/pybind11/detail/class.h b/include/pybind11/detail/class.h index b2ee3cc698..7fe692856b 100644 --- a/include/pybind11/detail/class.h +++ b/include/pybind11/detail/class.h @@ -228,6 +228,11 @@ extern "C" inline void pybind11_meta_dealloc(PyObject *obj) { internals.registered_types_cpp.erase(tindex); #if PYBIND11_INTERNALS_VERSION >= 12 internals.registered_types_cpp_fast.erase(tinfo->cpptype); + for (const std::type_info *alias : tinfo->alias_chain) { + auto num_erased = internals.registered_types_cpp_fast.erase(alias); + (void) num_erased; + assert(num_erased > 0); + } #endif } internals.registered_types_py.erase(tinfo->type); diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index ead330d286..4f8de120fe 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -357,6 +357,20 @@ struct type_info { void *get_buffer_data = nullptr; void *(*module_local_load)(PyObject *, const type_info *) = nullptr; holder_enum_t holder_enum_v = holder_enum_t::undefined; + +#if PYBIND11_INTERNALS_VERSION >= 12 + // When a type appears in multiple DSOs, + // internals::registered_types_cpp_fast will have multiple distinct + // keys (the std::type_info from each DSO) mapped to the same + // detail::type_info*. We need to keep track of these aliases so that we clean + // them up when our type is deallocated. A linked list is appropriate + // because it is expected to be 1) usually empty and 2) + // when it's not empty, usually very small. See also `struct + // nb_alias_chain` added in + // https://github.com/wjakob/nanobind/commit/b515b1f7f2f4ecc0357818e6201c94a9f4cbfdc2 + std::forward_list alias_chain; +#endif + /* A simple type never occurs as a (direct or indirect) parent * of a class that makes use of multiple inheritance. * A type can be simple even if it has non-simple ancestors as long as it has no descendants. diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index a07ef1a897..baf7c64022 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -246,6 +246,11 @@ inline detail::type_info *get_global_type_info_lock_held(const std::type_info &t auto it = types.find(std::type_index(tp)); if (it != types.end()) { #if PYBIND11_INTERNALS_VERSION >= 12 + // We found the type in the slow map but not the fast one, so + // some other DSO added it (otherwise it would be in the fast + // map under &tp) and therefore we must be an alias. Record + // that. + it->second->alias_chain.push_front(&tp); fast_types.emplace(&tp, it->second); #endif type_info = it->second; diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 8ac236d404..47ba4aa863 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -117,6 +117,7 @@ set(PYBIND11_TEST_FILES test_callbacks test_chrono test_class + test_class_cross_module_use_after_one_module_dealloc test_class_release_gil_before_calling_cpp_dtor test_class_sh_basic test_class_sh_disowning @@ -239,8 +240,9 @@ list(SORT PYBIND11_PYTEST_FILES) # Contains the set of test files that require pybind11_cross_module_tests to be # built; if none of these are built (i.e. because TEST_OVERRIDE is used and # doesn't include them) the second module doesn't get built. -tests_extra_targets("test_exceptions.py;test_local_bindings.py;test_stl.py;test_stl_binders.py" - "pybind11_cross_module_tests") +tests_extra_targets( + "test_class_cross_module_use_after_one_module_dealloc.py;test_exceptions.py;test_local_bindings.py;test_stl.py;test_stl_binders.py" + "pybind11_cross_module_tests") # And add additional targets for other tests. tests_extra_targets("test_exceptions.py" "cross_module_interleaved_error_already_set") diff --git a/tests/pybind11_cross_module_tests.cpp b/tests/pybind11_cross_module_tests.cpp index 76f40bfa97..837568352e 100644 --- a/tests/pybind11_cross_module_tests.cpp +++ b/tests/pybind11_cross_module_tests.cpp @@ -16,6 +16,15 @@ #include #include +class CrossDSOClass { +public: + CrossDSOClass() = default; + virtual ~CrossDSOClass(); + CrossDSOClass(const CrossDSOClass &) = default; +}; + +CrossDSOClass::~CrossDSOClass() = default; + PYBIND11_MODULE(pybind11_cross_module_tests, m, py::mod_gil_not_used()) { m.doc() = "pybind11 cross-module test module"; @@ -146,4 +155,7 @@ PYBIND11_MODULE(pybind11_cross_module_tests, m, py::mod_gil_not_used()) { // which appears when this header is missing. m.def("missing_header_arg", [](const std::vector &) {}); m.def("missing_header_return", []() { return std::vector(); }); + + // test_class_cross_module_use_after_one_module_dealloc + m.def("consume_cross_dso_class", [](const CrossDSOClass &) {}); } diff --git a/tests/test_class_cross_module_use_after_one_module_dealloc.cpp b/tests/test_class_cross_module_use_after_one_module_dealloc.cpp new file mode 100644 index 0000000000..6f6e62debc --- /dev/null +++ b/tests/test_class_cross_module_use_after_one_module_dealloc.cpp @@ -0,0 +1,23 @@ +#include "pybind11_tests.h" + +#include + +class CrossDSOClass { +public: + CrossDSOClass() = default; + virtual ~CrossDSOClass(); + CrossDSOClass(const CrossDSOClass &) = default; +}; + +CrossDSOClass::~CrossDSOClass() = default; + +struct UnrelatedClass {}; + +TEST_SUBMODULE(class_cross_module_use_after_one_module_dealloc, m) { + m.def("register_and_instantiate_cross_dso_class", [](const py::module_ &m) { + py::class_(m, "CrossDSOClass").def(py::init<>()); + return CrossDSOClass(); + }); + m.def("register_unrelated_class", + [](const py::module_ &m) { py::class_(m, "UnrelatedClass"); }); +} diff --git a/tests/test_class_cross_module_use_after_one_module_dealloc.py b/tests/test_class_cross_module_use_after_one_module_dealloc.py new file mode 100644 index 0000000000..ac8a47a8fc --- /dev/null +++ b/tests/test_class_cross_module_use_after_one_module_dealloc.py @@ -0,0 +1,67 @@ +from __future__ import annotations + +import gc +import sys +import sysconfig +import types +import weakref + +import pytest + +import env +from pybind11_tests import class_cross_module_use_after_one_module_dealloc as m + +is_python_3_13_free_threaded = ( + env.CPYTHON + and sysconfig.get_config_var("Py_GIL_DISABLED") + and (3, 13) <= sys.version_info < (3, 14) +) + + +def delattr_and_ensure_destroyed(*specs): + wrs = [] + for mod, name in specs: + wrs.append(weakref.ref(getattr(mod, name))) + delattr(mod, name) + + for _ in range(5): + gc.collect() + if all(wr() is None for wr in wrs): + break + else: + pytest.fail( + f"Could not delete bindings such as {next(wr for wr in wrs if wr() is not None)!r}" + ) + + +@pytest.mark.skipif("env.PYPY or env.GRAALPY or is_python_3_13_free_threaded") +def test_cross_module_use_after_one_module_dealloc(): + # This is a regression test for a bug that occurred during development of + # internals::registered_types_cpp_fast (see #5842). registered_types_cpp_fast maps + # &typeid(T) to a raw non-owning pointer to a Python type object. If two DSOs both + # look up the same global type, they will create two separate entries in + # registered_types_cpp_fast, which will look like: + # +=========================================+ + # |&typeid(T) from DSO 1|type object pointer| + # |&typeid(T) from DSO 2|type object pointer| + # +=========================================+ + # + # Then, if the type object is destroyed and we don't take extra steps to clean up + # the table thoroughly, the first row of the table will be cleaned up but the second + # one will contain a dangling pointer to the old type object. Further lookups from + # DSO 2 will then return that dangling pointer, which will cause use-after-frees. + + import pybind11_cross_module_tests as cm + + module_scope = types.ModuleType("module_scope") + instance = m.register_and_instantiate_cross_dso_class(module_scope) + cm.consume_cross_dso_class(instance) + + del instance + delattr_and_ensure_destroyed((module_scope, "CrossDSOClass")) + + # Make sure that CrossDSOClass gets allocated at a different address. + m.register_unrelated_class(module_scope) + + instance = m.register_and_instantiate_cross_dso_class(module_scope) + cm.consume_cross_dso_class(instance)