Uh oh!
There was an error while loading. Please reload this page.
Fix dangling pointer in internals::registered_types_cpp_fast from #5842 - #5867
Conversation
…ind#5842@oremanj pointed out in a comment on pybind#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.
| // 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 |
There was a problem hiding this comment.
| // 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 | |
| // 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 |
Suggest disambiguating since both the key and value types are something::type_info
| // nb_alias_chain` added in | ||
| // https://github.com/wjakob/nanobind/commit/b515b1f7f2f4ecc0357818e6201c94a9f4cbfdc2 | ||
| #if PYBIND11_INTERNALS_VERSION >= 12 | ||
| struct alias_chain_entry { |
There was a problem hiding this comment.
You can use std::forward_list instead of rolling your own singly linked list. nanobind goes to a lot of trouble to avoid anything from the STL, while pybind11 already has other uses of forward_list and it is basically the same thing you wrote here.
There was a problem hiding this comment.
TIL sizeof(std::forward_list) == 8 and it's in C++11, SGTM
| 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 |
There was a problem hiding this comment.
These are just Python classes (types), not metaclasses (their instances are not types). I might say "type object pointer" to disambiguate from the std::type_info pointer.
oremanj
left a comment
There was a problem hiding this comment.
You will need to skip the new test on 3.13 free-threaded, since types are immortal there. It should work fine on 3.14+ free-threaded. Suggestions are what I expect you need to make clang-tidy happy.
| struct UnrelatedClass {}; | ||
| TEST_SUBMODULE(class_cross_module_use_after_one_module_dealloc, m) { | ||
| m.def("register_and_instantiate_cross_dso_class", [](py::module_ m) { |
There was a problem hiding this comment.
| m.def("register_and_instantiate_cross_dso_class", [](py::module_ m) { | |
| m.def("register_and_instantiate_cross_dso_class", [](constpy::module_ &m) { |
| return CrossDSOClass(); | ||
| }); | ||
| m.def("register_unrelated_class", | ||
| [](py::module_ m) { py::class_<UnrelatedClass>(m, "UnrelatedClass"); }); |
There was a problem hiding this comment.
| [](py::module_ m) { py::class_<UnrelatedClass>(m, "UnrelatedClass"); }); | |
| [](constpy::module_ &m) { py::class_<UnrelatedClass>(m, "UnrelatedClass"); }); |
| m.def("missing_header_return", []() { return std::vector<float>(); }); | ||
| // test_class_cross_module_use_after_one_module_dealloc | ||
| m.def("consume_cross_dso_class", [](CrossDSOClass) {}); |
There was a problem hiding this comment.
| m.def("consume_cross_dso_class", [](CrossDSOClass) {}); | |
| m.def("consume_cross_dso_class", [](constCrossDSOClass &) {}); |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
swolchok
commented
Oct 15, 2025
Thanks for fixing things up @oremanj! Was out today, planned to get to it tomorrow |
Description
@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.