diff --git a/docs/advanced/classes.rst b/docs/advanced/classes.rst index 7c0ec10e79..5f11215d06 100644 --- a/docs/advanced/classes.rst +++ b/docs/advanced/classes.rst @@ -428,6 +428,51 @@ Python side by allowing the Python function to return ``None`` or an ``int``: return false; // Alternatively return MyClass::myMethod(value); } +Avoiding Inheritance Slicing and ``std::weak_ptr`` surprises +------------------------------------------------------------ + +When working with classes that use virtual functions and are subclassed +in Python, special care must be taken when converting Python objects to +``std::shared_ptr``. Depending on whether the class uses a plain +``std::shared_ptr`` holder or ``py::smart_holder``, the resulting +``shared_ptr`` may either allow inheritance slicing or lead to potentially +surprising behavior when constructing ``std::weak_ptr`` instances. + +This section explains how ``std::shared_ptr`` and ``py::smart_holder`` manage +object lifetimes differently, how these differences affect trampoline-derived +objects, and what options are available to achieve the situation-specific +desired behavior. + +When using ``std::shared_ptr`` as the holder type, converting a Python object +to a ``std::shared_ptr`` (e.g., ``obj.cast>()``, or simply +passing the Python object as an argument to a ``.def()``-ed function) returns +a ``shared_ptr`` that shares ownership with the original ``class_`` holder, +usually preserving object lifetime. However, for Python classes that derive from +a trampoline, if the Python object is destroyed, only the base C++ object may +remain alive, leading to inheritance slicing +(see `#1333 `_). + +In contrast, with ``py::smart_holder``, converting a Python object to +a ``std::shared_ptr`` returns a new ``shared_ptr`` with an independent +control block that keeps the derived Python object alive. This avoids +inheritance slicing but can lead to unintended behavior when creating +``std::weak_ptr`` instances +(see `#5623 `_). + +If it is necessary to obtain a ``std::weak_ptr`` that shares the control block +with the ``smart_holder``—at the cost of reintroducing potential inheritance +slicing—you can use ``py::potentially_slicing_weak_ptr(obj)``. + +When precise lifetime management of derived Python objects is important, +using a Python-side ``weakref`` is the most reliable approach, as it avoids +both inheritance slicing and unintended interactions with ``std::weak_ptr`` +semantics in C++. + +.. seealso:: + + * :func:`potentially_slicing_weak_ptr` C++ documentation + * :file:`tests/test_potentially_slicing_weak_ptr.cpp` + .. _custom_constructors: diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 8952cf0943..3bedb9d1a3 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -983,6 +983,17 @@ struct copyable_holder_caster< return shared_ptr_storage; } + std::weak_ptr potentially_slicing_weak_ptr() { + if (typeinfo->holder_enum_v == detail::holder_enum_t::smart_holder) { + // Reusing shared_ptr code to minimize code complexity. + shared_ptr_storage + = sh_load_helper.load_as_shared_ptr(value, + /*responsible_parent=*/nullptr, + /*force_potentially_slicing_shared_ptr=*/true); + } + return shared_ptr_storage; + } + static handle cast(const std::shared_ptr &src, return_value_policy policy, handle parent) { const auto *ptr = src.get(); @@ -1077,6 +1088,54 @@ struct copyable_holder_caster< template class type_caster> : public copyable_holder_caster> {}; +PYBIND11_NAMESPACE_END(detail) + +/// Return a std::shared_ptr with the SAME CONTROL BLOCK as the std::shared_ptr owned by the +/// class_ holder. For class_-wrapped types with trampolines, the returned std::shared_ptr +/// does NOT keep any derived Python objects alive (see issue #1333). +/// +/// For class_-wrapped types using std::shared_ptr as the holder, the following expressions +/// produce equivalent results (see tests/test_potentially_slicing_weak_ptr.cpp,py): +/// +/// - obj.cast>() +/// - py::potentially_slicing_weak_ptr(obj).lock() +/// +/// For class_-wrapped types with trampolines and using py::smart_holder, obj.cast<>() +/// produces a std::shared_ptr that keeps any derived Python objects alive for its own lifetime, +/// but this is achieved by introducing a std::shared_ptr control block that is independent of +/// the one owned by the py::smart_holder. This can lead to surprising std::weak_ptr behavior +/// (see issue #5623). An easy solution is to use py::potentially_slicing_weak_ptr<>(obj), +/// as exercised in tests/test_potentially_slicing_weak_ptr.cpp,py (look for +/// "set_wp_potentially_slicing"). Note, however, that this reintroduces the inheritance +/// slicing issue (see issue #1333). The ideal — but usually more involved — solution is to use +/// a Python weakref to the derived Python object, instead of a C++ base-class std::weak_ptr. +/// +/// It is not possible (at least no known approach exists at the time of this writing) to +/// simultaneously achieve both desirable properties: +/// +/// - the same std::shared_ptr control block as the class_ holder +/// - automatic lifetime extension of any derived Python objects +/// +/// The reason is that this would introduce a reference cycle that cannot be garbage collected: +/// +/// - the derived Python object owns the class_ holder +/// - the class_ holder owns the std::shared_ptr +/// - the std::shared_ptr would own a reference to the derived Python object, +/// completing the cycle +template +std::weak_ptr potentially_slicing_weak_ptr(handle obj) { + detail::make_caster> caster; + if (caster.load(obj, /*convert=*/true)) { + return caster.potentially_slicing_weak_ptr(); + } + const char *obj_type_name = detail::obj_class_name(obj.ptr()); + throw type_error("\"" + std::string(obj_type_name) + + "\" object is not convertible to std::weak_ptr (with T = " + type_id() + + ")"); +} + +PYBIND11_NAMESPACE_BEGIN(detail) + // SMART_HOLDER_BAKEIN_FOLLOW_ON: Rewrite comment, with reference to unique_ptr specialization. /// Type caster for holder types like std::unique_ptr. /// Please consider the SFINAE hook an implementation detail, as explained diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index 1105c08e7c..ceef18cb4a 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -759,7 +759,11 @@ struct load_helper : value_and_holder_helper { } std::shared_ptr load_as_shared_ptr(void *void_raw_ptr, - handle responsible_parent = nullptr) const { + handle responsible_parent = nullptr, + // to support py::potentially_slicing_weak_ptr + // with minimal added code complexity: + bool force_potentially_slicing_shared_ptr + = false) const { if (!have_holder()) { return nullptr; } @@ -774,7 +778,7 @@ struct load_helper : value_and_holder_helper { throw std::runtime_error("Non-owning holder (load_as_shared_ptr)."); } auto *type_raw_ptr = static_cast(void_raw_ptr); - if (python_instance_is_alias) { + if (python_instance_is_alias && !force_potentially_slicing_shared_ptr) { auto *vptr_gd_ptr = std::get_deleter(hld.vptr); if (vptr_gd_ptr != nullptr) { std::shared_ptr released_ptr = vptr_gd_ptr->released_ptr.lock(); diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 69976f745a..29172b6ce6 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -161,6 +161,7 @@ set(PYBIND11_TEST_FILES test_opaque_types test_operator_overloading test_pickling + test_potentially_slicing_weak_ptr test_python_multiple_inheritance test_pytypes test_sequences_and_iterators diff --git a/tests/test_potentially_slicing_weak_ptr.cpp b/tests/test_potentially_slicing_weak_ptr.cpp new file mode 100644 index 0000000000..01b147faff --- /dev/null +++ b/tests/test_potentially_slicing_weak_ptr.cpp @@ -0,0 +1,168 @@ +// Copyright (c) 2025 The pybind Community. +// All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +#include "pybind11_tests.h" + +#include + +namespace pybind11_tests { +namespace potentially_slicing_weak_ptr { + +template // Using int as a trick to easily generate multiple types. +struct VirtBase { + virtual ~VirtBase() = default; + virtual int get_code() { return 100; } +}; + +using VirtBaseSH = VirtBase<0>; // for testing with py::smart_holder +using VirtBaseSP = VirtBase<1>; // for testing with std::shared_ptr as holder + +// Similar to trampoline_self_life_support +struct trampoline_is_alive_simple { + std::uint64_t magic_token = 197001010000u; + + trampoline_is_alive_simple() = default; + + ~trampoline_is_alive_simple() { magic_token = 20380118191407u; } + + trampoline_is_alive_simple(const trampoline_is_alive_simple &other) = default; + trampoline_is_alive_simple(trampoline_is_alive_simple &&other) noexcept + : magic_token(other.magic_token) { + other.magic_token = 20380118191407u; + } + + trampoline_is_alive_simple &operator=(const trampoline_is_alive_simple &) = delete; + trampoline_is_alive_simple &operator=(trampoline_is_alive_simple &&) = delete; +}; + +template +const char *determine_trampoline_state(const std::shared_ptr &sp) { + if (!sp) { + return "sp nullptr"; + } + auto *tias = dynamic_cast(sp.get()); + if (!tias) { + return "dynamic_cast failed"; + } + if (tias->magic_token == 197001010000u) { + return "trampoline alive"; + } + if (tias->magic_token == 20380118191407u) { + return "trampoline DEAD"; + } + return "UNDEFINED BEHAVIOR"; +} + +struct PyVirtBaseSH : VirtBaseSH, py::trampoline_self_life_support, trampoline_is_alive_simple { + using VirtBaseSH::VirtBaseSH; + int get_code() override { PYBIND11_OVERRIDE(int, VirtBaseSH, get_code); } +}; + +struct PyVirtBaseSP : VirtBaseSP, trampoline_is_alive_simple { // self-life-support not available + using VirtBaseSP::VirtBaseSP; + int get_code() override { PYBIND11_OVERRIDE(int, VirtBaseSP, get_code); } +}; + +template +std::shared_ptr rtrn_obj_cast_shared_ptr(py::handle obj) { + return obj.cast>(); +} + +// There is no type_caster>, and to minimize code complexity +// we do not want to add one, therefore we have to return a shared_ptr here. +template +std::shared_ptr rtrn_potentially_slicing_shared_ptr(py::handle obj) { + return py::potentially_slicing_weak_ptr(obj).lock(); +} + +template +struct SpOwner { + void set_sp(const std::shared_ptr &sp_) { sp = sp_; } + + int get_code() const { + if (!sp) { + return -888; + } + return sp->get_code(); + } + + const char *get_trampoline_state() const { return determine_trampoline_state(sp); } + +private: + std::shared_ptr sp; +}; + +template +struct WpOwner { + void set_wp(const std::weak_ptr &wp_) { wp = wp_; } + + int get_code() const { + auto sp = wp.lock(); + if (!sp) { + return -999; + } + return sp->get_code(); + } + + const char *get_trampoline_state() const { return determine_trampoline_state(wp.lock()); } + +private: + std::weak_ptr wp; +}; + +template +void wrap(py::module_ &m, + const char *roc_pyname, + const char *rps_pyname, + const char *spo_pyname, + const char *wpo_pyname) { + m.def(roc_pyname, rtrn_obj_cast_shared_ptr); + m.def(rps_pyname, rtrn_potentially_slicing_shared_ptr); + + py::classh>(m, spo_pyname) + .def(py::init<>()) + .def("set_sp", &SpOwner::set_sp) + .def("get_code", &SpOwner::get_code) + .def("get_trampoline_state", &SpOwner::get_trampoline_state); + + py::classh>(m, wpo_pyname) + .def(py::init<>()) + .def("set_wp", + [](WpOwner &self, py::handle obj) { + self.set_wp(obj.cast>()); + }) + .def("set_wp_potentially_slicing", + [](WpOwner &self, py::handle obj) { + self.set_wp(py::potentially_slicing_weak_ptr(obj)); + }) + .def("get_code", &WpOwner::get_code) + .def("get_trampoline_state", &WpOwner::get_trampoline_state); +} + +} // namespace potentially_slicing_weak_ptr +} // namespace pybind11_tests + +using namespace pybind11_tests::potentially_slicing_weak_ptr; + +TEST_SUBMODULE(potentially_slicing_weak_ptr, m) { + py::classh(m, "VirtBaseSH") + .def(py::init<>()) + .def("get_code", &VirtBaseSH::get_code); + + py::class_, PyVirtBaseSP>(m, "VirtBaseSP") + .def(py::init<>()) + .def("get_code", &VirtBaseSP::get_code); + + wrap(m, + "SH_rtrn_obj_cast_shared_ptr", + "SH_rtrn_potentially_slicing_shared_ptr", + "SH_SpOwner", + "SH_WpOwner"); + + wrap(m, + "SP_rtrn_obj_cast_shared_ptr", + "SP_rtrn_potentially_slicing_shared_ptr", + "SP_SpOwner", + "SP_WpOwner"); +} diff --git a/tests/test_potentially_slicing_weak_ptr.py b/tests/test_potentially_slicing_weak_ptr.py new file mode 100644 index 0000000000..d442f99a7f --- /dev/null +++ b/tests/test_potentially_slicing_weak_ptr.py @@ -0,0 +1,174 @@ +# Copyright (c) 2025 The pybind Community. +# All rights reserved. Use of this source code is governed by a +# BSD-style license that can be found in the LICENSE file. + +# This module tests the interaction of pybind11's shared_ptr and smart_holder +# mechanisms with trampoline object lifetime management and inheritance slicing. +# +# The following combinations are covered: +# +# - Holder type: std::shared_ptr (class_ holder) vs. +# py::smart_holder +# - Conversion function: obj.cast>() vs. +# py::potentially_slicing_weak_ptr(obj) +# - Python object type: C++ base class vs. +# Python-derived trampoline class +# +# The tests verify +# +# - that casting or passing Python objects into functions returns usable +# std::shared_ptr instances. +# - that inheritance slicing occurs as expected in controlled cases +# (issue #1333). +# - that surprising weak_ptr behavior (issue #5623) can be reproduced when +# smart_holder is used. +# - that the trampoline object remains alive in all situations +# (no use-after-free) as long as the C++ shared_ptr exists. +# +# Where applicable, trampoline state is introspected to confirm whether the +# C++ object retains knowledge of the Python override or has fallen back to +# the base implementation. + +from __future__ import annotations + +import gc +import weakref + +import pytest + +import env +import pybind11_tests.potentially_slicing_weak_ptr as m + + +class PyDrvdSH(m.VirtBaseSH): + def get_code(self): + return 200 + + +class PyDrvdSP(m.VirtBaseSP): + def get_code(self): + return 200 + + +VIRT_BASE_TYPES = { + "SH": {100: m.VirtBaseSH, 200: PyDrvdSH}, + "SP": {100: m.VirtBaseSP, 200: PyDrvdSP}, +} + +RTRN_FUNCS = { + "SH": { + "oc": m.SH_rtrn_obj_cast_shared_ptr, + "ps": m.SH_rtrn_potentially_slicing_shared_ptr, + }, + "SP": { + "oc": m.SP_rtrn_obj_cast_shared_ptr, + "ps": m.SP_rtrn_potentially_slicing_shared_ptr, + }, +} + +SP_OWNER_TYPES = { + "SH": m.SH_SpOwner, + "SP": m.SP_SpOwner, +} + +WP_OWNER_TYPES = { + "SH": m.SH_WpOwner, + "SP": m.SP_WpOwner, +} + +GC_IS_RELIABLE = not (env.PYPY or env.GRAALPY) + + +@pytest.mark.parametrize("expected_code", [100, 200]) +@pytest.mark.parametrize("rtrn_kind", ["oc", "ps"]) +@pytest.mark.parametrize("holder_kind", ["SH", "SP"]) +def test_rtrn_obj_cast_shared_ptr(holder_kind, rtrn_kind, expected_code): + obj = VIRT_BASE_TYPES[holder_kind][expected_code]() + ptr = RTRN_FUNCS[holder_kind][rtrn_kind](obj) + assert ptr.get_code() == expected_code + objref = weakref.ref(obj) + del obj + gc.collect() + assert ptr.get_code() == expected_code # the ptr Python object keeps obj alive + assert objref() is not None + del ptr + gc.collect() + if GC_IS_RELIABLE: + assert objref() is None + + +@pytest.mark.parametrize("expected_code", [100, 200]) +@pytest.mark.parametrize("holder_kind", ["SH", "SP"]) +def test_with_sp_owner(holder_kind, expected_code): + spo = SP_OWNER_TYPES[holder_kind]() + assert spo.get_code() == -888 + assert spo.get_trampoline_state() == "sp nullptr" + + obj = VIRT_BASE_TYPES[holder_kind][expected_code]() + assert obj.get_code() == expected_code + + spo.set_sp(obj) + assert spo.get_code() == expected_code + expected_trampoline_state = ( + "dynamic_cast failed" if expected_code == 100 else "trampoline alive" + ) + assert spo.get_trampoline_state() == expected_trampoline_state + + del obj + gc.collect() + if holder_kind == "SH": + assert spo.get_code() == expected_code + elif GC_IS_RELIABLE: + assert ( + spo.get_code() == 100 + ) # see issue #1333 (inheritance slicing) and PR #5624 + assert spo.get_trampoline_state() == expected_trampoline_state + + +@pytest.mark.parametrize("expected_code", [100, 200]) +@pytest.mark.parametrize("set_meth", ["set_wp", "set_wp_potentially_slicing"]) +@pytest.mark.parametrize("holder_kind", ["SH", "SP"]) +def test_with_wp_owner(holder_kind, set_meth, expected_code): + wpo = WP_OWNER_TYPES[holder_kind]() + assert wpo.get_code() == -999 + assert wpo.get_trampoline_state() == "sp nullptr" + + obj = VIRT_BASE_TYPES[holder_kind][expected_code]() + assert obj.get_code() == expected_code + + getattr(wpo, set_meth)(obj) + if ( + holder_kind == "SP" + or expected_code == 100 + or set_meth == "set_wp_potentially_slicing" + ): + assert wpo.get_code() == expected_code + else: + assert wpo.get_code() == -999 # see issue #5623 (weak_ptr expired) and PR #5624 + if expected_code == 100: + expected_trampoline_state = "dynamic_cast failed" + elif holder_kind == "SH" and set_meth == "set_wp": + expected_trampoline_state = "sp nullptr" + else: + expected_trampoline_state = "trampoline alive" + assert wpo.get_trampoline_state() == expected_trampoline_state + + del obj + gc.collect() + if GC_IS_RELIABLE: + assert wpo.get_code() == -999 + + +def test_potentially_slicing_weak_ptr_not_convertible_error(): + with pytest.raises(Exception) as excinfo: + m.SH_rtrn_potentially_slicing_shared_ptr("") + assert str(excinfo.value) == ( + '"str" object is not convertible to std::weak_ptr' + " (with T = pybind11_tests::potentially_slicing_weak_ptr::VirtBase<0>)" + ) + with pytest.raises(Exception) as excinfo: + m.SP_rtrn_potentially_slicing_shared_ptr([]) + assert str(excinfo.value) == ( + '"list" object is not convertible to std::weak_ptr' + " (with T = pybind11_tests::potentially_slicing_weak_ptr::VirtBase<1>)" + )