Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Detect and fail if using mismatched holders #2644

Draft
wants to merge 12 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
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.
  • Loading branch information
rhaschke committed Nov 8, 2020
commit e57a3db72e020c450b72ba5ed68d42260e79a019
14 changes: 7 additions & 7 deletions include/pybind11/cast.h
Original file line number Diff line number Diff line change
Expand Up @@ -1492,13 +1492,13 @@ struct holder_helper {
};

template <typename holder>
void check_for_holder_mismatch_impl() {
bool check_for_holder_mismatch_impl(bool throw_if_missing = true) {
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));
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<base_type>();
if (!same_type(*base_info->holder_type, holder_typeinfo)) {
Expand All @@ -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.
Expand All @@ -1538,7 +1539,6 @@ struct copyable_holder_caster : public type_caster_base<type> {
explicit operator holder_type&() { return holder; }

static handle cast(const holder_type &src, return_value_policy, handle) {
check_for_holder_mismatch_impl<holder_type>();
const auto *ptr = holder_helper<holder_type>::get(src);
return type_caster_base<type>::cast_holder(ptr, &src);
}
Expand Down Expand Up @@ -1635,10 +1635,10 @@ template <typename holder> using is_holder = any_of<
is_template_base_of<copyable_holder_caster, make_caster<holder>>>;

template <typename holder>
void check_for_holder_mismatch(enable_if_t<!is_holder<holder>::value, int> = 0) {}
bool check_for_holder_mismatch(enable_if_t<!is_holder<holder>::value, int> = 0) { return true; }
template <typename holder>
void check_for_holder_mismatch(enable_if_t<is_holder<holder>::value, int> = 0) {
check_for_holder_mismatch_impl<holder>();
bool check_for_holder_mismatch(enable_if_t<is_holder<holder>::value, int> = 0) {
return check_for_holder_mismatch_impl<holder>(false);
}

template <typename T> struct handle_type_name { static constexpr auto name = _<T>(); };
Expand Down
7 changes: 6 additions & 1 deletion include/pybind11/pybind11.h
Original file line number Diff line number Diff line change
Expand Up @@ -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<Return>();
if (!detail::check_for_holder_mismatch<Return>()) {
// 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 {
Expand Down
10 changes: 9 additions & 1 deletion tests/test_smart_ptr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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<HeldByShared>(new HeldByShared()); });
});
m.def("return_shared", []() { return std::make_shared<HeldByUnique>(); });
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");
});

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<HeldByUnique>(); });
});
m.def("register_HeldByUnique", [](py::module m) {
py::class_<HeldByUnique>(m, "HeldByUnique");
});
Expand Down
13 changes: 10 additions & 3 deletions tests/test_smart_ptr.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down