Skip to content

Fix smart_holder def_readwrite for non-smart-holder member types#6003

Open
virtuald wants to merge 2 commits intopybind:masterfrom
virtuald:enum-mismatch
Open

Fix smart_holder def_readwrite for non-smart-holder member types#6003
virtuald wants to merge 2 commits intopybind:masterfrom
virtuald:enum-mismatch

Conversation

@virtuald
Copy link
Contributor

@virtuald virtuald commented Mar 3, 2026

Description

Ran into an issue where my extension was failing with pybind11 calling free on an enum. 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.

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 smart_holder def_readwrite for non-smart-holder member types

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant