Fix segfault when reloading interpreter with external modules (#1092)

* Fix segfault when reloading interpreter with external modules

When embedding the interpreter and loading external modules in that
embedded interpreter, the external module correctly shares its
internals_ptr with the one in the embedded interpreter.  When the
interpreter is shut down, however, only the `internals_ptr` local to
the embedded code is actually reset to nullptr: the external module
remains set.

The result is that loading an external pybind11 module, letting the
interpreter go through a finalize/initialize, then attempting to use
something in the external module fails because this external module is
still trying to use the old (destroyed) internals.  This causes
undefined behaviour (typically a segfault).

This commit fixes it by adding a level of indirection in the internals
path, converting the local internals variable to `internals **` instead
of `internals *`.  With this change, we can detect a stale internals
pointer and reload the internals pointer (either from a capsule or by
creating a new internals instance).

(No issue number: this was reported on gitter by @henryiii and @aoloe).

tests/test_embed/CMakeLists.txt

@@ -33,4 +33,9 @@ target_link_libraries(test_embed PUBLIC ${CMAKE_THREAD_LIBS_INIT})
 
 add_custom_target(cpptest COMMAND $<TARGET_FILE:test_embed>
                   WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR})
+
+pybind11_add_module(external_module THIN_LTO external_module.cpp)
+set_target_properties(external_module PROPERTIES LIBRARY_OUTPUT_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR})
+add_dependencies(cpptest external_module)
+
 add_dependencies(check cpptest)

tests/test_embed/external_module.cpp

@@ -0,0 +1,23 @@
+#include <pybind11/pybind11.h>
+
+namespace py = pybind11;
+
+/* Simple test module/test class to check that the referenced internals data of external pybind11
+ * modules aren't preserved over a finalize/initialize.
+ */
+
+PYBIND11_MODULE(external_module, m) {
+    class A {
+    public:
+        A(int value) : v{value} {};
+        int v;
+    };
+
+    py::class_<A>(m, "A")
+        .def(py::init<int>())
+        .def_readwrite("value", &A::v);
+
+    m.def("internals_at", []() {
+        return reinterpret_cast<uintptr_t>(&py::detail::get_internals());
+    });
+}

tests/test_embed/test_interpreter.cpp

@@ -101,7 +101,8 @@ bool has_pybind11_internals_builtin() {
 };
 
 bool has_pybind11_internals_static() {
-    return py::detail::get_internals_ptr() != nullptr;
+    auto **&ipp = py::detail::get_internals_pp();
+    return ipp && *ipp;
 }
 
 TEST_CASE("Restart the interpreter") {
@@ -109,6 +110,11 @@ TEST_CASE("Restart the interpreter") {
     REQUIRE(py::module::import("widget_module").attr("add")(1, 2).cast<int>() == 3);
     REQUIRE(has_pybind11_internals_builtin());
     REQUIRE(has_pybind11_internals_static());
+    REQUIRE(py::module::import("external_module").attr("A")(123).attr("value").cast<int>() == 123);
+
+    // local and foreign module internals should point to the same internals:
+    REQUIRE(reinterpret_cast<uintptr_t>(*py::detail::get_internals_pp()) ==
+            py::module::import("external_module").attr("internals_at")().cast<uintptr_t>());
 
     // Restart the interpreter.
     py::finalize_interpreter();
@@ -123,6 +129,8 @@ TEST_CASE("Restart the interpreter") {
     pybind11::detail::get_internals();
     REQUIRE(has_pybind11_internals_builtin());
     REQUIRE(has_pybind11_internals_static());
+    REQUIRE(reinterpret_cast<uintptr_t>(*py::detail::get_internals_pp()) ==
+            py::module::import("external_module").attr("internals_at")().cast<uintptr_t>());
 
     // Make sure that an interpreter with no get_internals() created until finalize still gets the
     // internals destroyed