Fixing bugs discovered by ASAN. The code is now ASAN, MSAN, UBSAN clean.

include/pybind11/classh.h

@@ -266,37 +266,37 @@ public:
     }
 
 private:
-    /// Initialize holder object, variant 1: object derives from enable_shared_from_this
     template <typename T>
-    static void init_holder(detail::value_and_holder &/*v_h*/,
-            const holder_type * /* unused */, const std::enable_shared_from_this<T> * /* dummy */) {
+    static void init_holder(bool /*owned*/, detail::value_and_holder &/*v_h*/,
+            holder_type * /* unused */, const std::enable_shared_from_this<T> * /* dummy */) {
         throw std::runtime_error("Not implemented: classh::init_holder enable_shared_from_this.");
     }
 
-    /// Initialize holder object, variant 2: try to construct from existing holder object, if possible
-    static void init_holder(detail::value_and_holder &v_h,
-            const holder_type *holder_ptr, const void * /* dummy -- not enable_shared_from_this<T>) */) {
+    static void init_holder(bool owned, detail::value_and_holder &v_h,
+            holder_type *holder_ptr, const void * /* dummy -- not enable_shared_from_this<T>) */) {
         if (holder_ptr) {
-            new (std::addressof(v_h.holder<holder_type>())) holder_type(std::move(*holder_ptr));
-            v_h.set_holder_constructed();
-        } else { // Was: if (inst->owned || detail::always_construct_holder<holder_type>::value)
             new (std::addressof(v_h.holder<holder_type>())) holder_type(
-                std::move(holder_type::from_raw_ptr_take_ownership(v_h.value_ptr<type>())));
-            v_h.set_holder_constructed();
+                std::move(*holder_ptr));
+        } else if (owned) {
+            new (std::addressof(v_h.holder<holder_type>())) holder_type(
+                holder_type::from_raw_ptr_take_ownership(v_h.value_ptr<type>()));
+        }
+        else {
+            new (std::addressof(v_h.holder<holder_type>())) holder_type(
+                holder_type::from_raw_ptr_unowned(v_h.value_ptr<type>()));
         }
+        v_h.set_holder_constructed();
     }
 
-    /// Performs instance initialization including constructing a holder and registering the known
-    /// instance.  Should be called as soon as the `type` value_ptr is set for an instance.  Takes an
-    /// optional pointer to an existing holder to use; if not specified and the instance is
-    /// `.owned`, a new holder will be constructed to manage the value pointer.
     static void init_instance(detail::instance *inst, const void *holder_ptr) {
         auto v_h = inst->get_value_and_holder(detail::get_type_info(typeid(type)));
         if (!v_h.instance_registered()) {
             register_instance(inst, v_h.value_ptr(), v_h.type);
             v_h.set_instance_registered();
         }
-        init_holder(v_h, static_cast<const holder_type *>(holder_ptr), v_h.value_ptr<type>());
+        // Need for const_cast is a consequence of the type_info::init_instance type:
+        // void (*init_instance)(instance *, const void *);
+        init_holder(inst->owned, v_h, static_cast<holder_type *>(const_cast<void *>(holder_ptr)), v_h.value_ptr<type>());
     }
 
     /// Deallocates an instance; via holder, if constructed; otherwise via operator delete.

tests/test_classh_wip.cpp

@@ -15,11 +15,11 @@ struct mpty {
 // clang-format off
 
 mpty        rtrn_mpty_valu() { mpty obj{"rtrn_valu"}; return obj; }
-mpty&&      rtrn_mpty_rref() { mpty obj{"rtrn_rref"}; return std::move(obj); }
+mpty&&      rtrn_mpty_rref() { static mpty obj; obj.mtxt = "rtrn_rref"; return std::move(obj); }
 mpty const& rtrn_mpty_cref() { static mpty obj; obj.mtxt = "rtrn_cref"; return obj; }
 mpty&       rtrn_mpty_mref() { static mpty obj; obj.mtxt = "rtrn_mref"; return obj; }
-mpty const* rtrn_mpty_cptr() { static mpty obj; obj.mtxt = "rtrn_cptr"; return &obj; }
-mpty*       rtrn_mpty_mptr() { static mpty obj; obj.mtxt = "rtrn_mptr"; return &obj; }
+mpty const* rtrn_mpty_cptr() { return new mpty{"rtrn_cptr"}; }
+mpty*       rtrn_mpty_mptr() { return new mpty{"rtrn_mptr"}; }
 
 std::string pass_mpty_valu(mpty obj)        { return "pass_valu:" + obj.mtxt; }
 std::string pass_mpty_rref(mpty&& obj)      { return "pass_rref:" + obj.mtxt; }
@@ -315,7 +315,9 @@ struct type_caster<std::shared_ptr<mpty>> : smart_holder_type_caster_load<mpty>
 
         object inst            = reinterpret_steal<object>(make_new_instance(tinfo->type));
         instance *inst_raw_ptr = reinterpret_cast<instance *>(inst.ptr());
-        inst_raw_ptr->owned    = false; // Not actually used.
+        inst_raw_ptr->owned    = true;
+        void *&valueptr        = values_and_holders(inst_raw_ptr).begin()->value_ptr();
+        valueptr               = src_raw_void_ptr;
 
         auto smhldr = pybindit::memory::smart_holder::from_shared_ptr(src);
         tinfo->init_instance(inst_raw_ptr, static_cast<const void *>(&smhldr));
@@ -380,7 +382,9 @@ struct type_caster<std::unique_ptr<mpty>> : smart_holder_type_caster_load<mpty>
 
         object inst            = reinterpret_steal<object>(make_new_instance(tinfo->type));
         instance *inst_raw_ptr = reinterpret_cast<instance *>(inst.ptr());
-        inst_raw_ptr->owned    = false; // Not actually used.
+        inst_raw_ptr->owned    = true;
+        void *&valueptr        = values_and_holders(inst_raw_ptr).begin()->value_ptr();
+        valueptr               = src_raw_void_ptr;
 
         auto smhldr = pybindit::memory::smart_holder::from_unique_ptr(std::move(src));
         tinfo->init_instance(inst_raw_ptr, static_cast<const void *>(&smhldr));

tests/test_classh_wip.py

@@ -15,20 +15,13 @@ def test_mpty_constructors():
 
 def test_cast():
     assert m.get_mtxt(m.rtrn_mpty_valu()) == "rtrn_valu"
-    #                                         rtrn_rref exercised separately.
+    assert m.get_mtxt(m.rtrn_mpty_rref()) == "rtrn_rref"
     assert m.get_mtxt(m.rtrn_mpty_cref()) == "rtrn_cref"
     assert m.get_mtxt(m.rtrn_mpty_mref()) == "rtrn_mref"
     assert m.get_mtxt(m.rtrn_mpty_cptr()) == "rtrn_cptr"
     assert m.get_mtxt(m.rtrn_mpty_mptr()) == "rtrn_mptr"
 
 
-def test_cast_rref():
-    e = m.rtrn_mpty_rref()
-    assert e.__class__.__name__ == "mpty"
-    with pytest.raises(RuntimeError):
-        m.get_mtxt(e)  # E.g. basic_string::_M_construct null not valid
-
-
 def test_load():
     assert m.pass_mpty_valu(m.mpty("Valu")) == "pass_valu:Valu"
     assert m.pass_mpty_rref(m.mpty("Rref")) == "pass_rref:Rref"