diff --git a/include/pybind11/attr.h b/include/pybind11/attr.h index 0c41670926..975741e7cd 100644 --- a/include/pybind11/attr.h +++ b/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); diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index d6e440f787..99eb7c9783 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1521,10 +1521,7 @@ struct copyable_holder_caster : public type_caster_base { 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 struct is_holder_type : template struct is_holder_type> : std::true_type {}; +template using is_holder = any_of< + is_template_base_of>, + is_template_base_of>>; + +template +void check_for_holder_mismatch(const char*, enable_if_t::value, int> = 0) {} +template +void check_for_holder_mismatch(const char* func_name, enable_if_t::value, int> = 0) { + using iholder = intrinsic_t; + using base_type = decltype(*holder_helper::get(std::declval())); + 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() + "'"); +#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() + ", but " + type_id() + + " was declared using holder type " + holder_name); +#endif + } +} + template struct handle_type_name { static constexpr auto name = _(); }; template <> struct handle_type_name { static constexpr auto name = _(PYBIND11_BYTES_NAME); }; template <> struct handle_type_name { static constexpr auto name = _("int"); }; diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index a455715bfd..907a9632fc 100644 --- a/include/pybind11/detail/internals.h +++ b/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) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 51fb74a57f..21ee3d87cc 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -157,6 +157,13 @@ class cpp_function : public function { static_assert(expected_num_args(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::init(extra..., rec); + + // Fail if the return or argument types were previously registered with a different holder type + detail::check_for_holder_mismatch(rec->name); + PYBIND11_EXPAND_SIDE_EFFECTS(detail::check_for_holder_mismatch(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::init(extra..., rec); - { constexpr bool has_kw_only_args = any_of...>::value, has_pos_only_args = any_of...>::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); record.type_align = alignof(conditional_t&); + 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::value; set_operator_new(&record); diff --git a/tests/test_class.py b/tests/test_class.py index bdcced9643..04c14a76b7 100644 --- a/tests/test_class.py +++ b/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), ) diff --git a/tests/test_smart_ptr.cpp b/tests/test_smart_ptr.cpp index 60c2e692e5..f23118477f 100644 --- a/tests/test_smart_ptr.cpp +++ b/tests/test_smart_ptr.cpp @@ -37,11 +37,11 @@ PYBIND11_DECLARE_HOLDER_TYPE(T, std::shared_ptr); // holder size to trigger the non-simple-layout internal instance layout for single inheritance with // large holder type: template class huge_unique_ptr { - std::unique_ptr ptr; uint64_t padding[10]; + std::unique_ptr 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); @@ -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_(m, "HeldByDefaultHolder") - .def(py::init<>()) - .def_static("load_shared_ptr", [](std::shared_ptr) {}); - // 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_>(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(new HeldByShared()); }); + }); + m.def("register_mismatch_class", [](py::module m) { + // Fails: the class was already registered with a shared_ptr holder + py::class_>(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(); }); + }); + 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) {}); + }); + m.def("register_HeldByUnique", [](py::module m) { + py::class_(m, "HeldByUnique"); + }); } diff --git a/tests/test_smart_ptr.py b/tests/test_smart_ptr.py index c55bffba07..4a5ac0da08 100644 --- a/tests/test_smart_ptr.py +++ b/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)