diff --git a/include/pybind11/detail/init.h b/include/pybind11/detail/init.h index 5f9e925c3f..cce3eb5607 100644 --- a/include/pybind11/detail/init.h +++ b/include/pybind11/detail/init.h @@ -206,8 +206,8 @@ void construct(value_and_holder &v_h, Alias &&result, bool) { template smart_holder init_smart_holder_from_unique_ptr(std::unique_ptr &&unq_ptr, bool void_cast_raw_ptr) { - void *void_ptr = void_cast_raw_ptr ? static_cast(unq_ptr.get()) : nullptr; - return smart_holder::from_unique_ptr(std::move(unq_ptr), void_ptr); + return smart_holder::from_unique_ptr( + std::move(unq_ptr), /*mi_subobject_ptr*/ nullptr, void_cast_raw_ptr); } template static smart_holder from_unique_ptr(std::unique_ptr &&unq_ptr, - void *mi_subobject_ptr = nullptr) { + void *mi_subobject_ptr = nullptr, + bool void_cast_raw_ptr = false) { smart_holder hld; hld.rtti_uqp_del = &typeid(D); hld.vptr_is_using_std_default_delete = uqp_del_is_std_default_delete(); @@ -344,7 +348,19 @@ struct smart_holder { ? make_guarded_std_default_delete(true) : make_guarded_custom_deleter(std::move(unq_ptr.get_deleter()), true); // Critical: construct owner with pointer we intend to delete - std::shared_ptr owner(unq_ptr.get(), std::move(gd)); + std::shared_ptr owner; + if (void_cast_raw_ptr) { + // Passing a `T *` to the `shared_ptr` constructor would connect the + // `std::enable_shared_from_this` machinery to this control block, even for + // a `shared_ptr`. For trampolines, this control block must stay invisible + // (see the `void_cast_raw_ptr` comment near the top of this file). + // Cast the raw pointer to `void *` before construction; converting the resulting + // `shared_ptr` to `shared_ptr` afterwards would be too late. + owner = std::shared_ptr(static_cast(unq_ptr.get()), std::move(gd)); + } else { + owner + = std::static_pointer_cast(std::shared_ptr(unq_ptr.get(), std::move(gd))); + } // Relinquish ownership only after successful construction of owner (void) unq_ptr.release(); @@ -366,7 +382,7 @@ struct smart_holder { if (mi_subobject_ptr) { hld.vptr = std::shared_ptr(owner, mi_subobject_ptr); } else { - hld.vptr = std::static_pointer_cast(owner); + hld.vptr = std::move(owner); } hld.is_populated = true; diff --git a/tests/test_class_sh_trampoline_shared_from_this.cpp b/tests/test_class_sh_trampoline_shared_from_this.cpp index dc6bf1c72a..f21bd5d975 100644 --- a/tests/test_class_sh_trampoline_shared_from_this.cpp +++ b/tests/test_class_sh_trampoline_shared_from_this.cpp @@ -114,6 +114,13 @@ TEST_SUBMODULE(class_sh_trampoline_shared_from_this, m) { .def(py::init([](const std::string &history, int) { return std::make_shared(history); })) + // The second argument is only used to make this overload unambiguous. + .def(py::init([](const std::string &history, const std::string &) { + return std::unique_ptr(new SftTrampoline(history)); + })) + .def(py::init([](const std::string &history, const std::string &, bool) { + return std::unique_ptr(new SftTrampoline(history)); + })) .def_readonly("history", &Sft::history) // This leads to multiple entries in registered_instances: .def(py::init([](const std::shared_ptr &existing) { return existing; })); diff --git a/tests/test_class_sh_trampoline_shared_from_this.py b/tests/test_class_sh_trampoline_shared_from_this.py index c59d0d1dbb..965ee20057 100644 --- a/tests/test_class_sh_trampoline_shared_from_this.py +++ b/tests/test_class_sh_trampoline_shared_from_this.py @@ -162,6 +162,40 @@ def test_pure_cpp_sft_raw_ptr(make_f): assert obj.history == "PureCppSft_Stash1AddSharedFromThis" +@pytest.mark.parametrize("factory_args", [("unique_ptr",), ("unique_ptr", True)]) +def test_unique_ptr_factory_and_stash_via_shared_from_this(factory_args): + # Exercises that the smart_holder vptr stays invisible to the shared_from_this + # mechanism, also for a trampoline made by a unique_ptr factory. + class PySftUniquePtr(m.Sft): + def __init__(self, history): + super().__init__(history, *factory_args) + + obj = PySftUniquePtr("PySftUniquePtr") + assert obj.history == "PySftUniquePtr" + stash1 = m.SftSharedPtrStash(1) + with pytest.raises(RuntimeError) as exc_info: + stash1.AddSharedFromThis(obj) + assert str(exc_info.value) == "bad_weak_ptr" + stash1.Add(obj) + assert obj.history == "PySftUniquePtr_Stash1Add" + assert stash1.use_count(0) == 1 + stash1.AddSharedFromThis(obj) + assert obj.history == "PySftUniquePtr_Stash1Add_Stash1AddSharedFromThis" + assert stash1.use_count(0) == 2 + assert stash1.use_count(1) == 2 + + obj_ref = weakref.ref(obj) + del obj + pytest.gc_collect() + assert obj_ref() is not None + assert obj_ref().history == "PySftUniquePtr_Stash1Add_Stash1AddSharedFromThis" + stash1.Clear() + pytest.gc_collect() + # As in the lifetime tests below, only CPython guarantees prompt destruction. + if not env.PYPY and not env.GRAALPY: + assert obj_ref() is None + + def test_multiple_registered_instances_for_same_pointee(): obj0 = PySft("PySft") obj0.attachment_in_dict = "Obj0"