From e36686cf1c391080c810aeef5f3d28af26b8dd66 Mon Sep 17 00:00:00 2001 From: Scott Wolchok Date: Mon, 13 Oct 2025 17:26:19 -0700 Subject: [PATCH 1/6] Fix dangling pointer in internals::registered_types_cpp_fast from #5842 @oremanj pointed out in a comment on #5842 that I missed part of the nanobind PR I was porting in such a way that we could have dangling pointers in internals::registered_types_cpp_fast. This PR adds a test that reproed the bug and then fixes the test. --- include/pybind11/detail/class.h | 7 +++ include/pybind11/detail/internals.h | 21 +++++++ include/pybind11/detail/type_caster_base.h | 8 +++ tests/CMakeLists.txt | 6 +- tests/pybind11_cross_module_tests.cpp | 10 ++++ ...ss_module_use_after_one_module_dealloc.cpp | 21 +++++++ ...oss_module_use_after_one_module_dealloc.py | 59 +++++++++++++++++++ 7 files changed, 130 insertions(+), 2 deletions(-) create mode 100644 tests/test_class_cross_module_use_after_one_module_dealloc.cpp create mode 100644 tests/test_class_cross_module_use_after_one_module_dealloc.py diff --git a/include/pybind11/detail/class.h b/include/pybind11/detail/class.h index b2ee3cc698..6ccbc129e5 100644 --- a/include/pybind11/detail/class.h +++ b/include/pybind11/detail/class.h @@ -228,6 +228,13 @@ 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); + const alias_chain_entry *chain = tinfo->alias_chain.get(); + while (chain) { + auto num_erased = internals.registered_types_cpp_fast.erase(chain->value); + (void) num_erased; + assert(num_erased > 0); + chain = chain->next.get(); + } #endif } internals.registered_types_py.erase(tinfo->type); diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index ead330d286..51dbc017f6 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -335,6 +335,22 @@ enum class holder_enum_t : uint8_t { custom_holder, }; +// When a type appears in multiple DSOs, +// internals::registered_types_cpp_fast will have multiple distinct +// keys (the type_info from each DSO) mapped to the same +// 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 this structure 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 +#if PYBIND11_INTERNALS_VERSION >= 12 +struct alias_chain_entry { + std::unique_ptr next; + const std::type_info *value; +}; +#endif + /// Additional type information which does not fit into the PyTypeObject. /// Changes to this struct also require bumping `PYBIND11_INTERNALS_VERSION`. struct type_info { @@ -357,6 +373,11 @@ 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 + std::unique_ptr 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..50a6dbcf05 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -246,6 +246,14 @@ 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 in the chain. + std::unique_ptr chain(new alias_chain_entry); + chain->value = &tp; + chain->next = std::move(it->second->alias_chain); + it->second->alias_chain = std::move(chain); 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..626d637e75 100644 --- a/tests/pybind11_cross_module_tests.cpp +++ b/tests/pybind11_cross_module_tests.cpp @@ -16,6 +16,13 @@ #include #include +class CrossDSOClass { +public: + virtual ~CrossDSOClass(); +}; + +CrossDSOClass::~CrossDSOClass() = default; + PYBIND11_MODULE(pybind11_cross_module_tests, m, py::mod_gil_not_used()) { m.doc() = "pybind11 cross-module test module"; @@ -146,4 +153,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", [](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..417f6afb25 --- /dev/null +++ b/tests/test_class_cross_module_use_after_one_module_dealloc.cpp @@ -0,0 +1,21 @@ +#include "pybind11_tests.h" + +#include + +class CrossDSOClass { +public: + virtual ~CrossDSOClass(); +}; + +CrossDSOClass::~CrossDSOClass() = default; + +struct UnrelatedClass {}; + +TEST_SUBMODULE(class_cross_module_use_after_one_module_dealloc, m) { + m.def("register_and_instantiate_cross_dso_class", [](py::module_ m) { + py::class_(m, "CrossDSOClass").def(py::init<>()); + return CrossDSOClass(); + }); + m.def("register_unrelated_class", + [](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..f2eafaa94f --- /dev/null +++ b/tests/test_class_cross_module_use_after_one_module_dealloc.py @@ -0,0 +1,59 @@ +from __future__ import annotations + +import gc +import types +import weakref + +import pytest + +import env # noqa: F401 +from pybind11_tests import class_cross_module_use_after_one_module_dealloc as m + + +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") +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 metaclass. 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|metaclass pointer| + # |&typeid(T) from DSO 2|metaclass pointer| + # +=======================================+ + # + # Then, if the metaclass 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 metaclass instance. 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) From 9877f5b65c65205de67e3f5b77f49792d01f76ef Mon Sep 17 00:00:00 2001 From: Scott Wolchok Date: Mon, 13 Oct 2025 21:35:42 -0700 Subject: [PATCH 2/6] review feedback, attempt to fix -Werror in CI --- include/pybind11/detail/class.h | 6 ++--- include/pybind11/detail/internals.h | 27 +++++++------------ include/pybind11/detail/type_caster_base.h | 7 ++--- tests/pybind11_cross_module_tests.cpp | 2 ++ ...ss_module_use_after_one_module_dealloc.cpp | 2 ++ ...oss_module_use_after_one_module_dealloc.py | 18 ++++++------- 6 files changed, 27 insertions(+), 35 deletions(-) diff --git a/include/pybind11/detail/class.h b/include/pybind11/detail/class.h index 6ccbc129e5..7fe692856b 100644 --- a/include/pybind11/detail/class.h +++ b/include/pybind11/detail/class.h @@ -228,12 +228,10 @@ 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); - const alias_chain_entry *chain = tinfo->alias_chain.get(); - while (chain) { - auto num_erased = internals.registered_types_cpp_fast.erase(chain->value); + 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); - chain = chain->next.get(); } #endif } diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index 51dbc017f6..4f8de120fe 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -335,22 +335,6 @@ enum class holder_enum_t : uint8_t { custom_holder, }; -// When a type appears in multiple DSOs, -// internals::registered_types_cpp_fast will have multiple distinct -// keys (the type_info from each DSO) mapped to the same -// 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 this structure 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 -#if PYBIND11_INTERNALS_VERSION >= 12 -struct alias_chain_entry { - std::unique_ptr next; - const std::type_info *value; -}; -#endif - /// Additional type information which does not fit into the PyTypeObject. /// Changes to this struct also require bumping `PYBIND11_INTERNALS_VERSION`. struct type_info { @@ -375,7 +359,16 @@ struct type_info { holder_enum_t holder_enum_v = holder_enum_t::undefined; #if PYBIND11_INTERNALS_VERSION >= 12 - std::unique_ptr alias_chain; + // 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 diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index 50a6dbcf05..baf7c64022 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -249,11 +249,8 @@ inline detail::type_info *get_global_type_info_lock_held(const std::type_info &t // 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 in the chain. - std::unique_ptr chain(new alias_chain_entry); - chain->value = &tp; - chain->next = std::move(it->second->alias_chain); - it->second->alias_chain = std::move(chain); + // that. + it->second->alias_chain.push_front(&tp); fast_types.emplace(&tp, it->second); #endif type_info = it->second; diff --git a/tests/pybind11_cross_module_tests.cpp b/tests/pybind11_cross_module_tests.cpp index 626d637e75..f567e3fc86 100644 --- a/tests/pybind11_cross_module_tests.cpp +++ b/tests/pybind11_cross_module_tests.cpp @@ -18,7 +18,9 @@ class CrossDSOClass { public: + CrossDSOClass() = default; virtual ~CrossDSOClass(); + CrossDSOClass(const CrossDSOClass &) = default; }; CrossDSOClass::~CrossDSOClass() = default; 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 index 417f6afb25..c8f1a2a2c2 100644 --- a/tests/test_class_cross_module_use_after_one_module_dealloc.cpp +++ b/tests/test_class_cross_module_use_after_one_module_dealloc.cpp @@ -4,7 +4,9 @@ class CrossDSOClass { public: + CrossDSOClass() = default; virtual ~CrossDSOClass(); + CrossDSOClass(const CrossDSOClass &) = default; }; CrossDSOClass::~CrossDSOClass() = default; 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 index f2eafaa94f..52c369e66d 100644 --- a/tests/test_class_cross_module_use_after_one_module_dealloc.py +++ b/tests/test_class_cross_module_use_after_one_module_dealloc.py @@ -30,18 +30,18 @@ def delattr_and_ensure_destroyed(*specs): 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 metaclass. If two DSOs both + # &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|metaclass pointer| - # |&typeid(T) from DSO 2|metaclass pointer| - # +=======================================+ + # +=========================================+ + # |&typeid(T) from DSO 1|type object pointer| + # |&typeid(T) from DSO 2|type object pointer| + # +=========================================+ # - # Then, if the metaclass 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 metaclass instance. Further lookups - # from DSO 2 will then return that dangling pointer, which will cause use-after-frees. + # 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 From 44e1110f8b79237a6e9eb57dc5071bd8911af543 Mon Sep 17 00:00:00 2001 From: Scott Wolchok Date: Mon, 13 Oct 2025 22:17:11 -0700 Subject: [PATCH 3/6] use const ref, skip test on python 3.13 free-threaded --- tests/pybind11_cross_module_tests.cpp | 2 +- ...t_class_cross_module_use_after_one_module_dealloc.cpp | 4 ++-- ...st_class_cross_module_use_after_one_module_dealloc.py | 9 +++++++-- 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/tests/pybind11_cross_module_tests.cpp b/tests/pybind11_cross_module_tests.cpp index f567e3fc86..837568352e 100644 --- a/tests/pybind11_cross_module_tests.cpp +++ b/tests/pybind11_cross_module_tests.cpp @@ -157,5 +157,5 @@ PYBIND11_MODULE(pybind11_cross_module_tests, m, py::mod_gil_not_used()) { m.def("missing_header_return", []() { return std::vector(); }); // test_class_cross_module_use_after_one_module_dealloc - m.def("consume_cross_dso_class", [](CrossDSOClass) {}); + 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 index c8f1a2a2c2..6f6e62debc 100644 --- a/tests/test_class_cross_module_use_after_one_module_dealloc.cpp +++ b/tests/test_class_cross_module_use_after_one_module_dealloc.cpp @@ -14,10 +14,10 @@ CrossDSOClass::~CrossDSOClass() = default; struct UnrelatedClass {}; TEST_SUBMODULE(class_cross_module_use_after_one_module_dealloc, m) { - m.def("register_and_instantiate_cross_dso_class", [](py::module_ 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", - [](py::module_ m) { py::class_(m, "UnrelatedClass"); }); + [](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 index 52c369e66d..c499ab7f65 100644 --- a/tests/test_class_cross_module_use_after_one_module_dealloc.py +++ b/tests/test_class_cross_module_use_after_one_module_dealloc.py @@ -1,14 +1,19 @@ from __future__ import annotations import gc +import sys import types import weakref import pytest -import env # noqa: F401 +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 sys.version_info == (3, 13) and not sys._is_gil_enabled() +) + def delattr_and_ensure_destroyed(*specs): wrs = [] @@ -26,7 +31,7 @@ def delattr_and_ensure_destroyed(*specs): ) -@pytest.mark.skipif("env.PYPY or env.GRAALPY") +@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 From e2c6b5305bba6fa5e409d81dd0d244e6e62bfbc2 Mon Sep 17 00:00:00 2001 From: Joshua Oreman Date: Tue, 14 Oct 2025 15:21:42 -0600 Subject: [PATCH 4/6] Skip test on 3.13t more robustly --- tests/test_class_cross_module_use_after_one_module_dealloc.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 index c499ab7f65..29ee923c62 100644 --- a/tests/test_class_cross_module_use_after_one_module_dealloc.py +++ b/tests/test_class_cross_module_use_after_one_module_dealloc.py @@ -2,6 +2,7 @@ import gc import sys +import sysconfig import types import weakref @@ -11,7 +12,7 @@ from pybind11_tests import class_cross_module_use_after_one_module_dealloc as m is_python_3_13_free_threaded = ( - env.CPYTHON and sys.version_info == (3, 13) and not sys._is_gil_enabled() + sysconfig.get_config_var("Py_GIL_DISABLED") and (3, 13) <= sys.version_info < (3, 14) ) From 8764a3000b20434627acb91925c610999bf74b70 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Tue, 14 Oct 2025 21:22:08 +0000 Subject: [PATCH 5/6] style: pre-commit fixes --- ...est_class_cross_module_use_after_one_module_dealloc.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) 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 index 29ee923c62..837e6226c1 100644 --- a/tests/test_class_cross_module_use_after_one_module_dealloc.py +++ b/tests/test_class_cross_module_use_after_one_module_dealloc.py @@ -8,12 +8,12 @@ import pytest -import env from pybind11_tests import class_cross_module_use_after_one_module_dealloc as m -is_python_3_13_free_threaded = ( - sysconfig.get_config_var("Py_GIL_DISABLED") and (3, 13) <= sys.version_info < (3, 14) -) +is_python_3_13_free_threaded = sysconfig.get_config_var("Py_GIL_DISABLED") and ( + 3, + 13, +) <= sys.version_info < (3, 14) def delattr_and_ensure_destroyed(*specs): From 532bf4e1c697d493763efbea69b393d458b3a28e Mon Sep 17 00:00:00 2001 From: Joshua Oreman Date: Tue, 14 Oct 2025 15:43:25 -0600 Subject: [PATCH 6/6] CI fix --- ..._class_cross_module_use_after_one_module_dealloc.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) 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 index 837e6226c1..ac8a47a8fc 100644 --- a/tests/test_class_cross_module_use_after_one_module_dealloc.py +++ b/tests/test_class_cross_module_use_after_one_module_dealloc.py @@ -8,12 +8,14 @@ import pytest +import env from pybind11_tests import class_cross_module_use_after_one_module_dealloc as m -is_python_3_13_free_threaded = sysconfig.get_config_var("Py_GIL_DISABLED") and ( - 3, - 13, -) <= sys.version_info < (3, 14) +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):