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

Closed
virtuald wants to merge 2 commits into from
+57 −2
Closed

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
2 commits
Select commit Hold shift + click to select a range
6b7b3d2
Add tests that cause crash in def_readwrite
virtuald Mar 17, 2026
3e963ce
Fix crash in def_readwrite for non-smart-holder properties of smart-h…
virtuald Mar 17, 2026
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
Copy Markdown
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
Copy Markdown
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.

Copy link
Copy Markdown
Contributor Author

@virtuald virtuald Mar 17, 2026

Choose a reason for hiding this comment

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

Well, if you want stubgen to work, there's already an implicit requirement that you must bind a type before you use it as a parameter or other, so this doesn't seem that different to me.

I don't quite understand how your suggested fix would avoid that problem, you'd still need to call get_type_info(typeid(D)) at bind time (for efficiency) to determine the holder type of the member.

... with that said, clearly I don't understand how this works. If you remove the non-test changes from this PR (I split the tests and the fix into two separate commits for convenience), GPT decided the fix that you suggested looks like this:

diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h
index 1c4973b7..05aef0e9 100644
--- a/include/pybind11/cast.h
+++ b/include/pybind11/cast.h
@@ -1021,7 +1021,19 @@ public:
             return smart_holder_type_caster_support::smart_holder_from_shared_ptr(
                 src, policy, parent, srcs.result);
         }
-        return type_caster_base<type>::cast_holder(srcs, &src);
+
+        auto *tinfo = srcs.result.tinfo;
+        if (tinfo != nullptr && tinfo->holder_enum_v == holder_enum_t::std_shared_ptr) {
+            return type_caster_base<type>::cast_holder(srcs, &src);
+        }
+
+        if (parent) {
+            return type_caster_base<type>::cast(
+                srcs, return_value_policy::reference_internal, parent);
+        }
+
+        throw cast_error("Unable to convert std::shared_ptr<T> to Python when the bound type "
+                         "does not use std::shared_ptr or py::smart_holder as its holder type");
     }
 
     // This function will succeed even if the `responsible_parent` does not own the

and the tests pass, but it doesn't look right to me for some reason.

Copy link
Copy Markdown
Contributor Author

@virtuald virtuald Mar 17, 2026

Choose a reason for hiding this comment

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

I added #6008 with that fix to see if tests passed or not.

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