Fix dangling pointer in internals::registered_types_cpp_fast from #5842#5867
Fix dangling pointer in internals::registered_types_cpp_fast from #5842#5867oremanj merged 6 commits intopybind:masterfrom
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.
include/pybind11/detail/internals.h
Outdated
| // 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
include/pybind11/detail/internals.h
Outdated
| // 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
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", [](const py::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"); }); | |
| [](const py::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", [](const CrossDSOClass &) {}); |
|
Thanks for fixing things up @oremanj! Was out today, planned to get to it tomorrow |
3 participants
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.