pybind/pybind11
1
0
Fork 0
mirror of https://github.com/pybind/pybind11.git synced 2026-03-14 20:27:47 +00:00
Code Issues Packages Projects Releases Wiki Activity

Fix smart_holder multiple/virtual inheritance bugs in shared_ptr and unique_ptr to-Python conversions (#5836)

Browse Source 
* ChatGPT-generated diamond virtual-inheritance test case.

* Report "virtual base at offset 0" but don't skip test.

* Remove Left/Right virtual default dtors, to resolve clang-tidy errors:

```
/__w/pybind11/pybind11/tests/test_class_sh_mi_thunks.cpp:44:13: error: prefer using 'override' or (rarely) 'final' instead of 'virtual' [modernize-use-override,-warnings-as-errors]
   44 |     virtual ~Left() = default;
      |     ~~~~~~~ ^
      |                     override
/__w/pybind11/pybind11/tests/test_class_sh_mi_thunks.cpp:48:13: error: prefer using 'override' or (rarely) 'final' instead of 'virtual' [modernize-use-override,-warnings-as-errors]
   48 |     virtual ~Right() = default;
      |     ~~~~~~~ ^
      |                      override
```

* Add assert(ptr) in register_instance_impl, deregister_instance_impl

* Proper bug fix

* Also exercise smart_holder_from_unique_ptr

* [skip ci] ChatGPT-generated bug fix: smart_holder::from_unique_ptr()

* Exception-safe ownership transfer from unique_ptr to shared_ptr

ChatGPT:

* shared_ptr’s ctor can throw (control-block alloc). Using get() keeps unique_ptr owning the memory if that happens, so no leak.

* Only after the shared_ptr is successfully constructed do you release(), transferring ownership exactly once.

* [skip ci] Rename alias_ptr to mi_subobject_ptr to distinguish from trampoline code (which often uses the term "alias", too)

* [skip ci] Also exercise smart_holder::from_raw_ptr_take_ownership

* [skip ci] Add st.first comments (generated by ChatGPT)

* [skip ci] Copy and extend (raw_ptr, unique_ptr) reproducer from PR #5796

* Some polishing: comments, add back Left/Right dtors for consistency within test_class_sh_mi_thunks.cpp

* explicitly default copy/move for VBase to silence -Wdeprecated-copy-with-dtor

* Resolve clang-tidy error:

```
/__w/pybind11/pybind11/tests/test_class_sh_mi_thunks.cpp:67:5: error: 'auto ptr' can be declared as 'auto *ptr' [readability-qualified-auto,-warnings-as-errors]
   67 |     auto ptr = new Diamond;
      |     ^~~~
      |     auto *
```

* Expand comment in `smart_holder::from_unique_ptr()`

* Better Left/Right padding to make it more likely that we avoid "all at offset 0". Clarify comment.

* Give up on `alignas(16)` to resolve MSVC warning:

```
       "D:\a\pybind11\pybind11\build\ALL_BUILD.vcxproj" (default target) (1) ->
       "D:\a\pybind11\pybind11\build\tests\pybind11_tests.vcxproj" (default target) (13) ->
       (ClCompile target) ->
         D:\a\pybind11\pybind11\tests\test_class_sh_mi_thunks.cpp(70,17): warning C4316: 'test_class_sh_mi_thunks::Diamond': object allocated on the heap may not be aligned 16 [D:\a\pybind11\pybind11\build\tests\pybind11_tests.vcxproj]
         D:\a\pybind11\pybind11\tests\test_class_sh_mi_thunks.cpp(80,43): warning C4316: 'test_class_sh_mi_thunks::Diamond': object allocated on the heap may not be aligned 16 [D:\a\pybind11\pybind11\build\tests\pybind11_tests.vcxproj]
         C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.44.35207\include\memory(2913,46): warning C4316: 'std::_Ref_count_obj2<_Ty>': object allocated on the heap may not be aligned 16 [D:\a\pybind11\pybind11\build\tests\pybind11_tests.vcxproj]
       C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.44.35207\include\memory(2913,46): warning C4316:         with [D:\a\pybind11\pybind11\build\tests\pybind11_tests.vcxproj]
       C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.44.35207\include\memory(2913,46): warning C4316:         [ [D:\a\pybind11\pybind11\build\tests\pybind11_tests.vcxproj]
       C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.44.35207\include\memory(2913,46): warning C4316:             _Ty=test_class_sh_mi_thunks::Diamond [D:\a\pybind11\pybind11\build\tests\pybind11_tests.vcxproj]
       C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.44.35207\include\memory(2913,46): warning C4316:         ] [D:\a\pybind11\pybind11\build\tests\pybind11_tests.vcxproj]
         D:\a\pybind11\pybind11\include\pybind11\detail\init.h(77,21): warning C4316: 'test_class_sh_mi_thunks::Diamond': object allocated on the heap may not be aligned 16 [D:\a\pybind11\pybind11\build\tests\pybind11_tests.vcxproj]
```

The warning came from alignas(16) making Diamond over-aligned, while regular new/make_shared aren’t guaranteed to return 16-byte aligned memory on MSVC (hence C4316). I’ve removed the explicit alignment and switched to asymmetric payload sizes (char[4] vs char[24]), which still nudges MI layout without relying on over-alignment. This keeps the test goal and eliminates the warning across all MSVC builds. If we ever want to stress over-alignment explicitly, we can add aligned operator new/delete under __cpp_aligned_new, but that’s more than we need here.

* Rename test_virtual_base_at_offset_0() → test_virtual_base_not_at_offset_0() and replace pytest.skip() with assert. Add helpful comment for future maintainers.
This commit is contained in:
Ralf W. Grosse-Kunstleve
2025-10-01 11:21:47 -07:00
committed by GitHub
parent 0161da9d6d
commit 4dc33d6524
5 changed files with 228 additions and 16 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
include/pybind11/detail/class.h
View File

@@ -334,6 +334,7 @@ inline void enable_try_inc_ref(PyObject *obj) {
#endif
inline bool register_instance_impl(void *ptr, instance *self) {
assert(ptr);
#ifdef Py_GIL_DISABLED
enable_try_inc_ref(reinterpret_cast<PyObject *>(self));
#endif
@@ -341,6 +342,7 @@ inline bool register_instance_impl(void *ptr, instance *self) {
return true; // unused, but gives the same signature as the deregister func
}
inline bool deregister_instance_impl(void *ptr, instance *self) {
assert(ptr);
return with_instance_map(ptr, [&](instance_map &instances) {
auto range = instances.equal_range(ptr);
for (auto it = range.first; it != range.second; ++it) {

44
include/pybind11/detail/struct_smart_holder.h
View File

@@ -339,22 +339,42 @@ struct smart_holder {
template <typename T, typename D>
static smart_holder from_unique_ptr(std::unique_ptr<T, D> &&unq_ptr,
void *void_ptr = nullptr) {
void *mi_subobject_ptr = nullptr) {
smart_holder hld;
hld.rtti_uqp_del = &typeid(D);
hld.vptr_is_using_std_default_delete = uqp_del_is_std_default_delete<T, D>();
guarded_delete gd{nullptr, false};
if (hld.vptr_is_using_std_default_delete) {
gd = make_guarded_std_default_delete<T>(true);
} else {
gd = make_guarded_custom_deleter<T, D>(std::move(unq_ptr.get_deleter()), true);
}
if (void_ptr != nullptr) {
hld.vptr.reset(void_ptr, std::move(gd));
} else {
hld.vptr.reset(unq_ptr.get(), std::move(gd));
}
// Build the owning control block on the *real object start* (T*).
guarded_delete gd
= hld.vptr_is_using_std_default_delete
? 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));
// Relinquish ownership only after successful construction of owner
(void) unq_ptr.release();
// Publish either the MI/VI subobject pointer (if provided) or the full object.
// Why this is needed:
// * The `owner` shared_ptr must always manage the true object start (T*).
// That ensures the deleter is invoked on a valid object header, so the
// virtual destructor can dispatch safely (critical on MSVC with virtual
// inheritance, where base subobjects are not at offset 0).
// * However, pybind11 needs to *register* and expose the subobject pointer
// appropriate for the type being bound.
// This pointer may differ from the T* object start under multiple/virtual
// inheritance.
// This is achieved by using an aliasing shared_ptr<void>:
// - `owner` retains lifetime of the actual T* object start for deletion.
// - `vptr` points at the adjusted subobject (mi_subobject_ptr), giving
// Python the correct identity/registration address.
// If no subobject pointer is passed, we simply publish the full object.
if (mi_subobject_ptr) {
hld.vptr = std::shared_ptr<void>(owner, mi_subobject_ptr);
} else {
hld.vptr = std::static_pointer_cast<void>(owner);
}
hld.is_populated = true;
return hld;
}

10
include/pybind11/detail/type_caster_base.h
View File

@@ -576,6 +576,8 @@ handle smart_holder_from_unique_ptr(std::unique_ptr<T, D> &&src,
if (!src) {
return none().release();
}
// st.first is the subobject pointer appropriate for tinfo (may differ from src.get()
// under MI/VI). Use this for Python identity/registration, but keep ownership on T*.
void *src_raw_void_ptr = const_cast<void *>(st.first);
assert(st.second != nullptr);
const detail::type_info *tinfo = st.second;
@@ -657,9 +659,10 @@ handle smart_holder_from_shared_ptr(const std::shared_ptr<T> &src,
return none().release();
}
auto src_raw_ptr = src.get();
// st.first is the subobject pointer appropriate for tinfo (may differ from src.get()
// under MI/VI). Use this for Python identity/registration, but keep ownership on T*.
void *src_raw_void_ptr = const_cast<void *>(st.first);
assert(st.second != nullptr);
void *src_raw_void_ptr = static_cast<void *>(src_raw_ptr);
const detail::type_info *tinfo = st.second;
if (handle existing_inst = find_registered_python_instance(src_raw_void_ptr, tinfo)) {
// PYBIND11:REMINDER: MISSING: Enforcement of consistency with existing smart_holder.
@@ -673,8 +676,7 @@ handle smart_holder_from_shared_ptr(const std::shared_ptr<T> &src,
void *&valueptr = values_and_holders(inst_raw_ptr).begin()->value_ptr();
valueptr = src_raw_void_ptr;
auto smhldr
= smart_holder::from_shared_ptr(std::shared_ptr<void>(src, const_cast<void *>(st.first)));
auto smhldr = smart_holder::from_shared_ptr(std::shared_ptr<void>(src, src_raw_void_ptr));
tinfo->init_instance(inst_raw_ptr, static_cast<const void *>(&smhldr));
if (policy == return_value_policy::reference_internal) {

137
tests/test_class_sh_mi_thunks.cpp
View File

@@ -10,6 +10,8 @@ namespace test_class_sh_mi_thunks {
// C++ vtables - Part 2 - Multiple Inheritance
// ... the compiler creates a 'thunk' method that corrects `this` ...
// This test was added under PR #4380
struct Base0 {
virtual ~Base0() = default;
Base0() = default;
@@ -30,6 +32,110 @@ struct Derived : Base1, Base0 {
Derived(const Derived &) = delete;
};
// ChatGPT-generated Diamond added under PR #5836
struct VBase {
VBase() = default;
VBase(const VBase &) = default; // silence -Wdeprecated-copy-with-dtor
VBase &operator=(const VBase &) = default;
VBase(VBase &&) = default;
VBase &operator=(VBase &&) = default;
virtual ~VBase() = default;
virtual int ping() const { return 1; }
int vbase_tag = 42; // ensure it's not empty
};
// Make the virtual bases non-empty and (likely) differently sized.
// The test does *not* require different sizes; we only want to avoid "all at offset 0".
// If a compiler/ABI still places the virtual base at offset 0, our test logs that via
// test_virtual_base_at_offset_0() and continues.
struct Left : virtual VBase {
char pad_l[4]; // small, typically 4 + padding
~Left() override = default;
};
struct Right : virtual VBase {
char pad_r[24]; // larger, to differ from Left
~Right() override = default;
};
struct Diamond : Left, Right {
Diamond() = default;
Diamond(const Diamond &) = default;
~Diamond() override = default;
int ping() const override { return 7; }
int self_tag = 99;
};
VBase *make_diamond_as_vbase_raw_ptr() {
auto *ptr = new Diamond;
return ptr; // upcast
}
std::shared_ptr<VBase> make_diamond_as_vbase_shared_ptr() {
auto shptr = std::make_shared<Diamond>();
return shptr; // upcast
}
std::unique_ptr<VBase> make_diamond_as_vbase_unique_ptr() {
auto uqptr = std::unique_ptr<Diamond>(new Diamond);
return uqptr; // upcast
}
// For diagnostics
struct DiamondAddrs {
uintptr_t as_self;
uintptr_t as_vbase;
uintptr_t as_left;
uintptr_t as_right;
};
DiamondAddrs diamond_addrs() {
auto sp = std::make_shared<Diamond>();
return DiamondAddrs{reinterpret_cast<uintptr_t>(sp.get()),
reinterpret_cast<uintptr_t>(static_cast<VBase *>(sp.get())),
reinterpret_cast<uintptr_t>(static_cast<Left *>(sp.get())),
reinterpret_cast<uintptr_t>(static_cast<Right *>(sp.get()))};
}
// Animal-Cat-Tiger reproducer copied from PR #5796
// clone_raw_ptr, clone_unique_ptr added under PR #5836
class Animal {
public:
Animal() = default;
Animal(const Animal &) = default;
Animal &operator=(const Animal &) = default;
virtual Animal *clone_raw_ptr() const = 0;
virtual std::shared_ptr<Animal> clone_shared_ptr() const = 0;
virtual std::unique_ptr<Animal> clone_unique_ptr() const = 0;
virtual ~Animal() = default;
};
class Cat : virtual public Animal {
public:
Cat() = default;
Cat(const Cat &) = default;
Cat &operator=(const Cat &) = default;
~Cat() override = default;
};
class Tiger : virtual public Cat {
public:
Tiger() = default;
Tiger(const Tiger &) = default;
Tiger &operator=(const Tiger &) = default;
~Tiger() override = default;
Animal *clone_raw_ptr() const override {
return new Tiger(*this); // upcast
}
std::shared_ptr<Animal> clone_shared_ptr() const override {
return std::make_shared<Tiger>(*this); // upcast
}
std::unique_ptr<Animal> clone_unique_ptr() const override {
return std::unique_ptr<Tiger>(new Tiger(*this)); // upcast
}
};
} // namespace test_class_sh_mi_thunks
TEST_SUBMODULE(class_sh_mi_thunks, m) {
@@ -90,4 +196,35 @@ TEST_SUBMODULE(class_sh_mi_thunks, m) {
}
return obj_der->vec.size();
});
py::class_<VBase, py::smart_holder>(m, "VBase").def("ping", &VBase::ping);
py::class_<Left, VBase, py::smart_holder>(m, "Left");
py::class_<Right, VBase, py::smart_holder>(m, "Right");
py::class_<Diamond, Left, Right, py::smart_holder>(m, "Diamond", py::multiple_inheritance())
.def(py::init<>())
.def("ping", &Diamond::ping);
m.def("make_diamond_as_vbase_raw_ptr",
&make_diamond_as_vbase_raw_ptr,
py::return_value_policy::take_ownership);
m.def("make_diamond_as_vbase_shared_ptr", &make_diamond_as_vbase_shared_ptr);
m.def("make_diamond_as_vbase_unique_ptr", &make_diamond_as_vbase_unique_ptr);
py::class_<DiamondAddrs, py::smart_holder>(m, "DiamondAddrs")
.def_readonly("as_self", &DiamondAddrs::as_self)
.def_readonly("as_vbase", &DiamondAddrs::as_vbase)
.def_readonly("as_left", &DiamondAddrs::as_left)
.def_readonly("as_right", &DiamondAddrs::as_right);
m.def("diamond_addrs", &diamond_addrs);
py::classh<Animal>(m, "Animal");
py::classh<Cat, Animal>(m, "Cat");
py::classh<Tiger, Cat>(m, "Tiger", py::multiple_inheritance())
.def(py::init<>())
.def("clone_raw_ptr", &Tiger::clone_raw_ptr)
.def("clone_shared_ptr", &Tiger::clone_shared_ptr)
.def("clone_unique_ptr", &Tiger::clone_unique_ptr);
}

51
tests/test_class_sh_mi_thunks.py
View File

@@ -51,3 +51,54 @@ def test_get_shared_vec_size_unique():
assert (
str(exc_info.value) == "Cannot disown external shared_ptr (load_as_unique_ptr)."
)
def test_virtual_base_not_at_offset_0():
# This test ensures that the Diamond fixture actually exercises a non-zero
# virtual-base subobject offset on our supported platforms/ABIs.
#
# If this assert ever fails on some platform/toolchain, please adjust the
# C++ fixture so the virtual base is *not* at offset 0:
# - Keep VBase non-empty.
# - Make Left and Right non-empty and asymmetrically sized and, if
# needed, nudge with a modest alignment.
# - The goal is to achieve a non-zero address delta between `Diamond*`
# and `static_cast<VBase*>(Diamond*)`.
#
# Rationale: certain smart_holder features are exercised only when the
# registered subobject address differs from the most-derived object start,
# so this check guards test efficacy across compilers.
addrs = m.diamond_addrs()
assert addrs.as_vbase - addrs.as_self != 0, (
"Diamond VBase at offset 0 on this platform; to ensure test efficacy, "
"tweak fixtures (VBase/Left/Right) to ensure non-zero subobject offset."
)
@pytest.mark.parametrize(
"make_fn",
[
m.make_diamond_as_vbase_raw_ptr, # exercises smart_holder::from_raw_ptr_take_ownership
m.make_diamond_as_vbase_shared_ptr, # exercises smart_holder_from_shared_ptr
m.make_diamond_as_vbase_unique_ptr, # exercises smart_holder_from_unique_ptr
],
)
def test_make_diamond_as_vbase(make_fn):
# Added under PR #5836
vb = make_fn()
assert vb.ping() == 7
@pytest.mark.parametrize(
"clone_fn",
[
m.Tiger.clone_raw_ptr,
m.Tiger.clone_shared_ptr,
m.Tiger.clone_unique_ptr,
],
)
def test_animal_cat_tiger(clone_fn):
# Based on Animal-Cat-Tiger reproducer under PR #5796
tiger = m.Tiger()
cloned = clone_fn(tiger)
assert isinstance(cloned, m.Tiger)