demonstrate premature delete using current #2687

include/pybind11/cast.h

@@ -586,25 +586,11 @@ public:
         return inst.release();
     }
 
-    // Base methods for generic caster; there are overridden in copyable_holder_caster
+    // Base methods for generic caster; they are overridden in copyable_holder_caster
     void load_value(value_and_holder &&v_h) {
-        auto *&vptr = v_h.value_ptr();
-        // Lazy allocation for unallocated values:
-        if (vptr == nullptr) {
-            auto *type = v_h.type ? v_h.type : typeinfo;
-            if (type->operator_new) {
-                vptr = type->operator_new(type->type_size);
-            } else {
-                #if defined(__cpp_aligned_new) && (!defined(_MSC_VER) || _MSC_VER >= 1912)
-                    if (type->type_align > __STDCPP_DEFAULT_NEW_ALIGNMENT__)
-                        vptr = ::operator new(type->type_size,
-                                              std::align_val_t(type->type_align));
-                    else
-                #endif
-                vptr = ::operator new(type->type_size);
-            }
-        }
-        value = vptr;
+        value = v_h.value_ptr();
+        if (value == nullptr)
+            throw cast_error("Invalid object instance");
     }
     bool try_implicit_casts(handle src, bool convert) {
         for (auto &cast : typeinfo->implicit_casts) {
@@ -1568,15 +1554,81 @@ template <typename T>
 class type_caster<std::shared_ptr<T>> : public copyable_holder_caster<T, std::shared_ptr<T>> { };
 
 template <typename type, typename holder_type>
-struct move_only_holder_caster {
-    static_assert(std::is_base_of<type_caster_base<type>, type_caster<type>>::value,
+struct move_only_holder_caster : public type_caster_base<type> {
+public:
+    using base = type_caster_base<type>;
+    static_assert(std::is_base_of<base, type_caster<type>>::value,
             "Holder classes are only supported for custom types");
+    using base::base;
+    using base::cast;
+    using base::typeinfo;
+    using base::value;
+
+    bool load(handle& src, bool convert) {
+        bool success = base::template load_impl<move_only_holder_caster<type, holder_type>>(src, convert);
+        if (success) // On success, remember src pointer to withdraw later
+            this->v_h = reinterpret_cast<instance *>(src.ptr())->get_value_and_holder();
+        return success;
+    }
 
-    static handle cast(holder_type &&src, return_value_policy, handle) {
-        auto *ptr = holder_helper<holder_type>::get(src);
-        return type_caster_base<type>::cast_holder(ptr, std::addressof(src));
+    template <typename T> using cast_op_type = detail::movable_cast_op_type<T>;
+
+    // Workaround for Intel compiler bug
+    // see pybind11 issue 94
+    #if !defined(__ICC) && !defined(__INTEL_COMPILER)
+    explicit
+    #endif
+    operator holder_type&&() {
+        // In load_value() value_ptr was still valid. If it's invalid now, another argument consumed the same value before.
+        if (!v_h.value_ptr())
+            throw cast_error("Multiple access to moved argument");
+        v_h.value_ptr() = nullptr;
+        // TODO: release object instance in python
+        // clear_instance(src_handle->ptr()); ???
+
+        return std::move(*holder_ptr);
+    }
+
+    static handle cast(const holder_type &src, return_value_policy, handle) {
+        const auto *ptr = holder_helper<holder_type>::get(src);
+        return type_caster_base<type>::cast_holder(ptr, &src);
+    }
+
+protected:
+    friend class type_caster_generic;
+    void check_holder_compat() {
+//        if (typeinfo->default_holder)
+//            throw cast_error("Unable to load a custom holder type from a default-holder instance");
     }
-    static constexpr auto name = type_caster_base<type>::name;
+
+    bool load_value(value_and_holder &&v_h) {
+        if (v_h.holder_constructed()) {
+            holder_ptr = std::addressof(v_h.template holder<holder_type>());
+            value = const_cast<void*>(reinterpret_cast<const void*>(holder_helper<holder_type>::get(*holder_ptr)));
+            if (!value)
+                throw cast_error("Invalid object instance");
+            return true;
+        } else {
+            throw cast_error("Unable to cast from non-held to held instance (T& to Holder<T>) "
+#if defined(NDEBUG)
+                             "(compile in debug mode for type information)");
+#else
+                             "of type '" + type_id<holder_type>() + "''");
+#endif
+        }
+    }
+
+    template <typename T = holder_type, detail::enable_if_t<!std::is_constructible<T, const T &, type*>::value, int> = 0>
+    bool try_implicit_casts(handle, bool) { return false; }
+
+    template <typename T = holder_type, detail::enable_if_t<std::is_constructible<T, const T &, type*>::value, int> = 0>
+    bool try_implicit_casts(handle, bool) { return false; }
+
+    static bool try_direct_conversions(handle) { return false; }
+
+
+    holder_type* holder_ptr = nullptr;
+    value_and_holder v_h;
 };
 
 template <typename type, typename deleter>
@@ -2005,6 +2057,8 @@ private:
         if ((... || !std::get<Is>(argcasters).load(call.args[Is], call.args_convert[Is])))
             return false;
 #else
+        // BUG: When loading the arguments, the actual argument type (pointer, lvalue reference, rvalue reference)
+        // is already lost (argcasters only know the intrinsic type), while the behaviour should differ for them, e.g. for rvalue references.
         for (bool r : {std::get<Is>(argcasters).load(call.args[Is], call.args_convert[Is])...})
             if (!r)
                 return false;

tests/test_premature_delete.cpp

+#include "pybind11_tests.h"
+
+#include <iostream>
+#include <memory>
+
+namespace pybind11_tests {
+
+inline void to_cout(std::string text) { std::cout << text << std::endl; }
+
+class pointee { // NOT copyable.
+  public:
+    pointee() = default;
+
+    int get_int() const {
+        to_cout("pointee::get_int()");
+        return 213;
+    }
+
+    ~pointee() { to_cout("~pointee()"); }
+
+  private:
+    pointee(const pointee &) = delete;
+    pointee(pointee &&) = delete;
+    pointee &operator=(const pointee &) = delete;
+    pointee &operator=(pointee &&) = delete;
+};
+
+inline std::unique_ptr<pointee> make_unique_pointee() {
+    return std::unique_ptr<pointee>(new pointee);
+}
+
+class ptr_owner {
+  public:
+    explicit ptr_owner(std::unique_ptr<pointee> ptr) : ptr_(std::move(ptr)) {}
+
+    bool is_owner() const { return bool(ptr_); }
+
+    std::unique_ptr<pointee> give_up_ownership_via_unique_ptr() {
+        return std::move(ptr_);
+    }
+    std::shared_ptr<pointee> give_up_ownership_via_shared_ptr() {
+        return std::move(ptr_);
+    }
+
+  private:
+    std::unique_ptr<pointee> ptr_;
+};
+
+TEST_SUBMODULE(premature_delete, m) {
+    m.def("to_cout", to_cout);
+
+    py::class_<pointee>(m, "pointee")
+        .def(py::init<>())
+        .def("get_int", &pointee::get_int);
+
+    m.def("make_unique_pointee", make_unique_pointee);
+
+    py::class_<ptr_owner>(m, "ptr_owner")
+        .def(py::init<std::unique_ptr<pointee>>(), py::arg("ptr"))
+        .def("is_owner", &ptr_owner::is_owner)
+        .def("give_up_ownership_via_unique_ptr",
+             &ptr_owner::give_up_ownership_via_unique_ptr)
+        .def("give_up_ownership_via_shared_ptr",
+             &ptr_owner::give_up_ownership_via_shared_ptr);
+}
+
+} // namespace pybind11_tests

tests/test_premature_delete.py

+# -*- coding: utf-8 -*-
+import pytest
+
+from pybind11_tests import premature_delete as m
+
+
+def test_pointee_and_ptr_owner():
+    m.to_cout("")
+    obj = m.pointee()
+    assert obj.get_int() == 213
+    owner = m.ptr_owner(obj)
+    with pytest.raises(RuntimeError) as exc_info:
+        obj.get_int()
+    assert str(exc_info.value) == "Invalid object instance"
+    assert owner.is_owner()
+    m.to_cout("before give up")
+    if 1:  # Behavior is the same with 0 or 1 here.
+        reclaimed = owner.give_up_ownership_via_unique_ptr()
+    else:
+        reclaimed = owner.give_up_ownership_via_shared_ptr()
+    m.to_cout("after give up")
+    assert not owner.is_owner()
+    if 0:
+        # This is desired.
+        assert reclaimed.get_int() == 213
+    else:
+        # But obj is actually disowned.
+        with pytest.raises(RuntimeError) as exc_info:
+            obj.get_int()
+        assert str(exc_info.value) == "Invalid object instance"
+    m.to_cout("before del")
+    del reclaimed
+    m.to_cout("after del")
+    m.to_cout("")