Don't construct unique_ptr around unowned pointers

include/pybind11/common.h

@@ -299,7 +299,7 @@ template <typename type> 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

include/pybind11/pybind11.h

@@ -821,7 +821,7 @@ class generic_type : public object {
         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 @@ class class_ : public detail::generic_type {
         } 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 @@ class class_ : public detail::generic_type {
             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 <typename T = holder_type,
               detail::enable_if_t<!std::is_copy_constructible<T>::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<void> *) inst);
     }
 

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<SpecialClass>, py::return_value_policy::reference_internal)
+    m.def("get_special_cstats", &ConstructorStats::get<SpecialClass>, 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.

tests/test_issues.cpp

@@ -48,6 +48,24 @@ class Dupe2 {};
 class Dupe3 {};
 class DupeException : public std::runtime_error {};
 
+// #478
+template <typename T> 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<T> &&move) : _ptr{move._ptr} { move._ptr = nullptr; print_move_created(this); }
+    custom_unique_ptr &operator=(custom_unique_ptr<T> &&move) { print_move_assigned(this); if (_ptr) destruct_ptr(); _ptr = move._ptr; move._ptr = nullptr; return *this; }
+    custom_unique_ptr(const custom_unique_ptr<T> &) = delete;
+    void operator=(const custom_unique_ptr<T> &copy) = 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<T>);
+
+
+
 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_<SharedParent, std::shared_ptr<SharedParent>>(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_<SpecialHolderObj, custom_unique_ptr<SpecialHolderObj>>(m, "SpecialHolderObj")
+        .def(py::init<int>())
+        .def("child", &SpecialHolderObj::child, pybind11::return_value_policy::reference_internal)
+        .def_readwrite("val", &SpecialHolderObj::val)
+        .def_static("holder_cstats", &ConstructorStats::get<custom_unique_ptr<SpecialHolderObj>>,
+                py::return_value_policy::reference)
+        ;
 };
 
 

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