Detect and fail if using mismatched holders by rhaschke · Pull Request #2644 · pybind/pybind11 (original) (raw)

33 changes: 22 additions & 11 deletions include/pybind11/attr.h

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Original file line number	Diff line number	Diff line change
Expand Up	@@ -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;
Expand All	@@ -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;

Expand Down Expand Up	@@ -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;

Expand All	@@ -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);
Expand Down

38 changes: 34 additions & 4 deletions include/pybind11/cast.h

[ Show hidden characters]({{ revealButtonHref }})

Original file line number	Diff line number	Diff line change
Expand Up	@@ -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()) {
Expand Down Expand Up	@@ -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 using is_holder = any_of<
Copy link Collaborator EricCousineau-TRI Dec 22, 2020 • edited Loading Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. nit To me, it looks like holder is really holder_caster. Should this be is_holder_caster? Copy link Collaborator YannickJadoul Jan 5, 2021 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. This already exists in cast.h, it seems? // PYBIND11_DECLARE_HOLDER_TYPE holder types: template <typename base, typename holder> struct is_holder_type : std::is_base_of<detail::type_caster_holder<base, holder>, detail::type_caster> {}; // Specialization for always-supported unique_ptr holders: template <typename base, typename deleter> struct is_holder_type<base, std::unique_ptr<base, deleter>> : std::true_type {}; Copy link Contributor Author rhaschke Jan 6, 2021 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. Yeah, I missed that. The existing definition should serve my purpose. YannickJadoul reacted with thumbs up emoji
is_template_base_of<move_only_holder_caster, make_caster>,
is_template_base_of<copyable_holder_caster, make_caster>>;

template
void check_for_holder_mismatch(const char*, enable_if_t<!is_holder::value, int> = 0) {}
template
void check_for_holder_mismatch(const char* func_name, enable_if_t<is_holder::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<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() + ", but " + type_id<base_type>() +
" 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<int_> { static constexpr auto name = _("int"); };
Expand Down

5 changes: 2 additions & 3 deletions include/pybind11/detail/internals.h

[ Show hidden characters]({{ revealButtonHref }})

Original file line number	Diff line number	Diff line change
Expand Up	@@ -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 *);
Expand All	@@ -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
Copy link Collaborator YannickJadoul Jan 5, 2021 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. This breaks ABI, so should ideally be part of a batch of ABI-breaking PRs.

/// On MSVC, debug and release builds are not ABI-compatible!
#if defined(_MSC_VER) && defined(_DEBUG)
Expand Down

14 changes: 9 additions & 5 deletions include/pybind11/pybind11.h

[ Show hidden characters]({{ revealButtonHref }})

Original file line number	Diff line number	Diff line change
Expand Up	@@ -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(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;
Expand Down Expand Up	@@ -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,
Expand Down Expand Up	@@ -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;
Expand All	@@ -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();
Expand Down Expand Up	@@ -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(&record);

Expand Down

8 changes: 4 additions & 4 deletions tests/test_class.py

[ Show hidden characters]({{ revealButtonHref }})

Original file line number	Diff line number	Diff line change
Expand Up	@@ -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),
)

Expand Down

38 changes: 30 additions & 8 deletions tests/test_smart_ptr.cpp

[ Show hidden characters]({{ revealButtonHref }})

Original file line number	Diff line number	Diff line change
Expand Up	@@ -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);

Expand Down Expand Up	@@ -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 {
Expand Down Expand Up	@@ -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>(m, "HeldByShared");
m.def("register_mismatch_return", [](py::module m) {
Copy link Collaborator EricCousineau-TRI Dec 22, 2020 • edited Loading Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. My brief reading of this PR leads me to believe that there's a sharp edge: a user could still hit a segfault if they call py::cast<mismatched_holder_type>(my_object). Totes get the idea of wanting to shift overhead from py::cast<>() (runtime) to function declarations (binding-time, e.g. .def()), but it now means there are now distinct entry points to this type-checking that users can trigger :( Possible resolutions: if it's worth the risk, then just mention it in the holder / smart pointer docs (as a diff in this PR) If it's not worth the risk, somehow enable raw py::cast<>() to do a runtime check. This may make for some crazy dumb plumbing, though :( Alternatively, eat the cost of runtime casts and funnel it through type_caster<>. That's what we do for our fork (RobotLocomotion/pybind11), and we haven't yet had to point fingers at performance there (though our use case may be simpler). @YannickJadoul or @rwgk Any chance y'all have a good (and mebbe easy?) timing performance benchmark for this, to see what the risk is in these terms? EDIT: Hm... for docs/benchmark.py, it's only for compilation time and size, and doesn't really dip into the more nuanced things (e.g. inheritance, custom type casters). Copy link Collaborator rwgk Dec 24, 2020 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. @EricCousineau-TRI wrote: Any chance y'all have a good (and mebbe easy?) timing performance benchmark for this, to see what the risk is in these terms? The TensorFlow core team has very sophisticated pybind11 benchmarks, but it's currently Google-internal only. The author already gave me permission to extract most of it for external view, including sources, but it may take me a few days (I want to show what I extract to the author for approval). Copy link Collaborator EricCousineau-TRI Dec 28, 2020 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. Sweet!!! That'd be awesome! I'll see if I can gather some common "complex" patterns in Drake to see if I can get some patterns we have (but maintain feature-parity with upstream). Have you thought any about where your benchmarks might live? I think the ones I'd generate would be simple enough (i.e. just pybind11 bits) to be in a benchmarking subdirectory. Copy link Contributor Author rhaschke Jan 6, 2021 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. I explicitly converted my previous code doing compatibility checks at cast time to this code, doing these tests at definition time only, because of efficiency concerns raised elsewhere.
// 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_<HeldByShared, std::unique_ptr>(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");
});
}

39 changes: 29 additions & 10 deletions tests/test_smart_ptr.py

[ Show hidden characters]({{ revealButtonHref }})

Original file line number	Diff line number	Diff line change
Expand Up	@@ -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()
Expand All	@@ -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)