Detect and fail if using mismatched holders

include/pybind11/attr.h

@@ -221,7 +221,7 @@ struct function_record {
 struct type_record {
     PYBIND11_NOINLINE type_record()
         : multiple_inheritance(false), dynamic_attr(false), buffer_protocol(false),
-          default_holder(true), module_local(false), is_final(false) { }
+          module_local(false), is_final(false) { }
 
     /// Handle to the parent scope
     handle scope;
@@ -238,6 +238,9 @@ struct type_record {
     /// What is the alignment of the underlying C++ type?
     size_t type_align = 0;
 
+    // Pointer to RTTI type_info data structure of holder type
+    const std::type_info *holder_type = nullptr;
+
     /// How large is the type's holder?
     size_t holder_size = 0;
 
@@ -268,9 +271,6 @@ struct type_record {
     /// Does the class implement the buffer protocol?
     bool buffer_protocol : 1;
 
-    /// Is the default (unique_ptr) holder type used?
-    bool default_holder : 1;
-
     /// Is the class definition local to the module shared object?
     bool module_local : 1;
 
@@ -286,13 +286,24 @@ struct type_record {
                           "\" referenced unknown base type \"" + tname + "\"");
         }
 
-        if (default_holder != base_info->default_holder) {
-            std::string tname(base.name());
-            detail::clean_type_id(tname);
-            pybind11_fail("generic_type: type \"" + std::string(name) + "\" " +
-                    (default_holder ? "does not have" : "has") +
-                    " a non-default holder type while its base \"" + tname + "\" " +
-                    (base_info->default_holder ? "does not" : "does"));
+        // Check for holder compatibility
+        // We cannot simply check for same_type(*holder_type, *base_info->holder_type)
+        // as the typeids naturally differ as the base type differs from this type
+        auto clean_holder_name = [](const std::type_info* holder_type, const std::type_info* base_type) -> std::string {
+            std::string base_name(base_type->name());
+            detail::clean_type_id(base_name);
+            std::string holder_name(holder_type->name());
+            detail::clean_type_id(holder_name);
+            size_t start_pos = holder_name.find(base_name);
+            return holder_name.substr(0, start_pos-1);
+        };
+        std::string holder_name = clean_holder_name(holder_type, this->type);
+        std::string base_holder_name = clean_holder_name(base_info->holder_type, base_info->cpptype);
+        if (holder_name != base_holder_name) {
+            std::string base_name(base.name());
+            detail::clean_type_id(base_name);
+            pybind11_fail("generic_type: type \"" + std::string(name) +
+                    "\" uses different holder than its base \"" + base_name + "\" (" + base_holder_name + " vs " + holder_name + ")");
         }
 
         bases.append((PyObject *) base_info->type);

include/pybind11/cast.h

@@ -1521,10 +1521,7 @@ struct copyable_holder_caster : public type_caster_base<type> {
 
 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");
-    }
+    void check_holder_compat() {}
 
     bool load_value(value_and_holder &&v_h) {
         if (v_h.holder_constructed()) {
@@ -1607,6 +1604,39 @@ template <typename base, typename holder> struct is_holder_type :
 template <typename base, typename deleter> struct is_holder_type<base, std::unique_ptr<base, deleter>> :
     std::true_type {};
 
+template <typename holder> using is_holder = any_of<
+    is_template_base_of<move_only_holder_caster, make_caster<holder>>,
+    is_template_base_of<copyable_holder_caster, make_caster<holder>>>;
+
+template <typename holder>
+void check_for_holder_mismatch(const char*, enable_if_t<!is_holder<holder>::value, int> = 0) {}
+template <typename holder>
+void check_for_holder_mismatch(const char* func_name, enable_if_t<is_holder<holder>::value, int> = 0) {
+    using iholder = intrinsic_t<holder>;
+    using base_type = decltype(*holder_helper<iholder>::get(std::declval<iholder>()));
+    auto &holder_typeinfo = typeid(iholder);
+    auto base_info = detail::get_type_info(typeid(base_type), false);
+    if (!base_info) {
+#ifdef NDEBUG
+        pybind11_fail("Cannot register function using not yet registered type");
+#else
+        pybind11_fail("Cannot register function using not yet registered type '" + type_id<base_type>() + "'");
+#endif
+    }
+
+    if (!same_type(*base_info->holder_type, holder_typeinfo)) {
+#ifdef NDEBUG
+        pybind11_fail("Detected mismatching holder types when declaring function '" + std::string(func_name) + "'  (compile in debug mode for details)");
+#else
+        std::string holder_name(base_info->holder_type->name());
+        detail::clean_type_id(holder_name);
+        pybind11_fail("Detected mismatching holder types when declaring function '" + std::string(func_name) + "':"
+                " attempting to use holder type " + type_id<iholder>() + ", but " + type_id<base_type>() +
+                " was declared using holder type " + holder_name);
+#endif
+    }
+}
+
 template <typename T> struct handle_type_name { static constexpr auto name = _<T>(); };
 template <> struct handle_type_name<bytes> { static constexpr auto name = _(PYBIND11_BYTES_NAME); };
 template <> struct handle_type_name<int_> { static constexpr auto name = _("int"); };

include/pybind11/detail/internals.h

@@ -128,6 +128,7 @@ struct internals {
 struct type_info {
     PyTypeObject *type;
     const std::type_info *cpptype;
+    const std::type_info *holder_type = nullptr;
     size_t type_size, type_align, holder_size_in_ptrs;
     void *(*operator_new)(size_t);
     void (*init_instance)(instance *, const void *);
@@ -143,14 +144,12 @@ struct type_info {
     bool simple_type : 1;
     /* True if there is no multiple inheritance in this type's inheritance tree */
     bool simple_ancestors : 1;
-    /* for base vs derived holder_type checks */
-    bool default_holder : 1;
     /* true if this is a type registered with py::module_local */
     bool module_local : 1;
 };
 
 /// Tracks the `internals` and `type_info` ABI version independent of the main library version
-#define PYBIND11_INTERNALS_VERSION 4
+#define PYBIND11_INTERNALS_VERSION 5
 
 /// On MSVC, debug and release builds are not ABI-compatible!
 #if defined(_MSC_VER) && defined(_DEBUG)

include/pybind11/pybind11.h

@@ -157,6 +157,13 @@ class cpp_function : public function {
         static_assert(expected_num_args<Extra...>(sizeof...(Args), cast_in::has_args, cast_in::has_kwargs),
                       "The number of argument annotations does not match the number of function arguments");
 
+        /* Process any user-provided function attributes */
+        process_attributes<Extra...>::init(extra..., rec);
+
+        // Fail if the return or argument types were previously registered with a different holder type
+        detail::check_for_holder_mismatch<Return>(rec->name);
+        PYBIND11_EXPAND_SIDE_EFFECTS(detail::check_for_holder_mismatch<Args>(rec->name));
+
         /* Dispatch code which converts function arguments and performs the actual function call */
         rec->impl = [](function_call &call) -> handle {
             cast_in args_converter;
@@ -189,9 +196,6 @@ class cpp_function : public function {
             return result;
         };
 
-        /* Process any user-provided function attributes */
-        process_attributes<Extra...>::init(extra..., rec);
-
         {
             constexpr bool has_kw_only_args = any_of<std::is_same<kw_only, Extra>...>::value,
                            has_pos_only_args = any_of<std::is_same<pos_only, Extra>...>::value,
@@ -1042,6 +1046,7 @@ class generic_type : public object {
         auto *tinfo = new detail::type_info();
         tinfo->type = (PyTypeObject *) m_ptr;
         tinfo->cpptype = rec.type;
+        tinfo->holder_type = rec.holder_type;
         tinfo->type_size = rec.type_size;
         tinfo->type_align = rec.type_align;
         tinfo->operator_new = rec.operator_new;
@@ -1050,7 +1055,6 @@ class generic_type : public object {
         tinfo->dealloc = rec.dealloc;
         tinfo->simple_type = true;
         tinfo->simple_ancestors = true;
-        tinfo->default_holder = rec.default_holder;
         tinfo->module_local = rec.module_local;
 
         auto &internals = get_internals();
@@ -1227,10 +1231,10 @@ class class_ : public detail::generic_type {
         record.type = &typeid(type);
         record.type_size = sizeof(conditional_t<has_alias, type_alias, type>);
         record.type_align = alignof(conditional_t<has_alias, type_alias, type>&);
+        record.holder_type = &typeid(holder_type);
         record.holder_size = sizeof(holder_type);
         record.init_instance = init_instance;
         record.dealloc = dealloc;
-        record.default_holder = detail::is_instantiation<std::unique_ptr, holder_type>::value;
 
         set_operator_new<type>(&record);
 

tests/test_class.py

@@ -219,16 +219,16 @@ def test_mismatched_holder():
     with pytest.raises(RuntimeError) as excinfo:
         m.mismatched_holder_1()
     assert re.match(
-        'generic_type: type ".*MismatchDerived1" does not have a non-default '
-        'holder type while its base ".*MismatchBase1" does',
+        'generic_type: type ".*MismatchDerived1" uses different holder '
+        'than its base ".*MismatchBase1"',
         str(excinfo.value),
     )
 
     with pytest.raises(RuntimeError) as excinfo:
         m.mismatched_holder_2()
     assert re.match(
-        'generic_type: type ".*MismatchDerived2" has a non-default holder type '
-        'while its base ".*MismatchBase2" does not',
+        'generic_type: type ".*MismatchDerived2" uses different holder '
+        'than its base ".*MismatchBase2"',
         str(excinfo.value),
     )
 

tests/test_smart_ptr.cpp

@@ -37,11 +37,11 @@ PYBIND11_DECLARE_HOLDER_TYPE(T, std::shared_ptr<T>);
 // holder size to trigger the non-simple-layout internal instance layout for single inheritance with
 // large holder type:
 template <typename T> class huge_unique_ptr {
-    std::unique_ptr<T> ptr;
     uint64_t padding[10];
+    std::unique_ptr<T> ptr;
 public:
     huge_unique_ptr(T *p) : ptr(p) {};
-    T *get() { return ptr.get(); }
+    T *get() const { return ptr.get(); }
 };
 PYBIND11_DECLARE_HOLDER_TYPE(T, huge_unique_ptr<T>);
 
@@ -330,12 +330,6 @@ TEST_SUBMODULE(smart_ptr, m) {
         .def_readwrite("value", &TypeForMoveOnlyHolderWithAddressOf::value)
         .def("print_object", [](const TypeForMoveOnlyHolderWithAddressOf *obj) { py::print(obj->toString()); });
 
-    // test_smart_ptr_from_default
-    struct HeldByDefaultHolder { };
-    py::class_<HeldByDefaultHolder>(m, "HeldByDefaultHolder")
-        .def(py::init<>())
-        .def_static("load_shared_ptr", [](std::shared_ptr<HeldByDefaultHolder>) {});
-
     // test_shared_ptr_gc
     // #187: issue involving std::shared_ptr<> return value policy & garbage collection
     struct ElementBase {
@@ -367,4 +361,32 @@ TEST_SUBMODULE(smart_ptr, m) {
                 list.append(py::cast(e));
             return list;
         });
+
+    // test_holder_mismatch
+    // Tests the detection of trying to use mismatched holder types around the same instance type
+    struct HeldByShared {};
+    struct HeldByUnique {};
+    // HeldByShared declared with shared_ptr holder, but used with unique_ptr later
+    py::class_<HeldByShared, std::shared_ptr<HeldByShared>>(m, "HeldByShared");
+    m.def("register_mismatch_return", [](py::module m) {
+        // Fails: the class was already registered with a shared_ptr holder
+        m.def("bad1", []() { return std::unique_ptr<HeldByShared>(new HeldByShared()); });
+    });
+    m.def("register_mismatch_class", [](py::module m) {
+        // Fails: the class was already registered with a shared_ptr holder
+        py::class_<HeldByShared, std::unique_ptr<HeldByShared>>(m, "bad");
+    });
+
+    // HeldByUnique declared with unique_ptr holder, but used with shared_ptr before / later
+    m.def("register_return_shared", [](py::module m) {
+        // Fails if HeldByUnique is not yet registered or, if registered, due to mismatching holder
+        m.def("bad2", []() { return std::make_shared<HeldByUnique>(); });
+    });
+    m.def("register_consume_shared", [](py::module m) {
+        // Fails if HeldByUnique is not yet registered or, if registered, due to mismatching holder
+        m.def("bad3", [](std::shared_ptr<HeldByUnique>) {});
+    });
+    m.def("register_HeldByUnique", [](py::module m) {
+        py::class_<HeldByUnique>(m, "HeldByUnique");
+    });
 }

tests/test_smart_ptr.py

@@ -290,16 +290,6 @@ def test_move_only_holder_with_addressof_operator():
     assert stats.alive() == 0
 
 
-def test_smart_ptr_from_default():
-    instance = m.HeldByDefaultHolder()
-    with pytest.raises(RuntimeError) as excinfo:
-        m.HeldByDefaultHolder.load_shared_ptr(instance)
-    assert (
-        "Unable to load a custom holder type from a "
-        "default-holder instance" in str(excinfo.value)
-    )
-
-
 def test_shared_ptr_gc():
     """#187: issue involving std::shared_ptr<> return value policy & garbage collection"""
     el = m.ElementList()
@@ -308,3 +298,32 @@ def test_shared_ptr_gc():
     pytest.gc_collect()
     for i, v in enumerate(el.get()):
         assert i == v.value()
+
+
+def test_holder_mismatch():
+    """#1138: segfault if mixing holder types"""
+    with pytest.raises(RuntimeError) as excinfo:
+        m.register_mismatch_return(m)
+    assert "Detected mismatching holder types" in str(excinfo)
+    with pytest.raises(RuntimeError) as excinfo:
+        m.register_mismatch_class(m)
+    assert "is already registered" in str(excinfo)
+
+    with pytest.raises(RuntimeError) as excinfo:
+        m.register_return_shared(m)
+    expected_error = "Cannot register function using not yet registered type"
+    assert expected_error in str(excinfo)
+    with pytest.raises(RuntimeError) as excinfo:
+        m.register_consume_shared(m)
+    expected_error = "Cannot register function using not yet registered type"
+    assert expected_error in str(excinfo)
+
+    m.register_HeldByUnique(m)  # register the type
+
+    with pytest.raises(RuntimeError) as excinfo:
+        m.register_return_shared(m)
+    assert "Detected mismatching holder types" in str(excinfo)
+
+    with pytest.raises(RuntimeError) as excinfo:
+        m.register_consume_shared(m)
+    assert "Detected mismatching holder types" in str(excinfo)