Fix smart_holder multiple/virtual inheritance bugs in shared_ptr and unique_ptr to-Python conversions
#5836
+195
−15
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
```
/__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
```
|
CI results @ commit 5f77da3: 66 failing, 2 skipped, 17 successful checks CI: https://github.com/pybind/pybind11/actions/runs/17707681525?pr=5836 |
|
CI results @ commit 3620ceb: 20 failing, 2 skipped, 63 successful checks CI: https://github.com/pybind/pybind11/actions/runs/17713020523?pr=5836 |
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.
…ampoline code (which often uses the term "alias", too)
…ithin test_class_sh_mi_thunks.cpp
```
/__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 *
```
rwgk
changed the title
smart_holder multiple/virtual inheritance bugs in shared_ptr and unique_ptr to-Python conversions
|
@henryiii This PR is ready for review. @MannixYang If you get a chance to try this out, please report back. |
|
@rwgk Thank you very much indeed. Yes, I will give it a try. It should be within the next two days. |
|
@rwgk Great! This is very useful. My project can now run. Thank you again. |
Thanks a lot for confirming! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Closes #5786.
This PR fixes two independent bugs in the
smart_holdermachinery, both exposed by new tests involving virtual inheritance.1. Fix in
pybind11/detail/type_caster_base.hThe return-value caster for
std::shared_ptr<T>was incorrectly usingsrc.get()(the most-derived pointer) instead ofst.first(the adjusted subobject pointer appropriate for the registeredtinfo).On MSVC with virtual inheritance, this mismatch led to passing an
Animal*(see below) toregister_instancewhen bindingTiger, which caused the virtual-base offset walker to dereference a bogus vbptr.This is the bug reported in #5786. Using
st.firstensures the correct subobject is registered.2. Fix in
pybind11/detail/struct_smart_holder.hsmart_holder::from_unique_ptrwas storing the subobject pointer as the owned resource in its control block. Under MSVC, destroying through that misaligned pointer caused NX faults in the virtual destructor path.The fix mirrors the
shared_ptrpath: always own the realT*object start with the deleter, and, if needed, create an aliasingshared_ptr<void>at the subobject pointer for registration/identity.Tests
test_class_sh_mi_thunks) was added. This was designed to also fail on Linux/Clang/GCC without the fix (not just on MSVC). Indeed, the diamond case fails across allci.ymljobs and severaltests-cibw.ymljobs pre-fix, and passes after fix (see comment).smart_holder_from_unique_ptr, which immediately exposed the second bug. That test produced 20 MSVC failures pre-fix (comment), and passes after the fix.test_virtual_base_at_offset_0is included to make it explicit when a compiler/layout places the virtual base at offset 0. This isn’t a true skip, but a way to document in the pytest summary when the test can’t exercise the MI/VI code path. In practice, this skip has never been triggered in any GitHub Actions job.asserts remain inpybind11/detail/class.h. They were invaluable during debugging and experimentation, have no impact on non-debug builds, and may be useful again if MI/VI edge cases resurface.Context
The changes here are in the same spirit as PR #4380, which fixed earlier oversights in
smart_holderMI handling. As noted there, MI use cases are uncommon in downstream projects (and even discouraged in some organizations), so coverage has historically been thin. The new tests give us much better confidence that bothshared_ptrandunique_ptradoption paths are now robust under virtual and multiple inheritance.Full ChatGPT conversation guiding the work on the fixes (very long and involved): https://chatgpt.com/share/68c7950a-61b8-8008-8678-53b96643ce7e
A lot of the credit for the fixes goes to ChatGPT 5 Pro
Suggested changelog entry:
Fixed two
smart_holderbugs inshared_ptrandunique_ptradoption with multiple/virtual inheritance:shared_ptrto-Python caster now registers the correct subobject pointer (fixes [BUG]: Using smart_holder in virtual inheritance relationship error #5786).unique_ptradoption now owns the proper object start while aliasing subobject pointers for registration, fixing MSVC crashes during destruction.