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