Skip to content

Fix memory leak: clear managed dict in pybind11_object_dealloc on Python 3.13+ #5999

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rwgk merged 3 commits into from
Mar 24, 2026
+52 −0
Merged

Fix memory leak: clear managed dict in pybind11_object_dealloc on Python 3.13+ #5999

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0f50e4d
fix: clear managed dict in pybind11_object_dealloc on Python 3.13+
Feb 27, 2026
73c677c
[tests]: mark test_dynamic_attr_dealloc_frees_dict_contents to be str…
Mar 11, 2026
6c50b1b
[docs]: clarify Python version comments in pybind11_object_dealloc
Mar 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions include/pybind11/detail/class.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -503,6 +503,17 @@ extern "C" inline void pybind11_object_dealloc(PyObject *self) {
PyObject_GC_UnTrack(self);
}

#if PY_VERSION_HEX >= 0x030D0000
// PyObject_ClearManagedDict() is available from Python 3.13+. It must be
// called before tp_free() because on Python 3.14+ tp_free no longer
// implicitly clears the managed dict, which would abandon the refcounts of
// objects stored in __dict__ of py::dynamic_attr() types, causing permanent
// memory leaks.
if (PyType_HasFeature(type, Py_TPFLAGS_MANAGED_DICT)) {
PyObject_ClearManagedDict(self);
}
#endif

clear_instance(self);

type->tp_free(self);
Expand Down
24 changes: 24 additions & 0 deletions tests/test_methods_and_attributes.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,12 @@
#include "constructor_stats.h"
#include "pybind11_tests.h"

#if !defined(PYPY_VERSION)
// Flag set by the capsule destructor in test_dynamic_attr_dealloc_frees_dict_contents.
// File scope so the captureless capsule destructor (void(*)(void*)) can access it.
static bool s_dynamic_attr_capsule_freed = false;
#endif

#if !defined(PYBIND11_OVERLOAD_CAST)
template <typename... Args>
using overload_cast_ = pybind11::detail::overload_cast_impl<Args...>;
Expand Down Expand Up @@ -388,6 +394,24 @@ TEST_SUBMODULE(methods_and_attributes, m) {

class CppDerivedDynamicClass : public DynamicClass {};
py::class_<CppDerivedDynamicClass, DynamicClass>(m, "CppDerivedDynamicClass").def(py::init());

// test_dynamic_attr_dealloc_frees_dict_contents
// Regression test: pybind11_object_dealloc() must call PyObject_ClearManagedDict()
// before tp_free() so that objects stored in a py::dynamic_attr() instance __dict__
// have their refcounts decremented when the pybind11 object is freed. On Python 3.14+
Copy link
Collaborator

@rwgk rwgk Mar 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Opus 4.6 1M Thinking prompt and response:

Question

There is a small asymmetry between:

include/pybind11/detail/class.h line 507:

// On Python 3.13+, PyObject_GC_Del no longer implicitly clears the managed

tests/test_methods_and_attributes.cpp line 401+:

// have their refcounts decremented when the pybind11 object is freed. On Python 3.14+
// tp_free no longer implicitly clears the managed dict, causing permanent leaks.

Should it be Python 3.13+ also in test_methods_and_attributes.cpp?

Analysis

The code fix in class.h is gated on 3.13+ (#if PY_VERSION_HEX >= 0x030D0000), which is correct because PyObject_ClearManagedDict and Py_TPFLAGS_MANAGED_DICT were introduced in 3.13, and calling it is safe (idempotent) even if 3.13 still does the implicit clearing. The PR description itself says "On Python <= 3.13, PyObject_GC_Del ... cleared the managed dict as a side effect. On Python 3.14 this implicit clearing was removed."

So the comment in the test is more precisely correct about when the bug manifests (3.14+), while the code fix in class.h is correct about when the guard is needed (3.13+, since that's when the API exists).

The test comment should say "3.13+" to be consistent with the fix's comment and the #if guard. The leak behavior might only be observable on 3.14+, but the comment is describing the fix rationale, not the exact Python version where the bug appears. Saying "3.13+" keeps both comments aligned and avoids confusing a reader who sees 0x030D0000 (3.13) in the code but "3.14+" in the test comment.

Copy link
Contributor Author

@yamatveev yamatveev Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I made the comments aligned. I do not know your policy: should I just add new commits or squash them and force-push?

// tp_free no longer implicitly clears the managed dict, causing permanent leaks.
m.def("make_dynamic_attr_with_capsule", []() -> py::object {
s_dynamic_attr_capsule_freed = false;
Copy link
Collaborator

@rwgk rwgk Mar 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering if there is an easy way to avoid the global variable (and the subtle pitfalls that come with globals). Cursor then pointed out this:

DynamicClass already exists in the test module as a py::dynamic_attr() class, so the test can be done entirely in Python:

def test_dynamic_attr_dealloc_frees_dict_contents():
    freed = [False]

    class Flag:
        def __del__(self):
            freed[0] = True

    obj = m.DynamicClass()
    obj.flag = Flag()
    del obj
    pytest.gc_collect()
    assert freed[0]

This would remove the need for s_dynamic_attr_capsule_freed, make_dynamic_attr_with_capsule, and is_dynamic_attr_capsule_freed on the C++ side entirely.

It should work because obj.flag = Flag() stores into the managed dict — exactly the mechanism being tested. The __del__ on CPython is called synchronously when the refcount hits 0. For PyPy/GraalPy (where __del__ timing is non-deterministic), the test is already xfail/skip.

Could you please try this out?

Copy link
Contributor Author

@yamatveev yamatveev Mar 23, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, this won't work, this test always passes, event if I comment out fix. Here is claude summary:

Here's what happens with the commented-out fix on Python 3.14:

After del obj → pybind11_object_dealloc:

  1. PyObject_GC_UnTrack(self) — removes the pybind11 object from GC tracking
  2. PyObject_ClearManagedDict — skipped (fix commented out)
  3. tp_free(self) — frees raw memory; the managed dict's ob_refcnt is still 1 (now a dangling reference from freed memory)

Now pytest.gc_collect() runs the GC. The GC initializes gc_refs = ob_refcnt for all tracked objects and traverses them to subtract internal references:

Object gc_refs start effect of traversal gc_refs after
orphaned dict 1 nobody decrements it (pybind11 obj is untracked) 1 → "reachable from outside"
Flag instance 1 dict.tp_traverse decrements it 0 → "unreachable candidate"
py::capsule n/a not GC-tracked, invisible never enters GC set

After this phase:

  • Flag has gc_refs=0 → GC considers it a finalization candidate → del is called → freed[0] = True → test passes ✓
  • py::capsule is not GC-tracked → never gets gc_refs=0 → GC never sees it → destructor never runs → test fails ✗

The dict stays perpetually "reachable" (gc_refs=1 from the dangling reference in freed memory), so it leaks permanently along with the capsule. The Flag gets rescued as a side effect of the GC's traversal decrementing its to 0 — which masks the exact bug we're testing for.

The pure Python test is therefore not a valid regression test for this bug. We need to revert to the capsule approach.

Copy link
Collaborator

@rwgk rwgk Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed (Cursor Opus 4.6 1M Thinking):

  This is a great catch by Yamatveev (and their Claude). The pure Python Flag/__del__ approach I suggested earlier doesn't work as a regression test -- here's why:
  The core issue: `py::capsule` is not GC-tracked, but a Python `Flag` object is.
  When the fix is commented out (bug present), after del obj:
  1. pybind11_object_dealloc calls tp_free(self) without clearing the managed dict
  2. The dict becomes an orphan with a dangling reference from freed memory (gc_refs stays at 1)
  3. pytest.gc_collect() runs the cyclic GC

  For the capsule approach: py::capsule is not GC-tracked, so GC never sees it, never decrements its refcount, and its destructor never runs. The test correctly fails without the fix.
  For the Flag/`__del__` approach: The Flag instance IS GC-tracked. During GC traversal, the orphaned dict's tp_traverse decrements Flag's gc_refs from 1 to 0. GC considers it unreachable,
   finalizes it, and calls __del__ -- setting freed[0] = True. The test passes even without the fix, because GC's traversal accidentally rescues the Flag by decrementing its gc_refs to 0.
  So the capsule approach is essential: it tests exactly the right thing because capsules are invisible to the cyclic GC. The pure Python suggestion was wrong. The C++ global variable is a
   necessary price for a correct regression test.
  Yamatveev also addressed the version comment asymmetry in commit 6c50b1b, aligning the test comment to say "3.13+" consistently.

auto *dummy = new int(0);
py::capsule cap(dummy, [](void *ptr) {
delete static_cast<int *>(ptr);
s_dynamic_attr_capsule_freed = true;
});
py::object obj = py::cast(new DynamicClass(), py::return_value_policy::take_ownership);
obj.attr("data") = cap;
return obj;
});
m.def("is_dynamic_attr_capsule_freed", []() { return s_dynamic_attr_capsule_freed; });
#endif

// test_bad_arg_default
Expand Down
17 changes: 17 additions & 0 deletions tests/test_methods_and_attributes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -383,6 +383,23 @@ def test_cyclic_gc():
assert cstats.alive() == 0


@pytest.mark.xfail("env.PYPY", strict=False)
@pytest.mark.skipif("env.GRAALPY", reason="Cannot reliably trigger GC")
Copy link
Collaborator

@Skylion007 Skylion007 Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be xfail strict=False for flakey tests?

Copy link
Collaborator

@henryiii henryiii Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skipping it is fine if passing vs. failing doesn't mean anything. If you hope it always passes some day, then yes.

Copy link
Contributor Author

@yamatveev yamatveev Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, should I modify it or not?

Copy link
Collaborator

@henryiii henryiii Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For GRAALPY, it's fine. If the comment was that the PYPY xfail needed a strict=False, that would be fine, though I think due to the way the GC works in PyPy this will always be failed. And PyPy is dead now so we don't need to worry about that changing in the future.

Copy link
Contributor Author

@yamatveev yamatveev Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, done, strict=False

def test_dynamic_attr_dealloc_frees_dict_contents():
"""Regression: py::dynamic_attr() objects must free __dict__ contents on dealloc.

pybind11_object_dealloc() did not call PyObject_ClearManagedDict() before tp_free(),
causing objects stored in __dict__ to have their refcounts permanently abandoned on
Python 3.14+ (where tp_free no longer implicitly clears the managed dict).
This caused capsule destructors to never run, leaking the underlying C++ data.
"""
instance = m.make_dynamic_attr_with_capsule()
assert not m.is_dynamic_attr_capsule_freed()
del instance
pytest.gc_collect()
assert m.is_dynamic_attr_capsule_freed()


def test_bad_arg_default(msg):
from pybind11_tests import detailed_error_messages_enabled

Expand Down
Loading