fix(smart_holder): keep void-cast semantics in from_unique_ptr (#6163)
* fix(smart_holder): keep void-cast semantics in from_unique_ptr
Fixes item 4 of #6159.
* test: cover base unique_ptr factories and trampoline lifetime
* test: limit prompt trampoline destruction check to CPython
* docs: clarify smart_holder void-cast ownership safeguard
---------
Co-authored-by: Ralf W. Grosse-Kunstleve <rgrossekunst@nvidia.com>
diff --git a/include/pybind11/detail/init.h b/include/pybind11/detail/init.h
index 5f9e925..cce3eb5 100644
--- a/include/pybind11/detail/init.h
+++ b/include/pybind11/detail/init.h
@@ -206,8 +206,8 @@
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,
diff --git a/include/pybind11/detail/struct_smart_holder.h b/include/pybind11/detail/struct_smart_holder.h
index 57cd843..27e8591 100644
--- a/include/pybind11/detail/struct_smart_holder.h
+++ b/include/pybind11/detail/struct_smart_holder.h
@@ -43,7 +43,10 @@
* 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 @@
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 @@
? 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 @@
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;
diff --git a/tests/test_class_sh_trampoline_shared_from_this.cpp b/tests/test_class_sh_trampoline_shared_from_this.cpp
index dc6bf1c..f21bd5d 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 @@
.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; }));
diff --git a/tests/test_class_sh_trampoline_shared_from_this.py b/tests/test_class_sh_trampoline_shared_from_this.py
index c59d0d1..965ee20 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 @@
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"
