diff --git a/include/pybind11/common.h b/include/pybind11/common.h index e684b187e..c422081e1 100644 --- a/include/pybind11/common.h +++ b/include/pybind11/common.h @@ -311,7 +311,7 @@ template struct instance_essentials { type *value; PyObject *weakrefs; bool owned : 1; - bool constructed : 1; + bool holder_constructed : 1; }; /// PyObject wrapper around generic types, includes a special holder type that is responsible for lifetime management diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index d845d18a7..93f55ef96 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -821,7 +821,7 @@ protected: auto tinfo = detail::get_type_info(type); self->value = ::operator new(tinfo->type_size); self->owned = true; - self->constructed = false; + self->holder_constructed = false; detail::get_internals().registered_instances.emplace(self->value, (PyObject *) self); return (PyObject *) self; } @@ -1134,7 +1134,7 @@ private: } catch (const std::bad_weak_ptr &) { new (&inst->holder) holder_type(inst->value); } - inst->owned = true; + inst->holder_constructed = true; } /// Initialize holder object, variant 2: try to construct from existing holder object, if possible @@ -1145,31 +1145,32 @@ private: new (&inst->holder) holder_type(*holder_ptr); else new (&inst->holder) holder_type(inst->value); - inst->owned = true; + inst->holder_constructed = true; } /// Initialize holder object, variant 3: holder is not copy constructible (e.g. unique_ptr), always initialize from raw pointer template ::value, int> = 0> static void init_holder_helper(instance_type *inst, const holder_type * /* unused */, const void * /* dummy */) { - new (&inst->holder) holder_type(inst->value); + if (inst->owned) { + new (&inst->holder) holder_type(inst->value); + inst->holder_constructed = true; + } } /// Initialize holder object of an instance, possibly given a pointer to an existing holder static void init_holder(PyObject *inst_, const void *holder_ptr) { auto inst = (instance_type *) inst_; init_holder_helper(inst, (const holder_type *) holder_ptr, inst->value); - inst->constructed = true; } static void dealloc(PyObject *inst_) { instance_type *inst = (instance_type *) inst_; - if (inst->owned) { - if (inst->constructed) - inst->holder.~holder_type(); - else - ::operator delete(inst->value); - } + if (inst->holder_constructed) + inst->holder.~holder_type(); + else if (inst->owned) + ::operator delete(inst->value); + generic_type::dealloc((detail::instance *) inst); } diff --git a/tests/constructor_stats.h b/tests/constructor_stats.h index 5dd215f19..eb3e49cab 100644 --- a/tests/constructor_stats.h +++ b/tests/constructor_stats.h @@ -56,7 +56,7 @@ from the ConstructorStats instance `.values()` method. In some cases, when you need to track instances of a C++ class not registered with pybind11, you need to add a function returning the ConstructorStats for the C++ class; this can be done with: - m.def("get_special_cstats", &ConstructorStats::get, py::return_value_policy::reference_internal) + m.def("get_special_cstats", &ConstructorStats::get, py::return_value_policy::reference) Finally, you can suppress the output messages, but keep the constructor tracking (for inspection/testing in python) by using the functions with `print_` replaced with `track_` (e.g. diff --git a/tests/test_issues.cpp b/tests/test_issues.cpp index 362ad448b..1374afeb7 100644 --- a/tests/test_issues.cpp +++ b/tests/test_issues.cpp @@ -48,6 +48,24 @@ class Dupe2 {}; class Dupe3 {}; class DupeException : public std::runtime_error {}; +// #478 +template class custom_unique_ptr { +public: + custom_unique_ptr() { print_default_created(this); } + custom_unique_ptr(T *ptr) : _ptr{ptr} { print_created(this, ptr); } + custom_unique_ptr(custom_unique_ptr &&move) : _ptr{move._ptr} { move._ptr = nullptr; print_move_created(this); } + custom_unique_ptr &operator=(custom_unique_ptr &&move) { print_move_assigned(this); if (_ptr) destruct_ptr(); _ptr = move._ptr; move._ptr = nullptr; return *this; } + custom_unique_ptr(const custom_unique_ptr &) = delete; + void operator=(const custom_unique_ptr ©) = delete; + ~custom_unique_ptr() { print_destroyed(this); if (_ptr) destruct_ptr(); } +private: + T *_ptr = nullptr; + void destruct_ptr() { delete _ptr; } +}; +PYBIND11_DECLARE_HOLDER_TYPE(T, custom_unique_ptr); + + + void init_issues(py::module &m) { py::module m2 = m.def_submodule("issues"); @@ -311,6 +329,25 @@ void init_issues(py::module &m) { py::class_>(m, "SharedParent") .def(py::init<>()) .def("get_child", &SharedParent::get_child, py::return_value_policy::reference); + + /// Issue/PR #478: unique ptrs constructed and freed without destruction + class SpecialHolderObj { + public: + int val = 0; + SpecialHolderObj *ch = nullptr; + SpecialHolderObj(int v, bool make_child = true) : val{v}, ch{make_child ? new SpecialHolderObj(val+1, false) : nullptr} + { print_created(this, val); } + ~SpecialHolderObj() { delete ch; print_destroyed(this); } + SpecialHolderObj *child() { return ch; } + }; + + py::class_>(m, "SpecialHolderObj") + .def(py::init()) + .def("child", &SpecialHolderObj::child, pybind11::return_value_policy::reference_internal) + .def_readwrite("val", &SpecialHolderObj::val) + .def_static("holder_cstats", &ConstructorStats::get>, + py::return_value_policy::reference) + ; }; diff --git a/tests/test_issues.py b/tests/test_issues.py index 208cf3b41..6f84f777b 100644 --- a/tests/test_issues.py +++ b/tests/test_issues.py @@ -201,3 +201,20 @@ def test_enable_shared_from_this_with_reference_rvp(): assert cstats.alive() == 1 del child, parent assert cstats.alive() == 0 + +def test_non_destructed_holders(): + """ Issue #478: unique ptrs constructed and freed without destruction """ + from pybind11_tests import SpecialHolderObj + + a = SpecialHolderObj(123) + b = a.child() + + assert a.val == 123 + assert b.val == 124 + + cstats = SpecialHolderObj.holder_cstats() + assert cstats.alive() == 1 + del b + assert cstats.alive() == 1 + del a + assert cstats.alive() == 0