From 2447f72d055e668b24a9503fa0875a193be9e928 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Sun, 18 Oct 2026 13:05:52 +0000 Subject: [PATCH] [cpyrt] Unregister proxies whose C++ object was replaced through T** When a C++ function takes a proxied object as `T**` (or `void**`), the converter passes the address of the proxy's own pointer slot, so the function can delete the object and store a new one in the proxy, as `CppjitTestData::set_pod_ptrptr_out` does in test_datatypes. The memory regulator still tracks the proxy under the old address, and when the proxy dies, `UnregisterPyObject` looks it up under the new one. That erases nothing, or erases the entry of an unrelated proxy that is tracked under the new address, and leaves the old entry pointing at the dead proxy. Once the allocator hands the old address to a new object of the same class, `RegisterPyObject` finds the dangling entry and clears `kIsRegulated` in the `fFlags` of the dead proxy, i.e. it clears bit 0x800 somewhere in whatever object reused that memory. This was the sporadic crash of test_datatypes on the mac-beta runner of ROOT CI (root-project/root#23740): the constructor call in test21 got the address of the pod that test16 had replaced, and the write landed in the item array of pytest's list of passed reports, which the garbage collector later traversed. Only macOS's allocator reused the address at that point. `UnregisterPyObject` now erases an entry only if it belongs to the proxy, and searches the map for the proxy otherwise, which also covers proxies whose held pointer was nulled. The search runs only when a registered proxy is not found under the address it holds now. --- src/cpyrt/MemoryRegulator.cxx | 28 +++++++++++++++++----- test/test_regression.py | 44 +++++++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 6 deletions(-) diff --git a/src/cpyrt/MemoryRegulator.cxx b/src/cpyrt/MemoryRegulator.cxx index a7a06c63..bc4d376b 100644 --- a/src/cpyrt/MemoryRegulator.cxx +++ b/src/cpyrt/MemoryRegulator.cxx @@ -8,6 +8,7 @@ using namespace cppjit; #include "ProxyWrappers.h" // Standard +#include #include #include #include @@ -199,10 +200,8 @@ bool cpyrt::MemoryRegulator::UnregisterPyObject(CPPInstance* pyobj, interop::TCppObject_t cppobj = pyobj->IsSmart() ? pyobj->GetObjectRaw() : pyobj->GetObject(); - if (!cppobj) - return false; - if (unregisterHook) { + if (cppobj && unregisterHook) { auto res = unregisterHook(cppobj, ((CPPClass*)pyclass)->fCppType); if (!res.second) return res.first; @@ -212,9 +211,26 @@ bool cpyrt::MemoryRegulator::UnregisterPyObject(CPPInstance* pyobj, if (!cppobjs) return false; - // erase if tracked - if (cppobjs->erase(cppobj)) { - pyobj->fFlags &= ~CPPInstance::kIsRegulated; + // The proxy is normally tracked under the address it holds now. That + // address can have changed since registration, though: a function taking + // the object as T** (or void**) may delete it and write a new object into + // the proxy, and cross-inheritance can null the held pointer. Only erase an + // entry that belongs to this proxy, because another proxy may be tracked + // under the new address by now, and otherwise search the map, because an + // entry left behind under the old address would dangle once this proxy is + // gone: a later C++ object at that address would be matched with the dead + // proxy, and re-registering the address writes into its freed memory. + auto ppo = cppobj ? cppobjs->find(cppobj) : cppobjs->end(); + if (ppo == cppobjs->end() || ppo->second != (PyObject*)pyobj) { + ppo = std::find_if(cppobjs->begin(), cppobjs->end(), + [pyobj](const CppToPyMap_t::value_type& entry) { + return entry.second == (PyObject*)pyobj; + }); + } + + pyobj->fFlags &= ~CPPInstance::kIsRegulated; + if (ppo != cppobjs->end()) { + cppobjs->erase(ppo); return true; } diff --git a/test/test_regression.py b/test/test_regression.py index 2a2bda4a..369ecbdf 100644 --- a/test/test_regression.py +++ b/test/test_regression.py @@ -1727,3 +1727,47 @@ def test56_enum_arg_typedefed_underlying_type(self): assert ns.pick(ns.S.A) == 1 assert ns.pick(ns.S.B) == 5 + + def test57_regulator_follows_replaced_object(self): + """A proxy whose object a T** argument replaced unregisters cleanly""" + + import gc + + import cppjit + + cppjit.cppdef("""\ + namespace RegulatorReplaced { + struct Pod { int m_int; }; + // allocate the replacement first, so its address always differs + void replace(Pod** pp) { + Pod* fresh = new Pod(**pp); + delete *pp; + *pp = fresh; + } + }""") + + ns = cppjit.gbl.RegulatorReplaced + + p = ns.Pod() + old = cppjit.addressof(p) + ns.replace(p) + new = cppjit.addressof(p) + assert new != old + + # a second proxy for the object at the new address + q = cppjit.bind_object(new, ns.Pod) + assert q is not p + + # p must not take q's entry along when it dies (the C++ object stays + # alive for q) + p.__python_owns__ = False + del p + gc.collect() + assert cppjit.bind_object(new, ns.Pod) is q + + # nor leave its own entry behind under the old address, where a later + # object at that address would be matched with the dead proxy (binding + # does not dereference the address) + r = cppjit.bind_object(old, ns.Pod) + assert cppjit.addressof(r) == old + assert r is not q