Fix crash in def_readwrite for non-smart-holder properties of smart-holder classes (v2) (#6008)

* Add tests that cause crash in def_readwrite

- Occurs with non-smart-holder property of smart-holder class

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

* Use default policy

* Address PR comments

* Add test for cast error path

* style: pre-commit fixes

* Revert "Use default policy"

This reverts commit b299f32.

* Disable test_shared_ptr_return_for_unique_ptr_holder when PYBIND11_TEST_SMART_HOLDER=ON

* Add counterexample

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Ralf W. Grosse-Kunstleve <rgrossekunst@nvidia.com>

Author: 3 people

include/pybind11/cast.h

@@ -1020,7 +1020,19 @@ struct copyable_holder_caster<
             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

tests/pybind11_tests.cpp

@@ -96,6 +96,12 @@ PYBIND11_MODULE(pybind11_tests, m, py::mod_gil_not_used()) {
 #else
         false;
 #endif
+    m.attr("PYBIND11_TEST_SMART_HOLDER") =
+#if defined(PYBIND11_RUN_TESTING_WITH_SMART_HOLDER_AS_DEFAULT_BUT_NEVER_USE_IN_PRODUCTION_PLEASE)
+        true;
+#else
+        false;
+#endif
 
     bind_ConstructorStats(m);
 

tests/test_class_sh_property.cpp

@@ -43,6 +43,24 @@ struct WithConstCharPtrMember {
     const char *const_char_ptr_member = "ConstChar*";
 };
 
+// See PR #6008
+enum class EnumAB {
+    A = 0,
+    B = 1,
+};
+
+struct ShWithEnumABMember {
+    EnumAB level = EnumAB::A;
+};
+
+struct SimpleStruct {
+    int value = 7;
+};
+
+struct ShWithSimpleStructMember {
+    SimpleStruct legacy;
+};
+
 } // namespace test_class_sh_property
 
 TEST_SUBMODULE(class_sh_property, m) {
@@ -91,4 +109,21 @@ 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);
+
+    // See PR #6008
+    py::enum_<EnumAB>(m, "EnumAB").value("A", EnumAB::A).value("B", EnumAB::B);
+
+    py::classh<ShWithEnumABMember>(m, "ShWithEnumABMember")
+        .def(py::init<>())
+        .def_readwrite("level", &ShWithEnumABMember::level);
+
+    py::class_<SimpleStruct>(m, "SimpleStruct")
+        .def(py::init<>())
+        .def_readwrite("value", &SimpleStruct::value);
+
+    py::classh<ShWithSimpleStructMember>(m, "ShWithSimpleStructMember")
+        .def(py::init<>())
+        .def_readwrite("legacy", &ShWithSimpleStructMember::legacy);
+
+    m.def("getSimpleStructAsShared", []() { return std::make_shared<SimpleStruct>(); });
 }

tests/test_class_sh_property.py

@@ -5,6 +5,7 @@
 import pytest
 
 import env  # noqa: F401
+import pybind11_tests
 from pybind11_tests import class_sh_property as m
 
 
@@ -164,3 +165,42 @@ def test_readonly_char6_member():
 def test_readonly_const_char_ptr_member():
     obj = m.WithConstCharPtrMember()
     assert obj.const_char_ptr_member == "ConstChar*"
+
+
+# See PR #6008
+def test_enum_member_with_smart_holder_def_readwrite():
+    obj = m.ShWithEnumABMember()
+    assert obj.level == m.EnumAB.A
+    for _ in range(100):
+        v = obj.level
+        assert v == m.EnumAB.A
+        del v
+
+
+# See PR #6008
+def test_non_smart_holder_member_type_with_smart_holder_owner():
+    obj = m.ShWithSimpleStructMember()
+    for _ in range(1000):
+        v = obj.legacy
+        assert v.value == 7
+        del v
+
+
+# See PR #6008, previously this was UB
+@pytest.mark.skipif(
+    pybind11_tests.PYBIND11_TEST_SMART_HOLDER,
+    reason="PYBIND11_TEST_SMART_HOLDER changes the default holder",
+)
+def test_shared_ptr_return_for_unique_ptr_holder():
+    with pytest.raises(
+        RuntimeError,
+        match="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",
+    ):
+        m.getSimpleStructAsShared()
+
+
+def test_non_smart_holder_member_type_with_smart_holder_owner_aliases_member():
+    obj = m.ShWithSimpleStructMember()
+    legacy = obj.legacy
+    legacy.value = 13
+    assert obj.legacy.value == 13