From 4edb1ce20c323c0e91cdbb0f309f15550eaafb5d Mon Sep 17 00:00:00 2001
From: Jason Rhinelander <jason@imaginary.ca>
Date: Wed, 7 Jun 2017 12:35:39 -0300
Subject: [PATCH] Destroy internals if created during Py_Finalize()

Py_Finalize could potentially invoke code that calls `get_internals()`,
which could create a new internals object if one didn't exist.
`finalize_interpreter()` didn't catch this because it only used the
pre-finalize interpreter pointer status; if this happens, it results in
the internals pointer not being properly destroyed with the interpreter,
which leaks, and also causes a `get_internals()` under a future
interpreter to return an internals object that is wrong in various ways.
---
 include/pybind11/embed.h              |  6 ++++-
 tests/test_embed/test_interpreter.cpp | 37 ++++++++++++++++++++++-----
 2 files changed, 36 insertions(+), 7 deletions(-)

diff --git a/include/pybind11/embed.h b/include/pybind11/embed.h
index 29d0950b2..a3f6a4e67 100644
--- a/include/pybind11/embed.h
+++ b/include/pybind11/embed.h
@@ -143,7 +143,11 @@ inline void finalize_interpreter() {
     handle builtins(PyEval_GetBuiltins());
     const char *id = PYBIND11_INTERNALS_ID;
 
-    detail::internals **internals_ptr_ptr = nullptr;
+    // Get the internals pointer (without creating it if it doesn't exist).  It's possible for the
+    // internals to be created during Py_Finalize() (e.g. if a py::capsule calls `get_internals()`
+    // during destruction), so we get the pointer-pointer here and check it after Py_Finalize().
+    detail::internals **internals_ptr_ptr = &detail::get_internals_ptr();
+    // It could also be stashed in builtins, so look there too:
     if (builtins.contains(id) && isinstance<capsule>(builtins[id]))
         internals_ptr_ptr = capsule(builtins[id]);
 
diff --git a/tests/test_embed/test_interpreter.cpp b/tests/test_embed/test_interpreter.cpp
index 6501c85d9..58f791a61 100644
--- a/tests/test_embed/test_interpreter.cpp
+++ b/tests/test_embed/test_interpreter.cpp
@@ -84,15 +84,20 @@ TEST_CASE("There can be only one interpreter") {
     py::initialize_interpreter();
 }
 
-bool has_pybind11_internals() {
+bool has_pybind11_internals_builtin() {
     auto builtins = py::handle(PyEval_GetBuiltins());
     return builtins.contains(PYBIND11_INTERNALS_ID);
 };
 
+bool has_pybind11_internals_static() {
+    return py::detail::get_internals_ptr() != nullptr;
+}
+
 TEST_CASE("Restart the interpreter") {
     // Verify pre-restart state.
     REQUIRE(py::module::import("widget_module").attr("add")(1, 2).cast<int>() == 3);
-    REQUIRE(has_pybind11_internals());
+    REQUIRE(has_pybind11_internals_builtin());
+    REQUIRE(has_pybind11_internals_static());
 
     // Restart the interpreter.
     py::finalize_interpreter();
@@ -102,9 +107,27 @@ TEST_CASE("Restart the interpreter") {
     REQUIRE(Py_IsInitialized() == 1);
 
     // Internals are deleted after a restart.
-    REQUIRE_FALSE(has_pybind11_internals());
+    REQUIRE_FALSE(has_pybind11_internals_builtin());
+    REQUIRE_FALSE(has_pybind11_internals_static());
     pybind11::detail::get_internals();
-    REQUIRE(has_pybind11_internals());
+    REQUIRE(has_pybind11_internals_builtin());
+    REQUIRE(has_pybind11_internals_static());
+
+    // Make sure that an interpreter with no get_internals() created until finalize still gets the
+    // internals destroyed
+    py::finalize_interpreter();
+    py::initialize_interpreter();
+    bool ran = false;
+    py::module::import("__main__").attr("internals_destroy_test") =
+        py::capsule(&ran, [](void *ran) { py::detail::get_internals(); *static_cast<bool *>(ran) = true; });
+    REQUIRE_FALSE(has_pybind11_internals_builtin());
+    REQUIRE_FALSE(has_pybind11_internals_static());
+    REQUIRE_FALSE(ran);
+    py::finalize_interpreter();
+    REQUIRE(ran);
+    py::initialize_interpreter();
+    REQUIRE_FALSE(has_pybind11_internals_builtin());
+    REQUIRE_FALSE(has_pybind11_internals_static());
 
     // C++ modules can be reloaded.
     auto cpp_module = py::module::import("widget_module");
@@ -125,7 +148,8 @@ TEST_CASE("Subinterpreter") {
 
         REQUIRE(m.attr("add")(1, 2).cast<int>() == 3);
     }
-    REQUIRE(has_pybind11_internals());
+    REQUIRE(has_pybind11_internals_builtin());
+    REQUIRE(has_pybind11_internals_static());
 
     /// Create and switch to a subinterpreter.
     auto main_tstate = PyThreadState_Get();
@@ -134,7 +158,8 @@ TEST_CASE("Subinterpreter") {
     // Subinterpreters get their own copy of builtins. detail::get_internals() still
     // works by returning from the static variable, i.e. all interpreters share a single
     // global pybind11::internals;
-    REQUIRE_FALSE(has_pybind11_internals());
+    REQUIRE_FALSE(has_pybind11_internals_builtin());
+    REQUIRE(has_pybind11_internals_static());
 
     // Modules tags should be gone.
     REQUIRE_FALSE(py::hasattr(py::module::import("__main__"), "tag"));