fix: Clean sibling storage after old-style init (#6180)
diff --git a/docs/upgrade.rst b/docs/upgrade.rst
index 236494a..196e999 100644
--- a/docs/upgrade.rst
+++ b/docs/upgrade.rst
@@ -420,14 +420,14 @@
though no C++ object has been constructed there yet. Accessing the storage through such
a pointer as though it contained a live C++ object results in undefined behavior.
- Consequently, until placement-new completes, the binding must not otherwise load or inspect
- the instance as a C++ object. Unsafe access can occur through reentrant argument conversion
- or callback code, nested initialization, another C++ base in a Python multiple-inheritance
- instance, or concurrent access. Mixing old- and new-style constructor overloads does not
- narrow the window. Such access may treat unconstructed storage as a live object and result
- in undefined behavior. To avoid these hazards, use ``py::init()`` factories and
- ``py::pickle()`` for new bindings, and migrate existing placement-new callbacks wherever
- practical.
+ Consequently, while such a constructor overload chain is active, the binding must not otherwise
+ load or inspect an unconstructed value slot as a C++ object. Unsafe access can occur through
+ reentrant argument conversion or callback code, nested initialization, another C++ base in a
+ Python multiple-inheritance instance, or concurrent access. Mixing old- and new-style
+ constructor overloads does not narrow the window. Such access may treat unconstructed storage
+ as a live object and result in undefined behavior. To avoid these hazards, use ``py::init()``
+ factories and ``py::pickle()`` for new bindings, and migrate existing placement-new callbacks
+ wherever practical.
Mirroring the custom constructor changes, ``py::pickle()`` is now the preferred
way to get and set object state. See :ref:`pickling` for details.
diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h
index 65b6bd0..14b3feb 100644
--- a/include/pybind11/detail/type_caster_base.h
+++ b/include/pybind11/detail/type_caster_base.h
@@ -545,23 +545,33 @@
/// reference from `docs/advanced/classes.rst`: the broad scope preserves historical behavior,
/// with documented reentrancy, multiple-inheritance, nesting, and concurrency limitations.
///
-/// If construction fails (the holder was never constructed) after storage was lazily allocated
-/// inside this scope, the destructor frees that storage and resets the value pointer, so that the
-/// uninitialized-value guard in `load_value()` stays effective for later uses of the instance.
+/// When the scope exits, the destructor frees storage that was lazily allocated in any value slot
+/// that was empty on entry and whose holder was never constructed. This keeps the uninitialized-
+/// value guard in `load_value()` effective for later uses of the instance, including sibling slots
+/// in a Python multiple-inheritance layout.
class old_style_init_scope {
public:
- explicit old_style_init_scope(value_and_holder *v_h) : v_h_{v_h} {
- if (v_h_ != nullptr) {
- was_active_ = v_h_->inst->old_style_init_active;
- value_was_null_ = v_h_->value_ptr() == nullptr;
- v_h_->inst->old_style_init_active = true;
+ explicit old_style_init_scope(value_and_holder *v_h)
+ : inst_{v_h != nullptr ? v_h->inst : nullptr} {
+ if (inst_ != nullptr) {
+ values_and_holders vhs(inst_);
+ empty_slots_.reserve(vhs.size());
+ for (auto &slot : vhs) {
+ if (slot.value_ptr() == nullptr) {
+ empty_slots_.push_back(slot);
+ }
+ }
+ was_active_ = inst_->old_style_init_active;
+ inst_->old_style_init_active = true;
}
}
~old_style_init_scope() {
- if (v_h_ != nullptr) {
- v_h_->inst->old_style_init_active = was_active_;
- if (value_was_null_ && !v_h_->holder_constructed() && v_h_->value_ptr() != nullptr) {
- v_h_->type->dealloc(*v_h_); // Frees the storage and nulls the value pointer.
+ if (inst_ != nullptr) {
+ inst_->old_style_init_active = was_active_;
+ for (auto &slot : empty_slots_) {
+ if (!slot.holder_constructed() && slot.value_ptr() != nullptr) {
+ slot.type->dealloc(slot); // Frees the storage and nulls the value pointer.
+ }
}
}
}
@@ -569,9 +579,9 @@
old_style_init_scope &operator=(const old_style_init_scope &) = delete;
private:
- value_and_holder *v_h_;
+ instance *inst_;
+ std::vector<value_and_holder> empty_slots_;
bool was_active_ = false;
- bool value_was_null_ = false;
};
PYBIND11_NOINLINE bool isinstance_generic(handle obj, const std::type_info &tp) {
diff --git a/tests/test_class.cpp b/tests/test_class.cpp
index 21ff617..47430d3 100644
--- a/tests/test_class.cpp
+++ b/tests/test_class.cpp
@@ -650,6 +650,7 @@
// This probe intentionally does not dereference the pointer. It documents the narrow scope of
// this fix without itself reading storage before an OldStyleInit lifetime has begun.
m.def("expose_old_style_init_pointer", [](OldStyleInit *value) { return value != nullptr; });
+ m.def("expose_new_no_init_pointer", [](NewNoInit *value) { return value != nullptr; });
}
template <int N>
diff --git a/tests/test_class.py b/tests/test_class.py
index 645b799..638053b 100644
--- a/tests/test_class.py
+++ b/tests/test_class.py
@@ -338,6 +338,35 @@
assert obj.data() == 44
+def test_old_style_init_cleans_multiple_inheritance_sibling_storage():
+ """The old-style compatibility window is instance-wide, so a reentrant load can lazily
+ allocate an unconstructed sibling base slot. Scope cleanup must reset that slot too."""
+
+ class Derived(m.OldStyleInit, m.NewNoInit):
+ pass
+
+ obj = Derived.__new__(Derived)
+ seen = {}
+
+ class LoadSiblingOnIndex:
+ def __index__(self):
+ # Deliberately obtain but do not dereference the sibling's unconstructed storage.
+ seen["exposed"] = m.expose_new_no_init_pointer(obj)
+ return 45
+
+ m.OldStyleInit.__init__(obj, LoadSiblingOnIndex())
+
+ assert seen == {"exposed": True}
+ assert m.OldStyleInit.data(obj) == 45
+ with pytest.raises(ValueError, match="uninitialized"):
+ m.expose_new_no_init_pointer(obj)
+
+ # The sibling slot remains usable after the old-style constructor returns.
+ m.NewNoInit.__init__(obj, 46)
+ assert m.OldStyleInit.data(obj) == 45
+ assert m.NewNoInit.data(obj) == 46
+
+
def test_reentrant_load_during_new_style_init():
"""New-style constructors never need lazy allocation, so passing the half-built instance
to another bound function while `__init__` runs must raise, not hand out garbage."""
