From b498e520ee6b37538aba125f6f6d137d1445eeee Mon Sep 17 00:00:00 2001 From: Jason Rhinelander Date: Sun, 29 Oct 2017 20:32:00 +0100 Subject: [PATCH 01/12] Incompatible holder types cause crash (unit test) --- tests/test_smart_ptr.cpp | 15 +++++++++++++++ tests/test_smart_ptr.py | 11 +++++++++++ 2 files changed, 26 insertions(+) diff --git a/tests/test_smart_ptr.cpp b/tests/test_smart_ptr.cpp index 60c2e692e5..714edd590c 100644 --- a/tests/test_smart_ptr.cpp +++ b/tests/test_smart_ptr.cpp @@ -367,4 +367,19 @@ 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 {}; + 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("return_shared", []() { return std::make_shared(); }); + m.def("register_mismatch_class", [](py::module m) { + // Fails: `return_shared' already returned this via shared_ptr holder + py::class_(m, "HeldByUnique"); + }); } diff --git a/tests/test_smart_ptr.py b/tests/test_smart_ptr.py index c55bffba07..738af888d5 100644 --- a/tests/test_smart_ptr.py +++ b/tests/test_smart_ptr.py @@ -308,3 +308,14 @@ 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 "Mismatched holders detected" in str(excinfo) + + with pytest.raises(RuntimeError) as excinfo: + m.register_mismatch_class(m) + assert "Mismatched holders detected" in str(excinfo) From 44f23a28f6743125b9babfab8f0d8962a7162bca Mon Sep 17 00:00:00 2001 From: Jason Rhinelander Date: Sun, 29 Oct 2017 16:32:26 -0300 Subject: [PATCH 02/12] Detect and fail if using mismatched holders This adds a check when registering a class or a function with a holder return that the same wrapped type hasn't been previously seen using a different holder type. This fixes #1138 by detecting the failure; currently attempting to use two different holder types (e.g. a unique_ptr and shared_ptr) in difference places can segfault because we don't have any type safety on the holder instances. --- include/pybind11/cast.h | 27 +++++++++++++++++++++++++++ include/pybind11/detail/internals.h | 3 ++- include/pybind11/pybind11.h | 6 ++++++ tests/test_smart_ptr.cpp | 2 +- 4 files changed, 36 insertions(+), 2 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index d6e440f787..8b4f0ac434 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1607,6 +1607,33 @@ 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(enable_if_t::value, int> = 0) {} +template +void check_for_holder_mismatch(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 ins = get_internals().holders_seen.emplace(typeid(base_type), &holder_typeinfo); + + auto debug = type_id(); + if (!ins.second && !same_type(*ins.first->second, holder_typeinfo)) { +#ifdef NDEBUG + pybind11_fail("Mismatched holders detected (compile in debug mode for details)"); +#else + std::string seen_holder_name(ins.first->second->name()); + detail::clean_type_id(seen_holder_name); + pybind11_fail("Mismatched holders detected: " + " attempting to use holder type " + type_id() + ", but " + type_id() + + " was already seen using holder type " + seen_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..f7c5987b19 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -97,6 +97,7 @@ struct internals { type_map registered_types_cpp; // std::type_index -> pybind11's type information std::unordered_map> registered_types_py; // PyTypeObject* -> base type_info(s) std::unordered_multimap registered_instances; // void * -> instance* + type_map holders_seen; // type -> seen holder type (to detect holder conflicts) std::unordered_set, override_hash> inactive_override_cache; type_map> direct_conversions; std::unordered_map> patients; @@ -150,7 +151,7 @@ struct type_info { }; /// 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..67d932ce62 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -157,6 +157,9 @@ 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"); + // Fail if we've previously seen a different holder around the held type + detail::check_for_holder_mismatch(); + /* Dispatch code which converts function arguments and performs the actual function call */ rec->impl = [](function_call &call) -> handle { cast_in args_converter; @@ -1221,6 +1224,9 @@ class class_ : public detail::generic_type { none_of...>::value), // no multiple_inheritance attr "Error: multiple inheritance bases must be specified via class_ template options"); + // Fail if we've previously seen a different holder around the type + detail::check_for_holder_mismatch(); + type_record record; record.scope = scope; record.name = name; diff --git a/tests/test_smart_ptr.cpp b/tests/test_smart_ptr.cpp index 714edd590c..7e4afd0a42 100644 --- a/tests/test_smart_ptr.cpp +++ b/tests/test_smart_ptr.cpp @@ -41,7 +41,7 @@ template class huge_unique_ptr { uint64_t padding[10]; 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); From 0fe5697c22616ef4988f029cfd1fa3985318ec4b Mon Sep 17 00:00:00 2001 From: Robert Haschke Date: Tue, 20 Oct 2020 23:18:21 +0200 Subject: [PATCH 03/12] Failing test on mismatching function arguments --- tests/test_smart_ptr.cpp | 4 +++- tests/test_smart_ptr.py | 2 ++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/test_smart_ptr.cpp b/tests/test_smart_ptr.cpp index 7e4afd0a42..b9e11e1da9 100644 --- a/tests/test_smart_ptr.cpp +++ b/tests/test_smart_ptr.cpp @@ -37,8 +37,8 @@ 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() const { return ptr.get(); } @@ -382,4 +382,6 @@ TEST_SUBMODULE(smart_ptr, m) { // Fails: `return_shared' already returned this via shared_ptr holder py::class_(m, "HeldByUnique"); }); + // segfaults, because std::shared_ptr is interpreted as huge_unique_ptr + m.def("consume_mismatching_holder", [](std::shared_ptr o) { return o->value; }); } diff --git a/tests/test_smart_ptr.py b/tests/test_smart_ptr.py index 738af888d5..a364324ee7 100644 --- a/tests/test_smart_ptr.py +++ b/tests/test_smart_ptr.py @@ -319,3 +319,5 @@ def test_holder_mismatch(): with pytest.raises(RuntimeError) as excinfo: m.register_mismatch_class(m) assert "Mismatched holders detected" in str(excinfo) + + assert m.consume_mismatching_holder(m.MyObject5(42)) == 42 From 6360006269838eb85284818eaa9cc6d9b327d141 Mon Sep 17 00:00:00 2001 From: Robert Haschke Date: Thu, 5 Nov 2020 21:53:08 +0100 Subject: [PATCH 04/12] Correctly handle mismatched holders in function arguments --- include/pybind11/cast.h | 40 +++++++++++++++++++++++----------------- tests/test_smart_ptr.cpp | 2 +- tests/test_smart_ptr.py | 4 +++- 3 files changed, 27 insertions(+), 19 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 8b4f0ac434..58b02476aa 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1491,6 +1491,27 @@ struct holder_helper { static auto get(const T &p) -> decltype(p.get()) { return p.get(); } }; +template +void check_for_holder_mismatch_impl() { + using iholder = intrinsic_t; + using base_type = decltype(*holder_helper::get(std::declval())); + auto &holder_typeinfo = typeid(iholder); + auto ins = get_internals().holders_seen.emplace(typeid(base_type), &holder_typeinfo); + + auto debug = type_id(); + if (!ins.second && !same_type(*ins.first->second, holder_typeinfo)) { +#ifdef NDEBUG + pybind11_fail("Mismatched holders detected (compile in debug mode for details)"); +#else + std::string seen_holder_name(ins.first->second->name()); + detail::clean_type_id(seen_holder_name); + pybind11_fail("Mismatched holders detected: " + " attempting to use holder type " + type_id() + ", but " + type_id() + + " was already seen using holder type " + seen_holder_name); +#endif + } +} + /// Type caster for holder types like std::shared_ptr, etc. template struct copyable_holder_caster : public type_caster_base { @@ -1524,6 +1545,7 @@ struct copyable_holder_caster : public type_caster_base { void check_holder_compat() { if (typeinfo->default_holder) throw cast_error("Unable to load a custom holder type from a default-holder instance"); + check_for_holder_mismatch_impl(); } bool load_value(value_and_holder &&v_h) { @@ -1615,23 +1637,7 @@ template void check_for_holder_mismatch(enable_if_t::value, int> = 0) {} template void check_for_holder_mismatch(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 ins = get_internals().holders_seen.emplace(typeid(base_type), &holder_typeinfo); - - auto debug = type_id(); - if (!ins.second && !same_type(*ins.first->second, holder_typeinfo)) { -#ifdef NDEBUG - pybind11_fail("Mismatched holders detected (compile in debug mode for details)"); -#else - std::string seen_holder_name(ins.first->second->name()); - detail::clean_type_id(seen_holder_name); - pybind11_fail("Mismatched holders detected: " - " attempting to use holder type " + type_id() + ", but " + type_id() + - " was already seen using holder type " + seen_holder_name); -#endif - } + check_for_holder_mismatch_impl(); } template struct handle_type_name { static constexpr auto name = _(); }; diff --git a/tests/test_smart_ptr.cpp b/tests/test_smart_ptr.cpp index b9e11e1da9..34528e5937 100644 --- a/tests/test_smart_ptr.cpp +++ b/tests/test_smart_ptr.cpp @@ -382,6 +382,6 @@ TEST_SUBMODULE(smart_ptr, m) { // Fails: `return_shared' already returned this via shared_ptr holder py::class_(m, "HeldByUnique"); }); - // segfaults, because std::shared_ptr is interpreted as huge_unique_ptr + // Fails: MyObject5 was declared with huge_unique_ptr as holder instead of shared_ptr m.def("consume_mismatching_holder", [](std::shared_ptr o) { return o->value; }); } diff --git a/tests/test_smart_ptr.py b/tests/test_smart_ptr.py index a364324ee7..ca26e70af6 100644 --- a/tests/test_smart_ptr.py +++ b/tests/test_smart_ptr.py @@ -320,4 +320,6 @@ def test_holder_mismatch(): m.register_mismatch_class(m) assert "Mismatched holders detected" in str(excinfo) - assert m.consume_mismatching_holder(m.MyObject5(42)) == 42 + with pytest.raises(RuntimeError) as excinfo: + m.consume_mismatching_holder(m.MyObject5(42)) + assert "Mismatched holders detected" in str(excinfo) From e2aa4eceeec56ec1cf2b0db8d67bf72d296461b3 Mon Sep 17 00:00:00 2001 From: Robert Haschke Date: Fri, 6 Nov 2020 10:18:34 +0100 Subject: [PATCH 05/12] Replace global map holders_seen with local holder_type in registered type_info --- include/pybind11/attr.h | 3 +++ include/pybind11/cast.h | 8 +++++--- include/pybind11/detail/internals.h | 2 +- include/pybind11/pybind11.h | 2 ++ 4 files changed, 11 insertions(+), 4 deletions(-) diff --git a/include/pybind11/attr.h b/include/pybind11/attr.h index 0c41670926..b2207c69f9 100644 --- a/include/pybind11/attr.h +++ b/include/pybind11/attr.h @@ -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; diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 58b02476aa..16d43ea743 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1496,14 +1496,16 @@ void check_for_holder_mismatch_impl() { using iholder = intrinsic_t; using base_type = decltype(*holder_helper::get(std::declval())); auto &holder_typeinfo = typeid(iholder); - auto ins = get_internals().holders_seen.emplace(typeid(base_type), &holder_typeinfo); + auto base_info = detail::get_type_info(typeid(base_type)); + if (!base_info) + return; // Don't complain if we see this type the first time auto debug = type_id(); - if (!ins.second && !same_type(*ins.first->second, holder_typeinfo)) { + if (!same_type(*base_info->holder_type, holder_typeinfo)) { #ifdef NDEBUG pybind11_fail("Mismatched holders detected (compile in debug mode for details)"); #else - std::string seen_holder_name(ins.first->second->name()); + std::string seen_holder_name(base_info->holder_type->name()); detail::clean_type_id(seen_holder_name); pybind11_fail("Mismatched holders detected: " " attempting to use holder type " + type_id() + ", but " + type_id() + diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index f7c5987b19..bbdc2cea3a 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -97,7 +97,6 @@ struct internals { type_map registered_types_cpp; // std::type_index -> pybind11's type information std::unordered_map> registered_types_py; // PyTypeObject* -> base type_info(s) std::unordered_multimap registered_instances; // void * -> instance* - type_map holders_seen; // type -> seen holder type (to detect holder conflicts) std::unordered_set, override_hash> inactive_override_cache; type_map> direct_conversions; std::unordered_map> patients; @@ -129,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 *); diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 67d932ce62..5eb203b880 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1045,6 +1045,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; @@ -1233,6 +1234,7 @@ 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; From 502bf24a62ce5487c5bc88ee7046544045048266 Mon Sep 17 00:00:00 2001 From: Robert Haschke Date: Fri, 6 Nov 2020 12:14:59 +0100 Subject: [PATCH 06/12] fixup! Replace global map holders_seen with local holder_type in registered type_info Having replaced the global map holders_seen, we can only check return values against already registered types. Hence, we need to replace the check at initialization time with a check at call time (when casting the return value). --- include/pybind11/cast.h | 1 + include/pybind11/pybind11.h | 5 +---- tests/test_smart_ptr.cpp | 3 +-- tests/test_smart_ptr.py | 5 ++++- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 16d43ea743..2faa5b604c 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1538,6 +1538,7 @@ struct copyable_holder_caster : public type_caster_base { explicit operator holder_type&() { return holder; } static handle cast(const holder_type &src, return_value_policy, handle) { + check_for_holder_mismatch_impl(); const auto *ptr = holder_helper::get(src); return type_caster_base::cast_holder(ptr, &src); } diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 5eb203b880..1c173fbb66 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -157,7 +157,7 @@ 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"); - // Fail if we've previously seen a different holder around the held type + // Fail if the type was previously registered with a different holder type detail::check_for_holder_mismatch(); /* Dispatch code which converts function arguments and performs the actual function call */ @@ -1225,9 +1225,6 @@ class class_ : public detail::generic_type { none_of...>::value), // no multiple_inheritance attr "Error: multiple inheritance bases must be specified via class_ template options"); - // Fail if we've previously seen a different holder around the type - detail::check_for_holder_mismatch(); - type_record record; record.scope = scope; record.name = name; diff --git a/tests/test_smart_ptr.cpp b/tests/test_smart_ptr.cpp index 34528e5937..6cc9aedcad 100644 --- a/tests/test_smart_ptr.cpp +++ b/tests/test_smart_ptr.cpp @@ -378,8 +378,7 @@ TEST_SUBMODULE(smart_ptr, m) { m.def("bad1", []() { return std::unique_ptr(new HeldByShared()); }); }); m.def("return_shared", []() { return std::make_shared(); }); - m.def("register_mismatch_class", [](py::module m) { - // Fails: `return_shared' already returned this via shared_ptr holder + m.def("register_HeldByUnique", [](py::module m) { py::class_(m, "HeldByUnique"); }); // Fails: MyObject5 was declared with huge_unique_ptr as holder instead of shared_ptr diff --git a/tests/test_smart_ptr.py b/tests/test_smart_ptr.py index ca26e70af6..97a823f0e7 100644 --- a/tests/test_smart_ptr.py +++ b/tests/test_smart_ptr.py @@ -316,8 +316,11 @@ def test_holder_mismatch(): m.register_mismatch_return(m) assert "Mismatched holders detected" in str(excinfo) + with pytest.raises(TypeError) as excinfo: + m.return_shared() # calling a function returning an unregistered type + m.register_HeldByUnique(m) # register the type with pytest.raises(RuntimeError) as excinfo: - m.register_mismatch_class(m) + m.return_shared() # calling a function returning a mismatching holder type assert "Mismatched holders detected" in str(excinfo) with pytest.raises(RuntimeError) as excinfo: From 7852e7d7b0037d5e0449c35f4bab8ce40b65ff9e Mon Sep 17 00:00:00 2001 From: Robert Haschke Date: Fri, 6 Nov 2020 10:19:19 +0100 Subject: [PATCH 07/12] Generalize holder compatibility check for derived classes A derived class needs to use the same holder type as its base class(es). So far, the check was constrained to the default holder vs. custom holder types. Thus, we replace the simple check (based on the default_holder flag) with a more elaborate one, comparing holder type names. --- include/pybind11/attr.h | 35 ++++++++++++++++++++--------- include/pybind11/cast.h | 2 -- include/pybind11/detail/internals.h | 2 -- include/pybind11/pybind11.h | 2 -- tests/test_class.py | 8 +++---- tests/test_smart_ptr.py | 5 +---- 6 files changed, 29 insertions(+), 25 deletions(-) diff --git a/include/pybind11/attr.h b/include/pybind11/attr.h index b2207c69f9..2c3d604a9f 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; @@ -271,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; @@ -289,13 +286,29 @@ 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); + // replace all occurences of base_name within holder_name with T + size_t start_pos = 0; + while((start_pos = holder_name.find(base_name, start_pos)) != std::string::npos) { + holder_name.replace(start_pos, base_name.length(), "T"); + start_pos += 1; + } + return holder_name; + }; + 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 2faa5b604c..9b8d88bd66 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1546,8 +1546,6 @@ 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"); check_for_holder_mismatch_impl(); } diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index bbdc2cea3a..907a9632fc 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -144,8 +144,6 @@ 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; }; diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 1c173fbb66..d511cd1ee2 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1054,7 +1054,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(); @@ -1235,7 +1234,6 @@ class class_ : public detail::generic_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.py b/tests/test_smart_ptr.py index 97a823f0e7..5142de8a85 100644 --- a/tests/test_smart_ptr.py +++ b/tests/test_smart_ptr.py @@ -294,10 +294,7 @@ 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) - ) + assert "Mismatched holders detected" in str(excinfo.value) def test_shared_ptr_gc(): From cde5c11c5ab35d2df84a912aa0714e8b33d4aafc Mon Sep 17 00:00:00 2001 From: Robert Haschke Date: Fri, 6 Nov 2020 10:44:42 +0100 Subject: [PATCH 08/12] Relax holder type check --- include/pybind11/attr.h | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/include/pybind11/attr.h b/include/pybind11/attr.h index 2c3d604a9f..975741e7cd 100644 --- a/include/pybind11/attr.h +++ b/include/pybind11/attr.h @@ -294,13 +294,8 @@ struct type_record { detail::clean_type_id(base_name); std::string holder_name(holder_type->name()); detail::clean_type_id(holder_name); - // replace all occurences of base_name within holder_name with T - size_t start_pos = 0; - while((start_pos = holder_name.find(base_name, start_pos)) != std::string::npos) { - holder_name.replace(start_pos, base_name.length(), "T"); - start_pos += 1; - } - return 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); From d5c43867b01daec9c3a0f74a682801c73704e422 Mon Sep 17 00:00:00 2001 From: Robert Haschke Date: Sat, 7 Nov 2020 09:47:25 +0100 Subject: [PATCH 09/12] fixup! Replace global map holders_seen with local holder_type in registered type_info Improve wording and variable name: - seen_holder_name -> holder_name - seen holder -> declared holder --- include/pybind11/cast.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 9b8d88bd66..13b99e267b 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1505,11 +1505,11 @@ void check_for_holder_mismatch_impl() { #ifdef NDEBUG pybind11_fail("Mismatched holders detected (compile in debug mode for details)"); #else - std::string seen_holder_name(base_info->holder_type->name()); - detail::clean_type_id(seen_holder_name); + std::string holder_name(base_info->holder_type->name()); + detail::clean_type_id(holder_name); pybind11_fail("Mismatched holders detected: " " attempting to use holder type " + type_id() + ", but " + type_id() + - " was already seen using holder type " + seen_holder_name); + " was declared using holder type " + holder_name); #endif } } From e57a3db72e020c450b72ba5ed68d42260e79a019 Mon Sep 17 00:00:00 2001 From: Robert Haschke Date: Sun, 8 Nov 2020 21:36:38 +0100 Subject: [PATCH 10/12] fixup! fixup! Replace global map holders_seen with local holder_type in registered type_info Runtime costs of checking holder compatibility for function return types during can be limited to definition time (instead of calling time), if we require types to be registered before defining a function. This allows to lookup the type_info record and validate the holder type as expected. As we cannot call functions with an unregistered return type anyway, I think this is a good compromise. --- include/pybind11/cast.h | 14 +++++++------- include/pybind11/pybind11.h | 7 ++++++- tests/test_smart_ptr.cpp | 10 +++++++++- tests/test_smart_ptr.py | 13 ++++++++++--- 4 files changed, 32 insertions(+), 12 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 13b99e267b..121dcfd478 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1492,13 +1492,13 @@ struct holder_helper { }; template -void check_for_holder_mismatch_impl() { +bool check_for_holder_mismatch_impl(bool throw_if_missing = true) { 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)); + auto base_info = detail::get_type_info(typeid(base_type), throw_if_missing); if (!base_info) - return; // Don't complain if we see this type the first time + return false; // Return false if the type is not yet registered auto debug = type_id(); if (!same_type(*base_info->holder_type, holder_typeinfo)) { @@ -1512,6 +1512,7 @@ void check_for_holder_mismatch_impl() { " was declared using holder type " + holder_name); #endif } + return true; } /// Type caster for holder types like std::shared_ptr, etc. @@ -1538,7 +1539,6 @@ struct copyable_holder_caster : public type_caster_base { explicit operator holder_type&() { return holder; } static handle cast(const holder_type &src, return_value_policy, handle) { - check_for_holder_mismatch_impl(); const auto *ptr = holder_helper::get(src); return type_caster_base::cast_holder(ptr, &src); } @@ -1635,10 +1635,10 @@ template using is_holder = any_of< is_template_base_of>>; template -void check_for_holder_mismatch(enable_if_t::value, int> = 0) {} +bool check_for_holder_mismatch(enable_if_t::value, int> = 0) { return true; } template -void check_for_holder_mismatch(enable_if_t::value, int> = 0) { - check_for_holder_mismatch_impl(); +bool check_for_holder_mismatch(enable_if_t::value, int> = 0) { + return check_for_holder_mismatch_impl(false); } template struct handle_type_name { static constexpr auto name = _(); }; diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index d511cd1ee2..45b59ad5b5 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -158,7 +158,12 @@ class cpp_function : public function { "The number of argument annotations does not match the number of function arguments"); // Fail if the type was previously registered with a different holder type - detail::check_for_holder_mismatch(); + if (!detail::check_for_holder_mismatch()) { + // If Return type was not yet registered, check_for_holder_mismatch() returns false w/o raising + std::string tname(typeid(Return).name()); + detail::clean_type_id(tname); + pybind11_fail("Cannot register function with not yet registered return type \"" + tname + "\""); + } /* Dispatch code which converts function arguments and performs the actual function call */ rec->impl = [](function_call &call) -> handle { diff --git a/tests/test_smart_ptr.cpp b/tests/test_smart_ptr.cpp index 6cc9aedcad..eacf572a32 100644 --- a/tests/test_smart_ptr.cpp +++ b/tests/test_smart_ptr.cpp @@ -377,7 +377,15 @@ TEST_SUBMODULE(smart_ptr, m) { // Fails: the class was already registered with a shared_ptr holder m.def("bad1", []() { return std::unique_ptr(new HeldByShared()); }); }); - m.def("return_shared", []() { return std::make_shared(); }); + m.def("register_mismatch_class", [](py::module m) { + // Fails: the class was already registered with a shared_ptr holder + py::class_>(m, "bad"); + }); + + m.def("register_return_shared", [](py::module m) { + // Fails if HeldByUnique is not yet registered or, if registered, due to mismatching holder + m.def("return_shared", []() { return std::make_shared(); }); + }); 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 5142de8a85..edf1e59623 100644 --- a/tests/test_smart_ptr.py +++ b/tests/test_smart_ptr.py @@ -312,12 +312,19 @@ def test_holder_mismatch(): with pytest.raises(RuntimeError) as excinfo: m.register_mismatch_return(m) assert "Mismatched holders detected" 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 with not yet registered return type" + assert expected_error in str(excinfo) - with pytest.raises(TypeError) as excinfo: - m.return_shared() # calling a function returning an unregistered type m.register_HeldByUnique(m) # register the type + with pytest.raises(RuntimeError) as excinfo: - m.return_shared() # calling a function returning a mismatching holder type + m.register_return_shared(m) assert "Mismatched holders detected" in str(excinfo) with pytest.raises(RuntimeError) as excinfo: From 8115384d038eb6fa9f71c4fa51fadf477d6b925d Mon Sep 17 00:00:00 2001 From: Robert Haschke Date: Mon, 9 Nov 2020 06:32:23 +0100 Subject: [PATCH 11/12] fixup! fixup! fixup! Replace global map holders_seen with local holder_type in registered type_info Simplify code using type_id() --- include/pybind11/pybind11.h | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 45b59ad5b5..7adbb77eaa 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -160,9 +160,7 @@ class cpp_function : public function { // Fail if the type was previously registered with a different holder type if (!detail::check_for_holder_mismatch()) { // If Return type was not yet registered, check_for_holder_mismatch() returns false w/o raising - std::string tname(typeid(Return).name()); - detail::clean_type_id(tname); - pybind11_fail("Cannot register function with not yet registered return type \"" + tname + "\""); + pybind11_fail("Cannot register function with not yet registered return type \"" + type_id() + "\""); } /* Dispatch code which converts function arguments and performs the actual function call */ From 02d0b53ba2d0ebdfaa00e885560cf9a10cdc6157 Mon Sep 17 00:00:00 2001 From: Robert Haschke Date: Mon, 23 Nov 2020 11:13:36 +0100 Subject: [PATCH 12/12] Move runtime check for holder compatibility into function registration To avoid costly runtime checks at function call time, compatibility of holder types is now checked once at functionn registration time. This assumes that all custom argument types are declared in advance! --- include/pybind11/cast.h | 56 +++++++++++++++++-------------------- include/pybind11/pybind11.h | 14 ++++------ tests/test_smart_ptr.cpp | 16 +++++------ tests/test_smart_ptr.py | 21 ++++++-------- 4 files changed, 48 insertions(+), 59 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 121dcfd478..99eb7c9783 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1491,30 +1491,6 @@ struct holder_helper { static auto get(const T &p) -> decltype(p.get()) { return p.get(); } }; -template -bool check_for_holder_mismatch_impl(bool throw_if_missing = true) { - 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), throw_if_missing); - if (!base_info) - return false; // Return false if the type is not yet registered - - auto debug = type_id(); - if (!same_type(*base_info->holder_type, holder_typeinfo)) { -#ifdef NDEBUG - pybind11_fail("Mismatched holders detected (compile in debug mode for details)"); -#else - std::string holder_name(base_info->holder_type->name()); - detail::clean_type_id(holder_name); - pybind11_fail("Mismatched holders detected: " - " attempting to use holder type " + type_id() + ", but " + type_id() + - " was declared using holder type " + holder_name); -#endif - } - return true; -} - /// Type caster for holder types like std::shared_ptr, etc. template struct copyable_holder_caster : public type_caster_base { @@ -1545,9 +1521,7 @@ struct copyable_holder_caster : public type_caster_base { protected: friend class type_caster_generic; - void check_holder_compat() { - check_for_holder_mismatch_impl(); - } + void check_holder_compat() {} bool load_value(value_and_holder &&v_h) { if (v_h.holder_constructed()) { @@ -1635,10 +1609,32 @@ template using is_holder = any_of< is_template_base_of>>; template -bool check_for_holder_mismatch(enable_if_t::value, int> = 0) { return true; } +void check_for_holder_mismatch(const char*, enable_if_t::value, int> = 0) {} template -bool check_for_holder_mismatch(enable_if_t::value, int> = 0) { - return check_for_holder_mismatch_impl(false); +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 = _(); }; diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 7adbb77eaa..21ee3d87cc 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -157,11 +157,12 @@ 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"); - // Fail if the type was previously registered with a different holder type - if (!detail::check_for_holder_mismatch()) { - // If Return type was not yet registered, check_for_holder_mismatch() returns false w/o raising - pybind11_fail("Cannot register function with not yet registered return type \"" + type_id() + "\""); - } + /* 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 { @@ -195,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, diff --git a/tests/test_smart_ptr.cpp b/tests/test_smart_ptr.cpp index eacf572a32..f23118477f 100644 --- a/tests/test_smart_ptr.cpp +++ b/tests/test_smart_ptr.cpp @@ -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 { @@ -372,6 +366,7 @@ TEST_SUBMODULE(smart_ptr, m) { // 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 @@ -382,13 +377,16 @@ TEST_SUBMODULE(smart_ptr, m) { 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("return_shared", []() { return std::make_shared(); }); + 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"); }); - // Fails: MyObject5 was declared with huge_unique_ptr as holder instead of shared_ptr - m.def("consume_mismatching_holder", [](std::shared_ptr o) { return o->value; }); } diff --git a/tests/test_smart_ptr.py b/tests/test_smart_ptr.py index edf1e59623..4a5ac0da08 100644 --- a/tests/test_smart_ptr.py +++ b/tests/test_smart_ptr.py @@ -290,13 +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 "Mismatched holders detected" in str(excinfo.value) - - def test_shared_ptr_gc(): """#187: issue involving std::shared_ptr<> return value policy & garbage collection""" el = m.ElementList() @@ -311,22 +304,26 @@ def test_holder_mismatch(): """#1138: segfault if mixing holder types""" with pytest.raises(RuntimeError) as excinfo: m.register_mismatch_return(m) - assert "Mismatched holders detected" in str(excinfo) + 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 with not yet registered return type" + 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 "Mismatched holders detected" in str(excinfo) + assert "Detected mismatching holder types" in str(excinfo) with pytest.raises(RuntimeError) as excinfo: - m.consume_mismatching_holder(m.MyObject5(42)) - assert "Mismatched holders detected" in str(excinfo) + m.register_consume_shared(m) + assert "Detected mismatching holder types" in str(excinfo)