fix(smart_holder): keep void-cast semantics in from_unique_ptr

include/pybind11/detail/init.h

@@ -206,8 +206,8 @@ void construct(value_and_holder &v_h, Alias<Class> &&result, bool) {
 template <typename T, typename D>
 smart_holder init_smart_holder_from_unique_ptr(std::unique_ptr<T, D> &&unq_ptr,
                                                bool void_cast_raw_ptr) {
-    void *void_ptr = void_cast_raw_ptr ? static_cast<void *>(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 <typename Class,

include/pybind11/detail/struct_smart_holder.h

@@ -43,7 +43,10 @@ High-level aspects:
 
 * The `void_cast_raw_ptr` option is needed to make the `smart_holder` `vptr`
   member invisible to the `shared_from_this` mechanism, in case the lifetime
-  of a `PyObject` is tied to the pointee.
+  of a `PyObject` is tied to the pointee. This control block cannot itself keep
+  the Python object alive: that would create a reference cycle through the
+  object's own holder that Python's garbage collector cannot detect.
+  See https://github.com/pybind/pybind11/pull/3023 for the original rationale.
 */
 
 #pragma once
@@ -333,7 +336,8 @@ struct smart_holder {
 
     template <typename T, typename D>
     static smart_holder from_unique_ptr(std::unique_ptr<T, D> &&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<T, D>();
@@ -344,7 +348,19 @@ struct smart_holder {
                   ? make_guarded_std_default_delete<T>(true)
                   : make_guarded_custom_deleter<T, D>(std::move(unq_ptr.get_deleter()), true);
         // Critical: construct owner with pointer we intend to delete
-        std::shared_ptr<T> owner(unq_ptr.get(), std::move(gd));
+        std::shared_ptr<void> 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<void>`. 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<void>` afterwards would be too late.
+            owner = std::shared_ptr<void>(static_cast<void *>(unq_ptr.get()), std::move(gd));
+        } else {
+            owner
+                = std::static_pointer_cast<void>(std::shared_ptr<T>(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<void>(owner, mi_subobject_ptr);
         } else {
-            hld.vptr = std::static_pointer_cast<void>(owner);
+            hld.vptr = std::move(owner);
         }
 
         hld.is_populated = true;

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<SftTrampoline>(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<SftTrampoline>(new SftTrampoline(history));
+        }))
+        .def(py::init([](const std::string &history, const std::string &, bool) {
+            return std::unique_ptr<Sft>(new SftTrampoline(history));
+        }))
         .def_readonly("history", &Sft::history)
         // This leads to multiple entries in registered_instances:
         .def(py::init([](const std::shared_ptr<Sft> &existing) { return existing; }));

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"