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

[virtuald]

Description

Ran into an issue where my extension was failing with pybind11 calling free on an enum (and on some platforms this ends up being a double-free?). Asked AI to dig into it and it got it wrong several times, but it finally did hit on a really subtle bug (described below).

Honestly, the fix and tests are probably slop -- I'm not sure why it decided to run the test in a subprocess, but the fix looks right and the tests pass. Open to suggestions.

I fixed the slop, looks better now.

gpt-5.3-high's analysis of the bug

This comes from a subtle interaction of generated bindings + smart_holder:

1. AlertSim::AlertInfo is bound as py::class_<..., py::smart_holder>.
2. AlertInfo.level is bound via def_readwrite("level", &AlertInfo::level).
3. For smart-holder classes, pybind’s def_readwrite path for by-value members uses a std::shared_ptr aliasing getter.
4. Here D is wpi::Alert::Level, which is bound as py::enum_ (inherits class_) with default holder = std::unique_ptr.
5. Casting that shared_ptr into a Python Level object goes through a path that assumes holder compatibility; for non-smart-holder
   target types it eventually treats holder memory as if it were unique_ptr.
6. That is UB; later deallocation runs unique_ptr delete on invalid data/pointer, producing free(): invalid size.

So pybind is not “randomly” deleting Level; it’s getting put into an invalid ownership state via holder-type mismatch.

Summarizing code locations with excerpts

Absolutely — here are the exact places.

1) def_readwrite routes through property_cpp_function<type, D>::read

  class_ &def_readwrite(const char *name, D C::*pm, const Extra &...extra) {                                                                 
      ...                                                                                                                                    
      def_property(name,                                                                                                                     
                   property_cpp_function<type, D>::read(pm, *this),                                                                          
                   property_cpp_function<type, D>::write(pm, *this),                                                                         
                   return_value_policy::reference_internal,                                                                                  
                   extra...);                                                                                   an expli                             
  }                                                                                                                                          

2) For smart-holder classes with by-value members, read returns std::shared_ptr

  template <typename T, typename D>                                                                                                          
  struct property_cpp_function_sh_member_held_by_value {                                                                                     
      ...                                                                                                                                    
      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) {                                                                         
              return cpp_function(                                                                                                           
                  [pm](handle c_hdl) -> std::shared_ptr<D> {                                                                                 
                      std::shared_ptr<T> c_sp                                                                                                
                          = type_caster<std::shared_ptr<T>>::shared_ptr_with_responsible_parent(c_hdl);                                      
                      return std::shared_ptr<D>(c_sp, &(c_sp.get()->*pm));                                                                   
                  },                                                                                                                         
                  is_method(hdl));                                                                                                           
          }                                                                                                                                  
          return property_cpp_function_classic<T, D>::read(pm, hdl);                                                                         
      }                                                                                                                                      
  }                                                                                                                                          

That is exactly the std::shared_ptr(c_sp, &(c_sp.get()->*pm)) aliasing getter.

---

3) This specialization is selected for by-value non-pointer/non-smart-pointer members

  struct property_cpp_function<...>                                                                                                          
      : detail::property_cpp_function_sh_member_held_by_value<T, D> {};                                                                      

Suggested changelog entry:

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

[oremanj]

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.

[virtuald]

I'll take a look this weekend.

[virtuald]

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.

[virtuald]

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