Skip to content

Commit 8f25a25

Browse files
committed
Revert "Revert to leak internals"
This reverts commit c5ec1cf. This reverts commit 72c2e0a.
1 parent c5ec1cf commit 8f25a25

File tree

2 files changed

+79
-10
lines changed

2 files changed

+79
-10
lines changed

include/pybind11/detail/internals.h

Lines changed: 54 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@
3939
/// further ABI-incompatible changes may be made before the ABI is officially
4040
/// changed to the new version.
4141
#ifndef PYBIND11_INTERNALS_VERSION
42-
# define PYBIND11_INTERNALS_VERSION 11
42+
# define PYBIND11_INTERNALS_VERSION 12
4343
#endif
4444

4545
#if PYBIND11_INTERNALS_VERSION < 11
@@ -337,7 +337,20 @@ struct internals {
337337
internals(internals &&other) = delete;
338338
internals &operator=(const internals &other) = delete;
339339
internals &operator=(internals &&other) = delete;
340-
~internals() = default;
340+
~internals() {
341+
// Normally this destructor runs during interpreter finalization and it may DECREF things.
342+
// In odd finalization scenarios it might end up running after the interpreter has
343+
// completely shut down, In that case, we should not decref these objects because pymalloc
344+
// is gone. This also applies across sub-interpreters, we should only DECREF when the
345+
// original owning interpreter is active.
346+
auto *cur_istate = get_interpreter_state_unchecked();
347+
if (cur_istate && cur_istate == istate) {
348+
gil_scoped_acquire_simple gil;
349+
Py_CLEAR(instance_base);
350+
Py_CLEAR(default_metaclass);
351+
Py_CLEAR(static_property_type);
352+
}
353+
}
341354
};
342355

343356
// the internals struct (above) is shared between all the modules. local_internals are only
@@ -347,13 +360,29 @@ struct internals {
347360
// impact any other modules, because the only things accessing the local internals is the
348361
// module that contains them.
349362
struct local_internals {
363+
local_internals() : istate(get_interpreter_state_unchecked()) {}
364+
350365
// It should be safe to use fast_type_map here because this entire
351366
// data structure is scoped to our single module, and thus a single
352367
// DSO and single instance of type_info for any particular type.
353368
fast_type_map<type_info *> registered_types_cpp;
354369

355370
std::forward_list<ExceptionTranslator> registered_exception_translators;
356371
PyTypeObject *function_record_py_type = nullptr;
372+
PyInterpreterState *istate = nullptr;
373+
374+
~local_internals() {
375+
// Normally this destructor runs during interpreter finalization and it may DECREF things.
376+
// In odd finalization scenarios it might end up running after the interpreter has
377+
// completely shut down, In that case, we should not decref these objects because pymalloc
378+
// is gone. This also applies across sub-interpreters, we should only DECREF when the
379+
// original owning interpreter is active.
380+
auto *cur_istate = get_interpreter_state_unchecked();
381+
if (cur_istate && cur_istate == istate) {
382+
gil_scoped_acquire_simple gil;
383+
Py_CLEAR(function_record_py_type);
384+
}
385+
}
357386
};
358387

359388
enum class holder_enum_t : uint8_t {
@@ -712,16 +741,27 @@ class internals_pp_manager {
712741
internals_pp_manager(char const *id, on_fetch_function *on_fetch)
713742
: holder_id_(id), on_fetch_(on_fetch) {}
714743

744+
static void internals_shutdown(PyObject *capsule) {
745+
auto *pp = static_cast<std::unique_ptr<InternalsType> *>(
746+
PyCapsule_GetPointer(capsule, nullptr));
747+
if (pp) {
748+
pp->reset();
749+
}
750+
// We reset the unique_ptr's contents but cannot delete the unique_ptr itself here.
751+
// The pp_manager in this module (and possibly other modules sharing internals) holds
752+
// a raw pointer to this unique_ptr, and that pointer would dangle if we deleted it now.
753+
//
754+
// For pybind11-owned interpreters (via embed.h or subinterpreter.h), destroy() is
755+
// called after Py_Finalize/Py_EndInterpreter completes, which safely deletes the
756+
// unique_ptr. For interpreters not owned by pybind11 (e.g., a pybind11 extension
757+
// loaded into an external interpreter), destroy() is never called and the unique_ptr
758+
// shell (8 bytes, not its contents) is leaked.
759+
// (See PR #5958 for ideas to eliminate this leak.)
760+
}
761+
715762
std::unique_ptr<InternalsType> *get_or_create_pp_in_state_dict() {
716-
// The `unique_ptr<InternalsType>` is intentionally leaked on interpreter shutdown.
717-
// Once an instance is created, it will never be deleted until the process exits (compare
718-
// to interpreter shutdown in multiple-interpreter scenarios).
719-
// We cannot guarantee the destruction order of capsules in the interpreter state dict on
720-
// interpreter shutdown, so deleting internals too early could cause undefined behavior
721-
// when other pybind11 objects access `get_internals()` during finalization (which would
722-
// recreate empty internals).
723763
auto result = atomic_get_or_create_in_state_dict<std::unique_ptr<InternalsType>>(
724-
holder_id_, /*dtor=*/nullptr /* leak the capsule content */);
764+
holder_id_, &internals_shutdown);
725765
auto *pp = result.first;
726766
bool created = result.second;
727767
// Only call on_fetch_ when fetching existing internals, not when creating new ones.
@@ -801,6 +841,8 @@ PYBIND11_NOINLINE internals &get_internals() {
801841

802842
/// Return the PyObject* for the internals capsule (borrowed reference).
803843
/// Returns nullptr if the capsule doesn't exist yet.
844+
/// This is used to prevent use-after-free during interpreter shutdown by allowing pybind11 types
845+
/// to hold a reference to the capsule (see comments in generic_type::initialize).
804846
inline PyObject *get_internals_capsule() {
805847
auto state_dict = reinterpret_borrow<dict>(get_python_state_dict());
806848
return dict_getitemstring(state_dict.ptr(), PYBIND11_INTERNALS_ID);
@@ -818,6 +860,8 @@ inline const std::string &get_local_internals_key() {
818860

819861
/// Return the PyObject* for the local_internals capsule (borrowed reference).
820862
/// Returns nullptr if the capsule doesn't exist yet.
863+
/// This is used to prevent use-after-free during interpreter shutdown by allowing pybind11 types
864+
/// to hold a reference to the capsule (see comments in generic_type::initialize).
821865
inline PyObject *get_local_internals_capsule() {
822866
const auto &key = get_local_internals_key();
823867
auto state_dict = reinterpret_borrow<dict>(get_python_state_dict());

include/pybind11/pybind11.h

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1733,6 +1733,31 @@ class generic_type : public object {
17331733
PYBIND11_WARNING_POP
17341734
});
17351735

1736+
// Prevent use-after-free during interpreter shutdown. GC order is not guaranteed, so the
1737+
// internals capsule may be destroyed (resetting internals via internals_shutdown) before
1738+
// all pybind11 types are destroyed. If a type's tp_traverse/tp_clear then calls py::cast,
1739+
// it would recreate an empty internals and fail because the type registry is gone.
1740+
//
1741+
// By holding references to the capsules, we ensure they outlive all pybind11 types. We use
1742+
// weakrefs on the type with a cpp_function callback. When the type is destroyed, Python
1743+
// will call the callback which releases the capsule reference and the weakref.
1744+
if (PyObject *capsule = get_internals_capsule()) {
1745+
Py_INCREF(capsule);
1746+
(void) weakref(handle(m_ptr), cpp_function([](handle wr) -> void {
1747+
Py_XDECREF(get_internals_capsule());
1748+
wr.dec_ref();
1749+
}))
1750+
.release();
1751+
}
1752+
if (PyObject *capsule = get_local_internals_capsule()) {
1753+
Py_INCREF(capsule);
1754+
(void) weakref(handle(m_ptr), cpp_function([](handle wr) -> void {
1755+
Py_XDECREF(get_local_internals_capsule());
1756+
wr.dec_ref();
1757+
}))
1758+
.release();
1759+
}
1760+
17361761
if (rec.bases.size() > 1 || rec.multiple_inheritance) {
17371762
mark_parents_nonsimple(tinfo->type);
17381763
tinfo->simple_ancestors = false;

0 commit comments

Comments
 (0)