Skip to content

Fix crash in def_readwrite for non-smart-holder properties of smart-holder classes #6003

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.

Sign up for GitHub

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

Open
virtuald wants to merge 1 commit into
base: master
Choose a base branch
from
+57 −2
Open

Fix crash in def_readwrite for non-smart-holder properties of smart-holder classes #6003

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 9 additions & 2 deletions include/pybind11/pybind11.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2013,10 +2013,16 @@ struct property_cpp_function_sh_raw_ptr_member {
// This prevents disowning of the Python object owning the member.
template <typename T, typename D>
struct property_cpp_function_sh_member_held_by_value {
static bool use_smart_holder_member_aliasing() {
type_info *tinfo = get_type_info(typeid(D), /*throw_if_missing=*/true);
Copy link
Collaborator

@oremanj oremanj Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This adds an implicit requirement that you must bind the type of a field before you bind a property that returns something of that type. That has never been required before, and is certainly going to trip people up.

Can we instead modify the shared_ptr to-Python cast path so that it doesn't blindly assume it's casting to a type with a compatible holder? It used to not be able to know better, but now type_info has a holder-type field. And it would save the wrong-holder UB in all cases rather than just this one.

Copy link
Contributor Author

@virtuald virtuald Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll take a look this weekend.

return tinfo->holder_enum_v == holder_enum_t::smart_holder;
}

template <typename PM, must_be_member_function_pointer<PM> = 0>
static cpp_function readonly(PM pm, const handle &hdl) {
type_info *tinfo = get_type_info(typeid(T), /*throw_if_missing=*/true);
if (tinfo->holder_enum_v == holder_enum_t::smart_holder) {
if (tinfo->holder_enum_v == holder_enum_t::smart_holder
&& use_smart_holder_member_aliasing()) {
return cpp_function(
[pm](handle c_hdl) -> std::shared_ptr<typename std::add_const<D>::type> {
std::shared_ptr<T> c_sp
Expand All @@ -2033,7 +2039,8 @@ struct property_cpp_function_sh_member_held_by_value {
template <typename PM, must_be_member_function_pointer<PM> = 0>
static cpp_function read(PM pm, const handle &hdl) {
type_info *tinfo = get_type_info(typeid(T), /*throw_if_missing=*/true);
if (tinfo->holder_enum_v == holder_enum_t::smart_holder) {
if (tinfo->holder_enum_v == holder_enum_t::smart_holder
&& use_smart_holder_member_aliasing()) {
return cpp_function(
[pm](handle c_hdl) -> std::shared_ptr<D> {
std::shared_ptr<T> c_sp
Expand Down
31 changes: 31 additions & 0 deletions tests/test_class_sh_property.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,23 @@ struct WithConstCharPtrMember {
const char *const_char_ptr_member = "ConstChar*";
};

enum class TinyLevel {
A = 0,
B = 1,
};

struct HolderWithEnum {
TinyLevel level = TinyLevel::A;
};

struct LegacyThing {
int value = 7;
};

struct HolderWithLegacyMember {
LegacyThing legacy;
};

} // namespace test_class_sh_property

TEST_SUBMODULE(class_sh_property, m) {
Expand Down Expand Up @@ -91,4 +108,18 @@ TEST_SUBMODULE(class_sh_property, m) {
py::classh<WithConstCharPtrMember>(m, "WithConstCharPtrMember")
.def(py::init<>())
.def_readonly("const_char_ptr_member", &WithConstCharPtrMember::const_char_ptr_member);

py::enum_<TinyLevel>(m, "TinyLevel").value("A", TinyLevel::A).value("B", TinyLevel::B);

py::classh<HolderWithEnum>(m, "HolderWithEnum")
.def(py::init<>())
.def_readwrite("level", &HolderWithEnum::level);

py::class_<LegacyThing>(m, "LegacyThing")
.def(py::init<>())
.def_readwrite("value", &LegacyThing::value);

py::classh<HolderWithLegacyMember>(m, "HolderWithLegacyMember")
.def(py::init<>())
.def_readwrite("legacy", &HolderWithLegacyMember::legacy);
}
17 changes: 17 additions & 0 deletions tests/test_class_sh_property.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,3 +164,20 @@ def test_readonly_char6_member():
def test_readonly_const_char_ptr_member():
obj = m.WithConstCharPtrMember()
assert obj.const_char_ptr_member == "ConstChar*"


def test_enum_member_with_smart_holder_def_readwrite():
obj = m.HolderWithEnum()
assert obj.level == m.TinyLevel.A
for _ in range(100):
v = obj.level
assert v == m.TinyLevel.A
del v


def test_non_smart_holder_member_type_with_smart_holder_owner():
obj = m.HolderWithLegacyMember()
for _ in range(1000):
v = obj.legacy
assert v.value == 7
del v
Loading