Make `init_holder` do registration, and rename to `init_instance`

The instance registration for offset base types fails (under macOS, with
a segfault) in the presense of virtual base types.  The issue occurs
when trying to `static_cast<Base *>(derived_ptr)` when `derived_ptr` has
been allocated (via `operator new`) but not initialized.

This commit fixes the issue by moving the addition to
`registered_instances` into `init_holder` rather than immediately after
value pointer allocation.

This also renames it to `init_instance` since it does more than holder
initialization now.  (I also further renamed `init_holder_helper` to
`init_holder` since `init_holder` isn't used anymore).

Fixes #959.

include/pybind11/attr.h

@@ -216,8 +216,8 @@ struct type_record {
     /// The global operator new can be overridden with a class-specific variant
     void *(*operator_new)(size_t) = ::operator new;
 
-    /// Function pointer to class_<..>::init_holder
-    void (*init_holder)(instance *, const void *) = nullptr;
+    /// Function pointer to class_<..>::init_instance
+    void (*init_instance)(instance *, const void *) = nullptr;
 
     /// Function pointer to class_<..>::dealloc
     void (*dealloc)(const detail::value_and_holder &) = nullptr;

include/pybind11/cast.h

@@ -44,7 +44,7 @@ struct type_info {
     const std::type_info *cpptype;
     size_t type_size, holder_size_in_ptrs;
     void *(*operator_new)(size_t);
-    void (*init_holder)(instance *, const void *);
+    void (*init_instance)(instance *, const void *);
     void (*dealloc)(const value_and_holder &v_h);
     std::vector<PyObject *(*)(PyObject *, PyTypeObject *)> implicit_conversions;
     std::vector<std::pair<const std::type_info *, void *(*)(void *)>> implicit_casts;
@@ -515,7 +515,6 @@ inline PyThreadState *get_thread_state_unchecked() {
 
 // Forward declarations
 inline void keep_alive_impl(handle nurse, handle patient);
-inline void register_instance(instance *self, void *valptr, const type_info *tinfo);
 inline PyObject *make_new_instance(PyTypeObject *type, bool allocate_value = true);
 
 class type_caster_generic {
@@ -595,8 +594,7 @@ public:
                 throw cast_error("unhandled return_value_policy: should not happen!");
         }
 
-        register_instance(wrapper, valueptr, tinfo);
-        tinfo->init_holder(wrapper, existing_holder);
+        tinfo->init_instance(wrapper, existing_holder);
 
         return inst.release();
     }

include/pybind11/class_support.h

@@ -234,9 +234,8 @@ inline bool deregister_instance(instance *self, void *valptr, const type_info *t
 
 /// Instance creation function for all pybind11 types. It only allocates space for the C++ object
 /// (or multiple objects, for Python-side inheritance from multiple pybind11 types), but doesn't
-/// call the constructor -- an `__init__` function must do that.  If allocating value, the instance
-/// is registered; otherwise register_instance will need to be called once the value has been
-/// assigned.
+/// call the constructor -- an `__init__` function must do that.  `register_instance` will need to
+/// be called after the object has been initialized.
 inline PyObject *make_new_instance(PyTypeObject *type, bool allocate_value /*= true (in cast.h)*/) {
 #if defined(PYPY_VERSION)
     // PyPy gets tp_basicsize wrong (issue 2482) under multiple inheritance when the first inherited
@@ -257,7 +256,6 @@ inline PyObject *make_new_instance(PyTypeObject *type, bool allocate_value /*= t
         for (auto &v_h : values_and_holders(inst)) {
             void *&vptr = v_h.value_ptr();
             vptr = v_h.type->operator_new(v_h.type->type_size);
-            register_instance(inst, vptr, v_h.type);
         }
     }
 
@@ -316,11 +314,12 @@ inline void clear_instance(PyObject *self) {
     // Deallocate any values/holders, if present:
     for (auto &v_h : values_and_holders(instance)) {
         if (v_h) {
+            // This is allowed to fail (the type might have been allocated but not
+            // initialized/registered):
+            deregister_instance(instance, v_h.value_ptr(), v_h.type);
+
             if (instance->owned || v_h.holder_constructed())
                 v_h.type->dealloc(v_h);
-
-            if (!deregister_instance(instance, v_h.value_ptr(), v_h.type))
-                pybind11_fail("pybind11_object_dealloc(): Tried to deallocate unregistered instance!");
         }
     }
     // Deallocate the value/holder layout internals:

include/pybind11/pybind11.h

@@ -714,7 +714,7 @@ protected:
         } else {
             if (overloads->is_constructor) {
                 auto tinfo = get_type_info((PyTypeObject *) overloads->scope.ptr());
-                tinfo->init_holder(reinterpret_cast<instance *>(parent.ptr()), nullptr);
+                tinfo->init_instance(reinterpret_cast<instance *>(parent.ptr()), nullptr);
             }
             return result.ptr();
         }
@@ -835,7 +835,7 @@ protected:
         tinfo->type_size = rec.type_size;
         tinfo->operator_new = rec.operator_new;
         tinfo->holder_size_in_ptrs = size_in_ptrs(rec.holder_size);
-        tinfo->init_holder = rec.init_holder;
+        tinfo->init_instance = rec.init_instance;
         tinfo->dealloc = rec.dealloc;
         tinfo->simple_type = true;
         tinfo->simple_ancestors = true;
@@ -971,7 +971,7 @@ public:
         record.type = &typeid(type);
         record.type_size = sizeof(conditional_t<has_alias, type_alias, type>);
         record.holder_size = sizeof(holder_type);
-        record.init_holder = init_holder;
+        record.init_instance = init_instance;
         record.dealloc = dealloc;
         record.default_holder = std::is_same<holder_type, std::unique_ptr<type>>::value;
 
@@ -1170,7 +1170,7 @@ public:
 private:
     /// Initialize holder object, variant 1: object derives from enable_shared_from_this
     template <typename T>
-    static void init_holder_helper(detail::instance *inst, detail::value_and_holder &v_h,
+    static void init_holder(detail::instance *inst, detail::value_and_holder &v_h,
             const holder_type * /* unused */, const std::enable_shared_from_this<T> * /* dummy */) {
         try {
             auto sh = std::dynamic_pointer_cast<typename holder_type::element_type>(
@@ -1198,7 +1198,7 @@ private:
     }
 
     /// Initialize holder object, variant 2: try to construct from existing holder object, if possible
-    static void init_holder_helper(detail::instance *inst, detail::value_and_holder &v_h,
+    static void init_holder(detail::instance *inst, detail::value_and_holder &v_h,
             const holder_type *holder_ptr, const void * /* dummy -- not enable_shared_from_this<T>) */) {
         if (holder_ptr) {
             init_holder_from_existing(v_h, holder_ptr, std::is_copy_constructible<holder_type>());
@@ -1209,10 +1209,14 @@ private:
         }
     }
 
-    /// Initialize holder object of an instance, possibly given a pointer to an existing holder
-    static void init_holder(detail::instance *inst, const void *holder_ptr) {
+    /// 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)));
-        init_holder_helper(inst, v_h, (const holder_type *) holder_ptr, v_h.value_ptr<type>());
+        register_instance(inst, v_h.value_ptr(), v_h.type);
+        init_holder(inst, v_h, (const holder_type *) holder_ptr, v_h.value_ptr<type>());
     }
 
     /// Deallocates an instance; via holder, if constructed; otherwise via operator delete.

tests/test_call_policies.py

@@ -116,7 +116,9 @@ def test_alive_gc_multi_derived(capture):
     from pybind11_tests import Parent, Child, ConstructorStats
 
     class Derived(Parent, Child):
-        pass
+        def __init__(self):
+            Parent.__init__(self)
+            Child.__init__(self)
 
     n_inst = ConstructorStats.detail_reg_inst()
     p = Derived()
@@ -131,6 +133,7 @@ def test_alive_gc_multi_derived(capture):
     assert capture == """
         Releasing parent.
         Releasing child.
+        Releasing child.
     """
 
 

tests/test_multiple_inheritance.cpp

@@ -201,4 +201,20 @@ TEST_SUBMODULE(multiple_inheritance, m) {
     py::class_<VanillaDictMix1, Vanilla, WithDict>(m, "VanillaDictMix1").def(py::init<>());
     py::class_<VanillaDictMix2, WithDict, Vanilla>(m, "VanillaDictMix2").def(py::init<>());
 #endif
+
+    // test_diamond_inheritance
+    // Issue #959: segfault when constructing diamond inheritance instance
+    // All of these have int members so that there will be various unequal pointers involved.
+    struct B { int b; virtual ~B() = default; };
+    struct C0 : public virtual B { int c0; };
+    struct C1 : public virtual B { int c1; };
+    struct D : public C0, public C1 { int d; };
+    py::class_<B>(m, "B")
+        .def("b", [](B *self) { return self; });
+    py::class_<C0, B>(m, "C0")
+        .def("c0", [](C0 *self) { return self; });
+    py::class_<C1, B>(m, "C1")
+        .def("c1", [](C1 *self) { return self; });
+    py::class_<D, C0, C1>(m, "D")
+        .def(py::init<>());
 }

tests/test_multiple_inheritance.py

@@ -331,3 +331,18 @@ def test_mi_base_return():
     e2 = m.i801e_b2()
     assert type(e2) is m.I801B2
     assert e2.b == 2
+
+
+def test_diamond_inheritance():
+    """Tests that diamond inheritance works as expected (issue #959)"""
+
+    # Issue #959: this shouldn't segfault:
+    d = m.D()
+
+    # Make sure all the various distinct pointers are all recognized as registered instances:
+    assert d is d.c0()
+    assert d is d.c1()
+    assert d is d.b()
+    assert d is d.c0().b()
+    assert d is d.c1().b()
+    assert d is d.c0().c1().b().c0().b()