diff --git a/docs/upgrade.rst b/docs/upgrade.rst index 236494a5cd..196e999651 100644 --- a/docs/upgrade.rst +++ b/docs/upgrade.rst @@ -420,14 +420,14 @@ constructors prevent such mistakes. See :ref:`custom_constructors` for details. 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 65b6bd07df..14b3feb389 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -545,23 +545,33 @@ PYBIND11_NOINLINE void instance::deallocate_layout() { /// 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 @@ class old_style_init_scope { old_style_init_scope &operator=(const old_style_init_scope &) = delete; private: - value_and_holder *v_h_; + instance *inst_; + std::vector 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 21ff617365..47430d3a29 100644 --- a/tests/test_class.cpp +++ b/tests/test_class.cpp @@ -650,6 +650,7 @@ TEST_SUBMODULE(class_, m) { // 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 diff --git a/tests/test_class.py b/tests/test_class.py index 645b799c9f..638053b494 100644 --- a/tests/test_class.py +++ b/tests/test_class.py @@ -338,6 +338,35 @@ def __index__(self): 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."""