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

[virtuald]

Description

Alternative fix for #6003, primarily made it because I'm not sure this is better and to check that all tests pass. See discussion over on that PR for background and analysis.

Suggested changelog entry:

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

[rwgk]

@virtuald @oremanj I think this PR is definitely the way to go (suggestion to close PR #6003 in favor of this PR).

Below is a Opus 4.6 1M Thinking analysis of the fix.

I think we should merge this fix as-is, because it's a strict improvement, and maybe do other things in follow-on PRs.

For this PR: I still need to look carefully at the tests.

---

The bug

When a smart-holder class (py::classh<T>) has a by-value member whose type uses a non-shared_ptr holder (e.g., an enum bound via py::enum_<E>, which uses unique_ptr<E>), def_readwrite creates an aliasing shared_ptr<E> pointing into the parent object. The shared_ptr-to-Python cast path then calls cast_holder(srcs, &src), which tries to stuff the shared_ptr into the target type's holder. For unique_ptr-held types, this is UB — it reinterprets shared_ptr memory as unique_ptr, leading to invalid free / double-free on deallocation.

The fix

In copyable_holder_caster<type, shared_ptr<type>, ...>::cast():

static handle
cast(const std::shared_ptr<type> &src, return_value_policy policy, handle parent) {
    const auto *ptr = src.get();
    typename type_caster_base<type>::cast_sources srcs{ptr};
    if (srcs.creates_smart_holder()) {
        return smart_holder_type_caster_support::smart_holder_from_shared_ptr(
            src, policy, parent, srcs.result);
    }

    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");
}

Three cases:

1. smart_holder target: handled first via smart_holder_from_shared_ptr (unchanged).
2. std::shared_ptr holder target: proceeds to cast_holder (the original path, now gated).
3. Any other holder (e.g., unique_ptr for enums): falls back to reference_internal if parent is available, otherwise throws.

Why the reference_internal fallback is safe

Traced through type_caster_generic::cast() with policy = reference_internal:

1. valueptr = src (raw pointer to the member inside the parent object).
2. wrapper->owned = false — pybind11 does NOT own this memory.
3. keep_alive_impl(inst, parent) — returned Python object holds a reference to parent, keeping the parent (and thus the member) alive.
4. tinfo->init_instance(wrapper, existing_holder) is called with existing_holder = nullptr.

For unique_ptr-held types, init_holder checks:

} else if (detail::always_construct_holder<holder_type>::value || inst->owned) {
    new (std::addressof(v_h.holder<holder_type>())) holder_type(v_h.value_ptr<type>());
    v_h.set_holder_constructed();
}

Since owned = false and always_construct_holder<unique_ptr<T>> defaults to false, the condition is false || false — the holder is never constructed. No unique_ptr wraps the pointer, so no double-free can occur.

On deallocation, pybind11 checks holder_constructed() (which is false), skips holder destruction, and since owned = false, doesn't call delete on the raw pointer. The raw pointer remains valid because keep_alive_impl guarantees the parent outlives the returned object.

The shared_ptr temporary lifetime is not a concern

The aliasing shared_ptr<D> returned by the def_readwrite lambda is a temporary. After cast() returns, it is destroyed. But the fallback path uses only the raw pointer (via srcs) and reference_internal to parent, so it does not depend on the shared_ptr's lifetime at all. The member's lifetime is managed by the parent Python object.

Generality vs. PR #6003

PR #6003 fixes the problem at the def_readwrite call site only — it checks D's holder type at bind time and skips the aliasing shared_ptr getter when incompatible.

PR #6008 fixes the problem at the shared_ptr-to-Python cast layer, catching all cases where a shared_ptr<T> is converted to Python for a type whose holder isn't shared_ptr or smart_holder. This includes:

• def_readwrite (the reported bug)
• Any user function that returns shared_ptr<T> where T has a unique_ptr holder
• Any other code path that creates a shared_ptr to a non-shared_ptr-holder type

This is exactly what @oremanj suggested:

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?

Notes for discussion

1. Hardcoded reference_internal in the fallback: The fallback always uses reference_internal regardless of the originally requested policy. For def_readwrite this is correct (policy is already reference_internal). For other callers with different policies, this is a silent override. However, the old behavior was UB, so any defined behavior is an improvement.

2. The throw case (no parent): If someone returns shared_ptr<EnumType> from a function without a parent object, the fix throws a clear cast_error. Previously this was UB. The user would need to change their code (e.g., return by value instead of shared_ptr, or change the holder type).

3. Enum semantics: For enum members accessed via def_readwrite, the returned Python object is a new instance wrapper around the raw pointer, not one of the pre-registered enum singletons. However, since py::enum_ inherits from Python's IntEnum, equality comparison (==) is by value, so obj.level == TinyLevel.A works correctly.

[virtuald]

The throw case (no parent): If someone returns shared_ptr from a function without a parent object, the fix throws a clear cast_error. Previously this was UB. The user would need to change their code (e.g., return by value instead of shared_ptr, or change the holder type).

This actually is checked at compile time:

m.def("getSharedEnumAB", []() -> std::shared_ptr<EnumAB> {
    return std::make_shared<EnumAB>();
});

/work/pybind11/include/pybind11/cast.h:973:61: error: static assertion failed: Holder classes are only supported for custom types
  973 |     static_assert(std::is_base_of<base, type_caster<type>>::value,

I did add a test for the "returning shared ptr when holder is unique_ptr" case though.

[rwgk]

@virtuald I merged master and reverted b299f32, to see if that gets us back to a successful CI.

[virtuald]

Sorry, I was going to get back to it this evening but got caught up with other things. I didn't add the counterexample as a test yet, but I think it should go in here -- I'll go ahead and add it since CI failed already.

[rwgk]

Sorry, I was going to get back to it this evening but got caught up with other things.

No worries. I just thought I give it a little nudge after merging three other PRs.

I didn't add the counterexample as a test yet, but I think it should go in here

Absolutely, thanks!

[rwgk]

Looks great to me, thanks for the fix!

I'm not sure if you consider this the final state of the PR: please let me know and I'll merge as soon as I see your response.

[virtuald]

I am satisfied that this fixes my issue.