From 7d635574400392dfb8818a13d8b4a4ac12e6fb07 Mon Sep 17 00:00:00 2001 From: Aaron Gokaslan Date: Wed, 8 Feb 2023 11:08:04 -0500 Subject: [PATCH 1/4] Fix #4459 (cpp atexit callbacks) without segfault --- include/pybind11/embed.h | 16 ++++++++++++---- tests/test_embed/catch.cpp | 3 ++- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/include/pybind11/embed.h b/include/pybind11/embed.h index 5a175b1341..ae5237d0e2 100644 --- a/include/pybind11/embed.h +++ b/include/pybind11/embed.h @@ -254,13 +254,21 @@ inline void finalize_interpreter() { if (builtins.contains(id) && isinstance(builtins[id])) { internals_ptr_ptr = capsule(builtins[id]); } - // Local internals contains data managed by the current interpreter, so we must clear them to - // avoid undefined behaviors when initializing another interpreter - detail::get_local_internals().registered_types_cpp.clear(); - detail::get_local_internals().registered_exception_translators.clear(); + + // We must clear this data after the interpreter is finalized but calling get_local_internals() + // is UB at that point, so he have to cache the references here. + auto &local_types_cpp = detail::get_local_internals().registered_types_cpp; + auto &local_exception_translators + = detail::get_local_internals().registered_exception_translators; Py_Finalize(); + // This local data is needed during Py_Finalize() for callbacks or hooks such as atexit. + // Local internals contains data managed by the current interpreter, so we must clear them to + // avoid undefined behaviors when initializing another interpreter + local_types_cpp.clear(); + local_exception_translators.clear(); + if (internals_ptr_ptr) { delete *internals_ptr_ptr; *internals_ptr_ptr = nullptr; diff --git a/tests/test_embed/catch.cpp b/tests/test_embed/catch.cpp index 558a7a35e5..8fec877788 100644 --- a/tests/test_embed/catch.cpp +++ b/tests/test_embed/catch.cpp @@ -34,7 +34,8 @@ int main(int argc, char *argv[]) { #else setenv("PYTHONPATH", updated_pythonpath.c_str(), /*replace=*/1); #endif - + // Below is a test case for a potential segfault. See PR #4500. + { py::scoped_interpreter guard; } py::scoped_interpreter guard{}; auto result = Catch::Session().run(argc, argv); From 0684269045db1a2242e6cfabdf3732865e935809 Mon Sep 17 00:00:00 2001 From: Aaron Gokaslan Date: Wed, 8 Feb 2023 11:29:13 -0500 Subject: [PATCH 2/4] Make local_internals non-copyable. Simplify embed.h code. --- include/pybind11/detail/internals.h | 2 ++ include/pybind11/embed.h | 10 ++++------ 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index 05e36ad18b..71a45bf09f 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -536,6 +536,8 @@ struct local_internals { = static_cast(ptr)->loader_life_support_tls_key; } #endif // defined(WITH_THREAD) && PYBIND11_INTERNALS_VERSION == 4 + local_internals(const local_internals &) = delete; + local_internals &operator=(const local_internals &) = delete; }; /// Works like `get_internals`, but for things which are locally registered. diff --git a/include/pybind11/embed.h b/include/pybind11/embed.h index ae5237d0e2..02f2cff0d6 100644 --- a/include/pybind11/embed.h +++ b/include/pybind11/embed.h @@ -256,18 +256,16 @@ inline void finalize_interpreter() { } // We must clear this data after the interpreter is finalized but calling get_local_internals() - // is UB at that point, so he have to cache the references here. - auto &local_types_cpp = detail::get_local_internals().registered_types_cpp; - auto &local_exception_translators - = detail::get_local_internals().registered_exception_translators; + // is UB at that point, so he have to cache a references here. + auto &local_internals = detail::get_local_internals(); Py_Finalize(); // This local data is needed during Py_Finalize() for callbacks or hooks such as atexit. // Local internals contains data managed by the current interpreter, so we must clear them to // avoid undefined behaviors when initializing another interpreter - local_types_cpp.clear(); - local_exception_translators.clear(); + local_internals.registered_types_cpp.clear(); + local_internals.registered_exception_translators.clear(); if (internals_ptr_ptr) { delete *internals_ptr_ptr; From 86acb12f58b42fbee9da1eda8bdb200797a5ee4c Mon Sep 17 00:00:00 2001 From: Aaron Gokaslan Date: Wed, 8 Feb 2023 11:37:56 -0500 Subject: [PATCH 3/4] Include default ctor for local_internals --- include/pybind11/detail/internals.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index 71a45bf09f..7c5137d05f 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -535,6 +535,8 @@ struct local_internals { loader_life_support_tls_key = static_cast(ptr)->loader_life_support_tls_key; } +#else + local_internals() = default; #endif // defined(WITH_THREAD) && PYBIND11_INTERNALS_VERSION == 4 local_internals(const local_internals &) = delete; local_internals &operator=(const local_internals &) = delete; From 91cbc66f49790803024e24e7a5494625836d6cfc Mon Sep 17 00:00:00 2001 From: Aaron Gokaslan Date: Tue, 14 Feb 2023 10:01:52 -0500 Subject: [PATCH 4/4] Try hack as optimization fence --- include/pybind11/embed.h | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/include/pybind11/embed.h b/include/pybind11/embed.h index 02f2cff0d6..35fbce6f40 100644 --- a/include/pybind11/embed.h +++ b/include/pybind11/embed.h @@ -259,13 +259,19 @@ inline void finalize_interpreter() { // is UB at that point, so he have to cache a references here. auto &local_internals = detail::get_local_internals(); + // Hack to materialize local internals from static method variable before Py_Finalize. + bool need_to_clear = !local_internals.registered_exception_translators.empty() + || !local_internals.registered_exception_translators.empty(); + Py_Finalize(); // This local data is needed during Py_Finalize() for callbacks or hooks such as atexit. // Local internals contains data managed by the current interpreter, so we must clear them to // avoid undefined behaviors when initializing another interpreter - local_internals.registered_types_cpp.clear(); - local_internals.registered_exception_translators.clear(); + if (need_to_clear) { + local_internals.registered_types_cpp.clear(); + local_internals.registered_exception_translators.clear(); + } if (internals_ptr_ptr) { delete *internals_ptr_ptr;