From b863457bc697d9fd07fb62a6ec276840d7e7a747 Mon Sep 17 00:00:00 2001 From: "Andrew M. James" Date: Fri, 28 Aug 2026 09:21:28 -0500 Subject: [PATCH 01/19] fix: Guard against using a uninitialized value after `__new__` allocating python object fixes: #6153 Objects initialized with `cls.__new__(cls)` (`cls` is a pybind11 bound type). Will not have the C++ object allocated. When hitting `load_value` storage is allocated but not initialized, calling a virtual method will load a garbage vptr and segfault. This is similar to #2152, but the guard in metaclass `__call__` is not triggered when using `__new__`. Protect against giving a pointer to garbage in all cases except the `__init__` + `__setstate__` path. Authored with claude --- docs/advanced/classes.rst | 8 +++++ include/pybind11/detail/common.h | 5 +++ include/pybind11/detail/type_caster_base.h | 40 ++++++++++++++++++++++ include/pybind11/pybind11.h | 8 +++++ tests/test_class.cpp | 24 +++++++++++++ tests/test_class.py | 34 ++++++++++++++++++ 6 files changed, 119 insertions(+) diff --git a/docs/advanced/classes.rst b/docs/advanced/classes.rst index 2954411d7b..c4c32f7c38 100644 --- a/docs/advanced/classes.rst +++ b/docs/advanced/classes.rst @@ -1427,4 +1427,12 @@ You can do that using ``py::custom_type_setup``: cls.def("size", &ContainerOwnsPythonObjects::size); cls.def("clear", &ContainerOwnsPythonObjects::clear); +.. note:: + + The ``py::detail::is_holder_constructed()`` guards above are required. During garbage + collection, ``tp_traverse`` and ``tp_clear`` may be handed an instance whose C++ value has + not been constructed yet -- for example one created with ``__new__`` before ``__init__`` + has run. Casting such an instance raises ``ValueError``, and an exception must not be + allowed to escape either of these slots. + .. versionadded:: 2.8 diff --git a/include/pybind11/detail/common.h b/include/pybind11/detail/common.h index c8fff5c144..6f4323513c 100644 --- a/include/pybind11/detail/common.h +++ b/include/pybind11/detail/common.h @@ -676,6 +676,11 @@ struct instance { bool has_patients : 1; /// If true, this Python object needs to be kept alive for the lifetime of the C++ value. bool is_alias : 1; + /// If true, an old-style placement-new `__init__`/`__setstate__` is currently constructing the + /// C++ value for this instance. This is the *only* situation in which + /// `type_caster_generic::load_value()` may lazily allocate storage for a value that has not + /// been constructed yet; see `instance_construction_scope` and `cpp_function::dispatcher()`. + bool construction_in_progress : 1; /// Initializes all of the above type/values/holders data (but not the instance values /// themselves) diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index 161b9884fa..3cdd3a9d55 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -525,6 +525,7 @@ PYBIND11_NOINLINE void instance::allocate_layout() { = reinterpret_cast(&nonsimple.values_and_holders[flags_at]); } owned = true; + construction_in_progress = false; } // NOLINTNEXTLINE(readability-make-member-function-const) @@ -534,6 +535,31 @@ PYBIND11_NOINLINE void instance::deallocate_layout() { } } +/// RAII helper marking `inst` as "currently being constructed", which is the only situation in +/// which `type_caster_generic::load_value()` will lazily allocate storage for a C++ value that has +/// not been constructed yet. Passing `nullptr` makes this a no-op. Nesting is supported: the +/// previous state is restored, not unconditionally cleared. +class instance_construction_scope { +public: + explicit instance_construction_scope(instance *inst) : inst_{inst} { + if (inst_ != nullptr) { + was_in_progress_ = inst_->construction_in_progress; + inst_->construction_in_progress = true; + } + } + ~instance_construction_scope() { + if (inst_ != nullptr) { + inst_->construction_in_progress = was_in_progress_; + } + } + instance_construction_scope(const instance_construction_scope &) = delete; + instance_construction_scope &operator=(const instance_construction_scope &) = delete; + +private: + instance *inst_; + bool was_in_progress_ = false; +}; + PYBIND11_NOINLINE bool isinstance_generic(handle obj, const std::type_info &tp) { handle type = detail::get_type_handle(tp, false); if (!type) { @@ -1140,6 +1166,20 @@ class type_caster_generic { auto *&vptr = v_h.value_ptr(); // Lazy allocation for unallocated values: if (vptr == nullptr) { + // Lazy allocation exists only to support the deprecated old-style placement-new + // `__init__`/`__setstate__` idiom, which is handed a reference to uninitialized + // storage and constructs the C++ value into it. In any other context a null value + // pointer means the C++ object was never constructed -- e.g. the instance was created + // with `__new__()`, bypassing `__init__()` -- and handing out a pointer to + // uninitialized memory from here is undefined behavior (typically a segfault on the + // first virtual call). Fail loudly instead. + if (!v_h.inst->construction_in_progress) { + throw value_error("Missing value for wrapped C++ type `" + + clean_type_id(cpptype->name()) + + "`: Python instance is uninitialized: the C++ object was " + "never constructed (`__init__()` was bypassed, e.g. by " + "calling `__new__()` directly)."); + } const auto *type = v_h.type ? v_h.type : typeinfo; if (type->operator_new) { vptr = type->operator_new(type->type_size); diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index f57514ae28..dc50d6d577 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1001,6 +1001,14 @@ class cpp_function : public function { } } + // While a constructor runs, `type_caster_generic::load_value()` is permitted to lazily + // allocate storage for the C++ value that the constructor is about to construct (the + // deprecated old-style placement-new `__init__`/`__setstate__` idiom relies on this). + // Outside this scope, loading a not-yet-constructed instance is an error. + detail::instance_construction_scope construction_scope( + overloads->is_constructor ? reinterpret_cast(parent.ptr()) + : nullptr); + try { // We do this in two passes: in the first pass, we load arguments with `convert=false`; // in the second, we allow conversion (except for arguments with an explicit diff --git a/tests/test_class.cpp b/tests/test_class.cpp index e520f29ec5..1bdc41ce79 100644 --- a/tests/test_class.cpp +++ b/tests/test_class.cpp @@ -77,6 +77,18 @@ static_assert(!py::detail::is_same_or_base_of< test_class::pr5396_forward_declared_class::ForwardClass>::value, ""); +// test_new_bypasses_init +struct NewNoInit { + int m_data; + explicit NewNoInit(int data) : m_data(data) {} + NewNoInit(const NewNoInit &) = default; + virtual ~NewNoInit() = default; + int data() const { return m_data; } + // Virtual on purpose: using a not-yet-constructed instance reads the vtable pointer out of + // uninitialized storage, which segfaults rather than merely returning a garbage value. + virtual int v_data() const { return m_data; } +}; + TEST_SUBMODULE(class_, m) { m.def("obj_class_name", [](py::handle obj) { return py::detail::obj_class_name(obj.ptr()); }); @@ -597,6 +609,18 @@ TEST_SUBMODULE(class_, m) { m.def("return_universal_recipient", []() -> test_class::ConvertibleFromAnything { return test_class::ConvertibleFromAnything{}; }); + + py::class_(m, "NewNoInit") + .def(py::init()) + .def("data", &NewNoInit::data) + .def("v_data", &NewNoInit::v_data) + .def(py::pickle([](const NewNoInit &p) { return py::make_tuple(p.m_data); }, + [](const py::tuple &t) { + if (t.size() != 1) { + throw std::runtime_error("Invalid state!"); + } + return NewNoInit(t[0].cast()); + })); } template diff --git a/tests/test_class.py b/tests/test_class.py index 201c7e339e..ad84715677 100644 --- a/tests/test_class.py +++ b/tests/test_class.py @@ -1,6 +1,7 @@ from __future__ import annotations import gc +import pickle import sys from unittest import mock @@ -251,6 +252,39 @@ def __init__(self): assert msg(exc_info.value) == expected +def test_new_bypasses_init(): + """`__new__` allocates the Python object but not the C++ one; using the instance before + `__init__` has run must raise instead of segfaulting.""" + obj = m.NewNoInit.__new__(m.NewNoInit) + + for use in (obj.data, obj.v_data, obj.__getstate__): + with pytest.raises(ValueError) as exc_info: + use() + assert "Python instance is uninitialized" in str(exc_info.value) + assert "NewNoInit" in str(exc_info.value) + + # Calling `__init__()` is the sanctioned way to finish an object made with `__new__()`. + obj.__init__(42) + assert obj.data() == 42 + assert obj.v_data() == 42 + + +def test_new_then_setstate(): + """`__new__` must not be blocked: pickle relies on it, and `__setstate__` finishes the + object off. This walks the protocol by hand, then checks the real thing.""" + real_obj = m.NewNoInit(42) + assert real_obj.data() == 42 + state = real_obj.__getstate__() + + obj = m.NewNoInit.__new__(m.NewNoInit) # NEWOBJ + obj.__setstate__(state) # BUILD + assert obj.data() == 42 + assert obj.v_data() == 42 + + for protocol in range(2, pickle.HIGHEST_PROTOCOL + 1): + assert pickle.loads(pickle.dumps(m.NewNoInit(7), protocol)).v_data() == 7 + + @pytest.mark.parametrize( "mock_return_value", [None, (1, 2, 3), m.Pet("Polly", "parrot"), m.Dog("Molly")] ) From 66f8f3760f02cb596b28e552fc4f95cd79586b7a Mon Sep 17 00:00:00 2001 From: Henry Schreiner Date: Sat, 29 Aug 2026 14:08:37 -0400 Subject: [PATCH 02/19] fix: free lazily allocated storage on failed init and only permit lazy allocation for old-style constructors If an old-style placement-new `__init__`/`__setstate__` failed after `self` was loaded, the lazily allocated storage stayed behind with a null-holder instance, so the uninitialized-value guard never fired again and later use read uninitialized memory. `instance_construction_scope` now tracks the constructor's `value_and_holder` and frees storage that was lazily allocated during a construction that did not complete. Also arm the scope only when the overload chain contains an old-style constructor. New-style constructors receive `self` directly and never need lazy allocation, so reentrant loads of the half-built instance now raise `ValueError` instead of handing out uninitialized storage. Assisted-by: ClaudeCode:claude-fable-5 Claude-Session: https://claude.ai/code/session_01TQXCSykMn5EL7sc6VgTUTC --- include/pybind11/detail/type_caster_base.h | 31 +++++++++++------ include/pybind11/pybind11.h | 24 +++++++++---- tests/test_class.cpp | 20 +++++++++++ tests/test_class.py | 40 ++++++++++++++++++++++ 4 files changed, 97 insertions(+), 18 deletions(-) diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index 3cdd3a9d55..4657a892ba 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -535,29 +535,38 @@ PYBIND11_NOINLINE void instance::deallocate_layout() { } } -/// RAII helper marking `inst` as "currently being constructed", which is the only situation in -/// which `type_caster_generic::load_value()` will lazily allocate storage for a C++ value that has -/// not been constructed yet. Passing `nullptr` makes this a no-op. Nesting is supported: the -/// previous state is restored, not unconditionally cleared. +/// RAII helper marking the instance behind `v_h` as "currently being constructed", which is the +/// only situation in which `type_caster_generic::load_value()` will lazily allocate storage for a +/// C++ value that has not been constructed yet. Passing `nullptr` makes this a no-op. Nesting is +/// supported: the previous state is restored, not unconditionally cleared. +/// +/// 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. class instance_construction_scope { public: - explicit instance_construction_scope(instance *inst) : inst_{inst} { - if (inst_ != nullptr) { - was_in_progress_ = inst_->construction_in_progress; - inst_->construction_in_progress = true; + explicit instance_construction_scope(value_and_holder *v_h) : v_h_{v_h} { + if (v_h_ != nullptr) { + was_in_progress_ = v_h_->inst->construction_in_progress; + value_was_null_ = v_h_->value_ptr() == nullptr; + v_h_->inst->construction_in_progress = true; } } ~instance_construction_scope() { - if (inst_ != nullptr) { - inst_->construction_in_progress = was_in_progress_; + if (v_h_ != nullptr) { + v_h_->inst->construction_in_progress = was_in_progress_; + 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. + } } } instance_construction_scope(const instance_construction_scope &) = delete; instance_construction_scope &operator=(const instance_construction_scope &) = delete; private: - instance *inst_; + value_and_holder *v_h_; bool was_in_progress_ = false; + bool value_was_null_ = false; }; PYBIND11_NOINLINE bool isinstance_generic(handle obj, const std::type_info &tp) { diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index dc50d6d577..9e4a91b405 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1001,13 +1001,23 @@ class cpp_function : public function { } } - // While a constructor runs, `type_caster_generic::load_value()` is permitted to lazily - // allocate storage for the C++ value that the constructor is about to construct (the - // deprecated old-style placement-new `__init__`/`__setstate__` idiom relies on this). - // Outside this scope, loading a not-yet-constructed instance is an error. - detail::instance_construction_scope construction_scope( - overloads->is_constructor ? reinterpret_cast(parent.ptr()) - : nullptr); + // While an old-style placement-new `__init__`/`__setstate__` runs, + // `type_caster_generic::load_value()` is permitted to lazily allocate storage for the C++ + // value that the constructor is about to construct into. New-style constructors never load + // `self` through a type caster (it is injected directly below), so the scope stays + // disarmed for chains that contain only new-style constructors and loading a + // not-yet-constructed instance remains an error even while they run. The scope also frees + // storage that was lazily allocated by a constructor call that then failed. + detail::value_and_holder *lazily_allocatable_v_h = nullptr; + if (overloads->is_constructor) { + for (const function_record *fr = overloads; fr != nullptr; fr = fr->next) { + if (!fr->is_new_style_constructor) { + lazily_allocatable_v_h = &self_value_and_holder; + break; + } + } + } + detail::instance_construction_scope construction_scope(lazily_allocatable_v_h); try { // We do this in two passes: in the first pass, we load arguments with `convert=false`; diff --git a/tests/test_class.cpp b/tests/test_class.cpp index 1bdc41ce79..15217802d8 100644 --- a/tests/test_class.cpp +++ b/tests/test_class.cpp @@ -89,6 +89,15 @@ struct NewNoInit { virtual int v_data() const { return m_data; } }; +// test_failed_old_style_init_does_not_leave_lazy_storage +struct OldStyleInit { + int m_data; + explicit OldStyleInit(int data) : m_data(data) {} + virtual ~OldStyleInit() = default; + int data() const { return m_data; } + virtual int v_data() const { return m_data; } +}; + TEST_SUBMODULE(class_, m) { m.def("obj_class_name", [](py::handle obj) { return py::detail::obj_class_name(obj.ptr()); }); @@ -621,6 +630,17 @@ TEST_SUBMODULE(class_, m) { } return NewNoInit(t[0].cast()); })); + + py::class_(m, "OldStyleInit") + .def("__init__", + [](OldStyleInit &self, int x) { + if (x < 0) { + throw std::runtime_error("negative data"); + } + new (&self) OldStyleInit(x); + }) + .def("data", &OldStyleInit::data) + .def("v_data", &OldStyleInit::v_data); } template diff --git a/tests/test_class.py b/tests/test_class.py index ad84715677..bb606eee65 100644 --- a/tests/test_class.py +++ b/tests/test_class.py @@ -285,6 +285,46 @@ def test_new_then_setstate(): assert pickle.loads(pickle.dumps(m.NewNoInit(7), protocol)).v_data() == 7 +def test_failed_old_style_init_does_not_leave_lazy_storage(): + """If an old-style placement-new `__init__` throws before constructing the value, the + lazily allocated storage must not linger: later use must still raise, not segfault.""" + obj = m.OldStyleInit.__new__(m.OldStyleInit) + with pytest.raises(RuntimeError, match="negative data"): + obj.__init__(-1) + + # The failed __init__ already lazily allocated storage for `self`, so without cleanup the + # uninitialized-instance guard never fires again and this reads a garbage vtable pointer. + with pytest.raises(ValueError, match="uninitialized"): + obj.v_data() + + # A successful retry is still allowed. + obj.__init__(42) + assert obj.v_data() == 42 + + +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.""" + obj = m.NewNoInit.__new__(m.NewNoInit) + seen = {} + + class Evil: + def __index__(self): + # Runs during int conversion of the constructor argument, while + # construction_in_progress is set on `obj` and its C++ value is unconstructed. + try: + seen["data"] = obj.data() + except ValueError as exc: + seen["error"] = exc + raise TypeError("stop the constructor") + + with pytest.raises(TypeError): + obj.__init__(Evil()) + + assert "data" not in seen, f"handed out uninitialized storage: {seen['data']!r}" + assert "error" in seen + + @pytest.mark.parametrize( "mock_return_value", [None, (1, 2, 3), m.Pet("Polly", "parrot"), m.Dog("Molly")] ) From b80c2243ee2d8b067f56fb792272955d44c74ee5 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 30 Aug 2026 13:34:14 -0700 Subject: [PATCH 03/19] fix: isolate old-style constructor storage Track construction per value-and-holder, grant a one-shot loader-frame permission only to the exact legacy constructor self conversion, and keep its raw storage private until the native callback returns. Reject reentrant, nested, cross-base, and cross-thread loads while preserving overload fallback, failure cleanup, pickle setstate callbacks, and repeated initialization behavior. --- include/pybind11/cast.h | 11 +- include/pybind11/detail/common.h | 8 +- include/pybind11/detail/type_caster_base.h | 250 ++++++++++++++++----- include/pybind11/detail/value_and_holder.h | 16 ++ include/pybind11/pybind11.h | 56 ++--- tests/test_class.cpp | 54 ++++- tests/test_class.py | 193 ++++++++++++++-- 7 files changed, 478 insertions(+), 110 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 1d857a0ed5..2c517cec69 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -2179,6 +2179,12 @@ class argument_loader { private: static bool load_impl_sequence(function_call &, index_sequence<>) { return true; } + template + bool load_one(function_call &call) { + loader_life_support::argument_load_guard guard(I == 0); + return std::get(argcasters).load(call.args[I], call.args_convert[I]); + } + template bool load_impl_sequence(function_call &call, index_sequence) { PYBIND11_WARNING_PUSH @@ -2187,11 +2193,11 @@ class argument_loader { PYBIND11_WARNING_DISABLE_GCC("-Warray-bounds") #endif #ifdef __cpp_fold_expressions - if ((... || !std::get(argcasters).load(call.args[Is], call.args_convert[Is]))) { + if ((... || !load_one(call))) { return false; } #else - for (bool r : {std::get(argcasters).load(call.args[Is], call.args_convert[Is])...}) { + for (bool r : {load_one(call)...}) { if (!r) { return false; } @@ -2203,6 +2209,7 @@ class argument_loader { template Return call_impl(Func &&f, index_sequence, Guard &&) && { + loader_life_support::old_style_init_call_guard old_style_init_guard; return std::forward(f)(cast_op(std::move(std::get(argcasters)))...); } diff --git a/include/pybind11/detail/common.h b/include/pybind11/detail/common.h index 6f4323513c..d9ab8b751f 100644 --- a/include/pybind11/detail/common.h +++ b/include/pybind11/detail/common.h @@ -676,11 +676,8 @@ struct instance { bool has_patients : 1; /// If true, this Python object needs to be kept alive for the lifetime of the C++ value. bool is_alias : 1; - /// If true, an old-style placement-new `__init__`/`__setstate__` is currently constructing the - /// C++ value for this instance. This is the *only* situation in which - /// `type_caster_generic::load_value()` may lazily allocate storage for a value that has not - /// been constructed yet; see `instance_construction_scope` and `cpp_function::dispatcher()`. - bool construction_in_progress : 1; + /// For simple layout, tracks whether a constructor is currently constructing the C++ value. + bool simple_value_constructing : 1; /// Initializes all of the above type/values/holders data (but not the instance values /// themselves) @@ -698,6 +695,7 @@ struct instance { /// Bit values for the non-simple status flags static constexpr uint8_t status_holder_constructed = 1; static constexpr uint8_t status_instance_registered = 2; + static constexpr uint8_t status_value_constructing = 4; }; static_assert(std::is_standard_layout::value, diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index 4657a892ba..bf6175162b 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -9,6 +9,7 @@ #pragma once +#include #include #include #include @@ -61,9 +62,37 @@ class loader_life_support { loader_life_support *parent = nullptr; std::unordered_set keep_alive; + // Old-style placement-new constructors need raw storage while loading their `self` + // argument. Keep it private to the exact overload candidate until its C++ callable returns. + value_and_holder *old_style_init_self = nullptr; + void *old_style_init_storage = nullptr; + bool old_style_init_self_load_allowed = false; + bool old_style_init_self_load_claimed = false; + + static bool is_same_value_and_holder(const value_and_holder &lhs, + const value_and_holder &rhs) { + return lhs.inst == rhs.inst && lhs.vh == rhs.vh; + } + + void cleanup_old_style_init_storage() { + if (old_style_init_storage == nullptr) { + return; + } + auto v_h = *old_style_init_self; + scoped_critical_section lock( + handle(reinterpret_cast(old_style_init_self->inst))); + if (v_h.value_ptr() != nullptr) { + pybind11_fail("loader_life_support: old-style constructor storage collision"); + } + v_h.value_ptr() = old_style_init_storage; + old_style_init_storage = nullptr; + v_h.type->dealloc(v_h); // Frees the storage and nulls the value pointer. + } + public: /// A new patient frame is created when a function is entered - loader_life_support() { + explicit loader_life_support(value_and_holder *old_style_init_self = nullptr) + : old_style_init_self(old_style_init_self) { auto &frame = tls_current_frame(); parent = frame; frame = this; @@ -76,11 +105,118 @@ class loader_life_support { pybind11_fail("loader_life_support: internal error"); } frame = parent; + cleanup_old_style_init_storage(); for (auto *item : keep_alive) { Py_DECREF(item); } } + /// Restricts the special old-style constructor permission to argument zero of the current + /// candidate. A nested bound call has its own loader frame and cannot inherit this permission. + class argument_load_guard { + public: + explicit argument_load_guard(bool is_first_argument) { + auto *current = tls_current_frame(); + if (is_first_argument && current != nullptr && current->old_style_init_self != nullptr + && !current->old_style_init_self_load_claimed) { + frame = current; + frame->old_style_init_self_load_allowed = true; + } + } + ~argument_load_guard() { + if (frame != nullptr) { + frame->old_style_init_self_load_allowed = false; + } + } + argument_load_guard(const argument_load_guard &) = delete; + argument_load_guard &operator=(const argument_load_guard &) = delete; + + private: + loader_life_support *frame = nullptr; + }; + + /// Some legacy `__setstate__` implementations accept `self` as `py::object` and perform the + /// typed cast inside the C++ callable. At that point all other arguments have finished + /// loading, so granting the same exact, one-shot permission is safe. Reentrant bound calls + /// still get a separate loader frame. + class old_style_init_call_guard { + public: + old_style_init_call_guard() { + auto *current = tls_current_frame(); + if (current != nullptr && current->old_style_init_self != nullptr + && !current->old_style_init_self_load_claimed) { + frame = current; + frame->old_style_init_self_load_allowed = true; + } + } + ~old_style_init_call_guard() { + if (frame != nullptr) { + frame->old_style_init_self_load_allowed = false; + } + } + old_style_init_call_guard(const old_style_init_call_guard &) = delete; + old_style_init_call_guard &operator=(const old_style_init_call_guard &) = delete; + + private: + loader_life_support *frame = nullptr; + }; + + /// Claims and allocates the private storage for the exact old-style constructor `self` load. + /// The permission is consumed before invoking a potentially user-defined operator new. + static bool try_reserve_old_style_init_storage(value_and_holder &v_h, + const type_info *type, + void *&value) { + auto *frame = tls_current_frame(); + if (frame == nullptr || !frame->old_style_init_self_load_allowed + || frame->old_style_init_self_load_claimed || frame->old_style_init_self == nullptr + || !is_same_value_and_holder(v_h, *frame->old_style_init_self) + || v_h.value_ptr() != nullptr) { + return false; + } + + frame->old_style_init_self_load_claimed = true; + frame->old_style_init_self_load_allowed = false; + if (type->operator_new) { + frame->old_style_init_storage = type->operator_new(type->type_size); + } else { +#if defined(__cpp_aligned_new) + if (type->type_align > __STDCPP_DEFAULT_NEW_ALIGNMENT__) { + frame->old_style_init_storage + = ::operator new(type->type_size, std::align_val_t(type->type_align)); + } else { + frame->old_style_init_storage = ::operator new(type->type_size); + } +#else + frame->old_style_init_storage = ::operator new(type->type_size); +#endif + } + if (frame->old_style_init_storage == nullptr) { + throw std::bad_alloc(); + } + value = frame->old_style_init_storage; + return true; + } + + /// Publishes a successfully placement-constructed value and immediately finalizes its holder. + /// This runs after the C++ callable, but before return-value conversion and post-call + /// policies. + static void complete_old_style_init() { + auto *frame = tls_current_frame(); + if (frame == nullptr || frame->old_style_init_storage == nullptr) { + return; + } + + auto v_h = *frame->old_style_init_self; + scoped_critical_section lock(handle(reinterpret_cast(v_h.inst))); + if (!v_h.value_constructing() || v_h.value_ptr() != nullptr) { + pybind11_fail("loader_life_support: invalid old-style constructor commit"); + } + v_h.value_ptr() = frame->old_style_init_storage; + frame->old_style_init_storage = nullptr; + v_h.type->init_instance(v_h.inst, nullptr); + v_h.set_value_constructing(false); + } + /// Keep `h` alive until the current patient frame is destroyed, if there is one. /// Returns false when called outside a bound function (no frame). Use this, rather /// than `add_patient`, when failing to register is acceptable because the caller @@ -525,7 +661,7 @@ PYBIND11_NOINLINE void instance::allocate_layout() { = reinterpret_cast(&nonsimple.values_and_holders[flags_at]); } owned = true; - construction_in_progress = false; + simple_value_constructing = false; } // NOLINTNEXTLINE(readability-make-member-function-const) @@ -535,37 +671,56 @@ PYBIND11_NOINLINE void instance::deallocate_layout() { } } -/// RAII helper marking the instance behind `v_h` as "currently being constructed", which is the -/// only situation in which `type_caster_generic::load_value()` will lazily allocate storage for a -/// C++ value that has not been constructed yet. Passing `nullptr` makes this a no-op. Nesting is -/// supported: the previous state is restored, not unconditionally cleared. -/// -/// 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. +/// Marks the exact value slot targeted by a constructor. This state is never permission to load +/// the value: every load is rejected until construction finishes, apart from the one-shot +/// old-style constructor `self` permission maintained by `loader_life_support`. class instance_construction_scope { public: - explicit instance_construction_scope(value_and_holder *v_h) : v_h_{v_h} { - if (v_h_ != nullptr) { - was_in_progress_ = v_h_->inst->construction_in_progress; - value_was_null_ = v_h_->value_ptr() == nullptr; - v_h_->inst->construction_in_progress = true; + explicit instance_construction_scope(value_and_holder *v_h) { + if (v_h == nullptr) { + return; } + v_h_ = *v_h; + started_ = false; + scoped_critical_section lock(handle(reinterpret_cast(v_h_.inst))); + if (v_h_.value_constructing()) { + return; + } + if (v_h_.instance_registered()) { + already_registered_ = true; + return; + } + value_was_null_ = v_h_.value_ptr() == nullptr; + v_h_.set_value_constructing(); + started_ = true; } ~instance_construction_scope() { - if (v_h_ != nullptr) { - v_h_->inst->construction_in_progress = was_in_progress_; - 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 (!started_ || v_h_.inst == nullptr) { + return; + } + + scoped_critical_section lock(handle(reinterpret_cast(v_h_.inst))); + // A failed new-style constructor can have published a value without completing its + // holder. Preserve the existing cleanup guarantee for that case. + if (value_was_null_ && !v_h_.holder_constructed() && v_h_.value_ptr() != nullptr) { + if (v_h_.instance_registered()) { + deregister_instance(v_h_.inst, v_h_.value_ptr(), v_h_.type); + v_h_.set_instance_registered(false); } + v_h_.type->dealloc(v_h_); } + v_h_.set_value_constructing(false); } instance_construction_scope(const instance_construction_scope &) = delete; instance_construction_scope &operator=(const instance_construction_scope &) = delete; + bool started() const { return started_; } + bool already_registered() const { return already_registered_; } + private: - value_and_holder *v_h_; - bool was_in_progress_ = false; + value_and_holder v_h_; + bool started_ = true; + bool already_registered_ = false; bool value_was_null_ = false; }; @@ -1163,6 +1318,25 @@ class type_caster_generic { // Base methods for generic caster; there are overridden in copyable_holder_caster void load_value(value_and_holder &&v_h) { + scoped_critical_section lock(handle(reinterpret_cast(v_h.inst))); + + // A non-null value pointer is not sufficient while a constructor is running: old-style + // placement-new storage may exist before the C++ object's lifetime has begun. Only the + // exact argument-zero load of the current old-style constructor may access private raw + // storage; reentrant, cross-base, nested, and cross-thread loads must all fail. + if (v_h.value_constructing()) { + void *reserved_value = nullptr; + const auto *type = v_h.type ? v_h.type : typeinfo; + if (loader_life_support::try_reserve_old_style_init_storage( + v_h, type, reserved_value)) { + value = reserved_value; + return; + } + throw value_error("Missing value for wrapped C++ type `" + + clean_type_id(cpptype->name()) + + "`: Python instance is still being constructed."); + } + if (typeinfo->holder_enum_v == detail::holder_enum_t::smart_holder) { smart_holder_type_caster_support::value_and_holder_helper v_h_helper; v_h_helper.loaded_v_h = v_h; @@ -1173,36 +1347,12 @@ class type_caster_generic { } } auto *&vptr = v_h.value_ptr(); - // Lazy allocation for unallocated values: if (vptr == nullptr) { - // Lazy allocation exists only to support the deprecated old-style placement-new - // `__init__`/`__setstate__` idiom, which is handed a reference to uninitialized - // storage and constructs the C++ value into it. In any other context a null value - // pointer means the C++ object was never constructed -- e.g. the instance was created - // with `__new__()`, bypassing `__init__()` -- and handing out a pointer to - // uninitialized memory from here is undefined behavior (typically a segfault on the - // first virtual call). Fail loudly instead. - if (!v_h.inst->construction_in_progress) { - throw value_error("Missing value for wrapped C++ type `" - + clean_type_id(cpptype->name()) - + "`: Python instance is uninitialized: the C++ object was " - "never constructed (`__init__()` was bypassed, e.g. by " - "calling `__new__()` directly)."); - } - const auto *type = v_h.type ? v_h.type : typeinfo; - if (type->operator_new) { - vptr = type->operator_new(type->type_size); - } else { -#if defined(__cpp_aligned_new) - if (type->type_align > __STDCPP_DEFAULT_NEW_ALIGNMENT__) { - vptr = ::operator new(type->type_size, std::align_val_t(type->type_align)); - } else { - vptr = ::operator new(type->type_size); - } -#else - vptr = ::operator new(type->type_size); -#endif - } + throw value_error("Missing value for wrapped C++ type `" + + clean_type_id(cpptype->name()) + + "`: Python instance is uninitialized: the C++ object was " + "never constructed (`__init__()` was bypassed, e.g. by " + "calling `__new__()` directly)."); } value = vptr; } diff --git a/include/pybind11/detail/value_and_holder.h b/include/pybind11/detail/value_and_holder.h index b24551e678..3c1442aba6 100644 --- a/include/pybind11/detail/value_and_holder.h +++ b/include/pybind11/detail/value_and_holder.h @@ -74,6 +74,22 @@ struct value_and_holder { &= static_cast(~instance::status_instance_registered); } } + bool value_constructing() const { + return inst->simple_layout + ? inst->simple_value_constructing + : ((inst->nonsimple.status[index] & instance::status_value_constructing) != 0); + } + // NOLINTNEXTLINE(readability-make-member-function-const) + void set_value_constructing(bool v = true) { + if (inst->simple_layout) { + inst->simple_value_constructing = v; + } else if (v) { + inst->nonsimple.status[index] |= instance::status_value_constructing; + } else { + inst->nonsimple.status[index] + &= static_cast(~instance::status_value_constructing); + } + } }; // This is a semi-public API to check if the corresponding instance has been constructed with a diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 9e4a91b405..ae4c45a802 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -513,10 +513,13 @@ class cpp_function : public function { handle result; if (call.func.is_setter) { (void) std::move(args_converter).template call(f); + loader_life_support::complete_old_style_init(); result = none().release(); } else { + auto &&cpp_result = std::move(args_converter).template call(f); + loader_life_support::complete_old_style_init(); result = cast_out::cast( - std::move(args_converter).template call(f), policy, call.parent); + std::forward(cpp_result), policy, call.parent); } return result; @@ -993,31 +996,28 @@ class cpp_function : public function { = get_type_info(reinterpret_cast(overloads->scope.ptr())); auto *const pi = reinterpret_cast(parent.ptr()); self_value_and_holder = pi->get_value_and_holder(tinfo, true); - - // If this value is already registered it must mean __init__ is invoked multiple times; - // we really can't support that in C++, so just ignore the second __init__. - if (self_value_and_holder.instance_registered()) { - return none().release().ptr(); - } } - // While an old-style placement-new `__init__`/`__setstate__` runs, - // `type_caster_generic::load_value()` is permitted to lazily allocate storage for the C++ - // value that the constructor is about to construct into. New-style constructors never load - // `self` through a type caster (it is injected directly below), so the scope stays - // disarmed for chains that contain only new-style constructors and loading a - // not-yet-constructed instance remains an error even while they run. The scope also frees - // storage that was lazily allocated by a constructor call that then failed. - detail::value_and_holder *lazily_allocatable_v_h = nullptr; + detail::instance_construction_scope construction_scope( + overloads->is_constructor ? &self_value_and_holder : nullptr); if (overloads->is_constructor) { - for (const function_record *fr = overloads; fr != nullptr; fr = fr->next) { - if (!fr->is_new_style_constructor) { - lazily_allocatable_v_h = &self_value_and_holder; - break; - } + // Invoking __init__ repeatedly on an already constructed value remains a no-op. + if (construction_scope.already_registered()) { + return none().release().ptr(); + } + if (!construction_scope.started()) { + set_error(PyExc_ValueError, + "Cannot initialize a wrapped C++ value while it is already being " + "constructed"); + return nullptr; } } - detail::instance_construction_scope construction_scope(lazily_allocatable_v_h); + + // On free-threaded Python, serialize the complete constructor transaction. Python + // critical sections are suspended around blocking operations, allowing another thread to + // enter, observe `value_constructing`, and reject access without racing status-byte + // updates. + scoped_critical_section constructor_lock(overloads->is_constructor ? parent : handle{}); try { // We do this in two passes: in the first pass, we load arguments with `convert=false`; @@ -1078,8 +1078,9 @@ class cpp_function : public function { // 0. Inject new-style `self` argument if (func.is_new_style_constructor) { - // The `value` may have been preallocated by an old-style `__init__` - // if it was a preceding candidate for overload resolution. + // Retain cleanup for a value partially published by a preceding failed + // new-style candidate. Old-style reservations are private and are cleaned up + // with their loader frame before another candidate is tried. if (self_value_and_holder) { self_value_and_holder.type->dealloc(self_value_and_holder); } @@ -1250,7 +1251,9 @@ class cpp_function : public function { // 6. Call the function. try { - loader_life_support guard{}; + loader_life_support guard{func.is_constructor && !func.is_new_style_constructor + ? &self_value_and_holder + : nullptr}; result = func.impl(call); } catch (reference_cast_error &) { result = PYBIND11_TRY_NEXT_OVERLOAD; @@ -1281,7 +1284,10 @@ class cpp_function : public function { // allowed for (auto &call : second_pass) { try { - loader_life_support guard{}; + loader_life_support guard{call.func.is_constructor + && !call.func.is_new_style_constructor + ? &self_value_and_holder + : nullptr}; result = call.func.impl(call); } catch (reference_cast_error &) { result = PYBIND11_TRY_NEXT_OVERLOAD; diff --git a/tests/test_class.cpp b/tests/test_class.cpp index 15217802d8..47e363b1d3 100644 --- a/tests/test_class.cpp +++ b/tests/test_class.cpp @@ -98,6 +98,14 @@ struct OldStyleInit { virtual int v_data() const { return m_data; } }; +// test_reentrant_load_during_mixed_style_init +struct MixedStyleInit { + int m_data; + explicit MixedStyleInit(int data) : m_data(data) {} + explicit MixedStyleInit(const std::string &data) : m_data(static_cast(data.size())) {} + int data() const { return m_data; } +}; + TEST_SUBMODULE(class_, m) { m.def("obj_class_name", [](py::handle obj) { return py::detail::obj_class_name(obj.ptr()); }); @@ -631,16 +639,42 @@ TEST_SUBMODULE(class_, m) { return NewNoInit(t[0].cast()); })); - py::class_(m, "OldStyleInit") - .def("__init__", - [](OldStyleInit &self, int x) { - if (x < 0) { - throw std::runtime_error("negative data"); - } - new (&self) OldStyleInit(x); - }) - .def("data", &OldStyleInit::data) - .def("v_data", &OldStyleInit::v_data); + py::class_ old_style_init(m, "OldStyleInit"); + ignoreOldStyleInitWarnings([&old_style_init]() { + old_style_init + .def("__init__", + [](OldStyleInit &self, int x) { + if (x < 0) { + throw std::runtime_error("negative data"); + } + new (&self) OldStyleInit(x); + }) + .def("__setstate__", [](const py::object &self_obj, const py::object &state) { + // Old-style callbacks taking a Python self perform the one authorized self cast + // inside the callable. Keep state conversion ahead of placement-new: Python + // executed by that cast must not be able to load the reserved storage again. + auto &self = self_obj.cast(); + int x = state.cast(); + new (&self) OldStyleInit(x); + }); + }); + old_style_init.def("data", &OldStyleInit::data).def("v_data", &OldStyleInit::v_data); + + py::class_ mixed_style_init(m, "MixedStyleInit"); + mixed_style_init.def(py::init()); + ignoreOldStyleInitWarnings([&mixed_style_init]() { + mixed_style_init.def("__init__", [](MixedStyleInit &self, const std::string &value) { + new (&self) MixedStyleInit(value); + }); + }); + mixed_style_init.def("data", &MixedStyleInit::data); + + // These functions intentionally do not dereference their arguments. They let the Python + // tests probe whether a type caster accepted reserved or uninitialized storage without + // invoking undefined behavior when testing a broken implementation. + m.def("accept_new_no_init", [](NewNoInit *value) { return value != nullptr; }); + m.def("accept_old_style_init", [](OldStyleInit *value) { return value != nullptr; }); + m.def("accept_mixed_style_init", [](MixedStyleInit *value) { return value != nullptr; }); } template diff --git a/tests/test_class.py b/tests/test_class.py index bb606eee65..f4beb3b5f2 100644 --- a/tests/test_class.py +++ b/tests/test_class.py @@ -3,6 +3,7 @@ import gc import pickle import sys +import threading from unittest import mock import pytest @@ -252,21 +253,36 @@ def __init__(self): assert msg(exc_info.value) == expected +def _record_uninitialized_load(seen, function, obj): + try: + seen["accepted"] = function(obj) + except ValueError as exc: + seen["error"] = exc + + +def _assert_uninitialized_load_rejected(seen): + assert "accepted" not in seen, "type caster accepted unconstructed storage" + assert isinstance(seen.get("error"), ValueError) + + def test_new_bypasses_init(): """`__new__` allocates the Python object but not the C++ one; using the instance before `__init__` has run must raise instead of segfaulting.""" - obj = m.NewNoInit.__new__(m.NewNoInit) - for use in (obj.data, obj.v_data, obj.__getstate__): + class PythonDerived(m.NewNoInit): + pass + + for cls in (m.NewNoInit, PythonDerived): + obj = cls.__new__(cls) with pytest.raises(ValueError) as exc_info: - use() + m.accept_new_no_init(obj) assert "Python instance is uninitialized" in str(exc_info.value) assert "NewNoInit" in str(exc_info.value) - # Calling `__init__()` is the sanctioned way to finish an object made with `__new__()`. - obj.__init__(42) - assert obj.data() == 42 - assert obj.v_data() == 42 + # Calling `__init__()` is the sanctioned way to finish an object made with `__new__()`. + obj.__init__(42) + assert obj.data() == 42 + assert obj.v_data() == 42 def test_new_then_setstate(): @@ -292,10 +308,10 @@ def test_failed_old_style_init_does_not_leave_lazy_storage(): with pytest.raises(RuntimeError, match="negative data"): obj.__init__(-1) - # The failed __init__ already lazily allocated storage for `self`, so without cleanup the - # uninitialized-instance guard never fires again and this reads a garbage vtable pointer. + # The failed __init__ already reserved storage for `self`. Without cleanup, a later load can + # mistake that storage for a constructed C++ object. with pytest.raises(ValueError, match="uninitialized"): - obj.v_data() + m.accept_old_style_init(obj) # A successful retry is still allowed. obj.__init__(42) @@ -310,19 +326,160 @@ def test_reentrant_load_during_new_style_init(): class Evil: def __index__(self): - # Runs during int conversion of the constructor argument, while - # construction_in_progress is set on `obj` and its C++ value is unconstructed. - try: - seen["data"] = obj.data() - except ValueError as exc: - seen["error"] = exc + # Runs during int conversion of the constructor argument while the C++ value is + # unconstructed. A new-style constructor never authorizes lazy allocation for it. + _record_uninitialized_load(seen, m.accept_new_no_init, obj) raise TypeError("stop the constructor") with pytest.raises(TypeError): obj.__init__(Evil()) - assert "data" not in seen, f"handed out uninitialized storage: {seen['data']!r}" - assert "error" in seen + _assert_uninitialized_load_rejected(seen) + + +def test_reentrant_load_during_old_style_init_argument_conversion(): + """Only the old-style constructor's own `self` load may reserve storage. Python called while + converting a later argument must not be able to load that storage as a C++ object.""" + obj = m.OldStyleInit.__new__(m.OldStyleInit) + seen = {} + + class ReenterThenFail: + def __index__(self): + _record_uninitialized_load(seen, m.accept_old_style_init, obj) + raise TypeError("stop the constructor") + + with pytest.raises(TypeError): + obj.__init__(ReenterThenFail()) + + _assert_uninitialized_load_rejected(seen) + with pytest.raises(ValueError): + m.accept_old_style_init(obj) + + # Failed conversion must release the reservation and leave a successful retry possible. + obj.__init__(42) + assert obj.data() == 42 + + +def test_reentrant_load_during_mixed_style_init(): + """An old-style overload must not authorize loads while a new-style candidate in the same + overload chain is being tried.""" + obj = m.MixedStyleInit.__new__(m.MixedStyleInit) + seen = {} + + class Reenter: + def __index__(self): + _record_uninitialized_load(seen, m.accept_mixed_style_init, obj) + return 42 + + obj.__init__(Reenter()) + _assert_uninitialized_load_rejected(seen) + assert obj.data() == 42 + + # Also retain coverage that the old-style overload itself remains usable. + assert m.MixedStyleInit("four").data() == 4 + + +def test_reentrant_load_during_old_style_setstate(): + """Old-style `__setstate__` may reserve storage for `self`, but Python executed inside its + callback before placement-new must still see the instance as unconstructed.""" + obj = m.OldStyleInit.__new__(m.OldStyleInit) + seen = {} + + class Reenter: + def __index__(self): + _record_uninitialized_load(seen, m.accept_old_style_init, obj) + return 43 + + obj.__setstate__(Reenter()) + _assert_uninitialized_load_rejected(seen) + assert obj.data() == 43 + + +def test_nested_old_style_init_is_rejected(): + """A nested initializer for the same reserved value must be rejected before its placement-new + callback runs; the outer initializer can then complete normally.""" + obj = m.OldStyleInit.__new__(m.OldStyleInit) + seen = {} + + class Reenter: + def __index__(self): + try: + obj.__init__(-1) + except Exception as exc: + seen["error"] = exc + else: + seen["accepted"] = True + return 44 + + obj.__init__(Reenter()) + assert "accepted" not in seen + assert isinstance(seen.get("error"), ValueError) + assert obj.data() == 44 + + +def test_old_style_init_does_not_authorize_another_python_mi_base(): + """Construction permission is for one exact value-and-holder, not every C++ base slot in the + same Python multiple-inheritance instance.""" + + class PythonMI(m.OldStyleInit, m.NewNoInit): + pass + + obj = PythonMI.__new__(PythonMI) + seen = {} + + class Reenter: + def __index__(self): + _record_uninitialized_load(seen, m.accept_new_no_init, obj) + return 45 + + m.OldStyleInit.__init__(obj, Reenter()) + _assert_uninitialized_load_rejected(seen) + assert obj.data() == 45 + + # Loading the unrelated base must remain rejected after the first base finishes construction. + with pytest.raises(ValueError): + m.accept_new_no_init(obj) + + +def test_old_style_init_does_not_authorize_another_thread(): + """While one thread is converting a later constructor argument, another thread must not load + the reserved storage. Events make the interleaving bounded and deterministic with or without + the GIL.""" + obj = m.OldStyleInit.__new__(m.OldStyleInit) + conversion_entered = threading.Event() + allow_conversion_to_finish = threading.Event() + thread_errors = [] + + class BlockingIndex: + def __index__(self): + conversion_entered.set() + if not allow_conversion_to_finish.wait(timeout=10): + raise RuntimeError("timed out waiting to finish conversion") + return 46 + + def initialize(): + try: + obj.__init__(BlockingIndex()) + except BaseException as exc: + thread_errors.append(exc) + + thread = threading.Thread(target=initialize) + thread.start() + try: + assert conversion_entered.wait(timeout=10), ( + "constructor did not enter conversion" + ) + with pytest.raises(ValueError): + m.accept_old_style_init(obj) + with pytest.raises(ValueError): + obj.__init__(47) + finally: + allow_conversion_to_finish.set() + thread.join(timeout=10) + + assert not thread.is_alive(), "constructor thread did not finish" + assert not thread_errors + assert obj.data() == 46 @pytest.mark.parametrize( From 4455e3f439bed9e8dd6a04cb87260f7d7486bb02 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 30 Aug 2026 13:45:11 -0700 Subject: [PATCH 04/19] test: skip constructor thread test on Emscripten --- tests/test_class.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test_class.py b/tests/test_class.py index f4beb3b5f2..0fa54f730e 100644 --- a/tests/test_class.py +++ b/tests/test_class.py @@ -441,6 +441,7 @@ def __index__(self): m.accept_new_no_init(obj) +@pytest.mark.skipif(sys.platform.startswith("emscripten"), reason="Requires threads") def test_old_style_init_does_not_authorize_another_thread(): """While one thread is converting a later constructor argument, another thread must not load the reserved storage. Events make the interleaving bounded and deterministic with or without From 14e32ae23af529df8d82681c2d3064884b259a3c Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 30 Aug 2026 14:42:37 -0700 Subject: [PATCH 05/19] fix: bump internals version to 13 The new detail::instance construction state has cross-DSO semantics that internals-v12 modules do not understand. Isolate the incompatible domains for v3.2.0 and document that future structural or semantic instance changes require another bump. --- include/pybind11/detail/common.h | 10 ++++++---- include/pybind11/detail/internals.h | 6 +++--- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/include/pybind11/detail/common.h b/include/pybind11/detail/common.h index d9ab8b751f..179f84e925 100644 --- a/include/pybind11/detail/common.h +++ b/include/pybind11/detail/common.h @@ -18,16 +18,16 @@ // See also: https://github.com/python/cpython/blob/HEAD/Include/patchlevel.h /* -- start version constants -- */ #define PYBIND11_VERSION_MAJOR 3 -#define PYBIND11_VERSION_MINOR 1 +#define PYBIND11_VERSION_MINOR 2 #define PYBIND11_VERSION_MICRO 0 // ALPHA = 0xA, BETA = 0xB, GAMMA = 0xC (release candidate), FINAL = 0xF (stable release) // - The release level is set to "alpha" for development versions. // Use 0xA0 (LEVEL=0xA, SERIAL=0) for development versions. // - For stable releases, set the serial to 0. -#define PYBIND11_VERSION_RELEASE_LEVEL PY_RELEASE_LEVEL_FINAL +#define PYBIND11_VERSION_RELEASE_LEVEL PY_RELEASE_LEVEL_ALPHA #define PYBIND11_VERSION_RELEASE_SERIAL 0 // String version of (micro, release level, release serial), e.g.: 0a0, 0b1, 0rc1, 0 -#define PYBIND11_VERSION_PATCH 0 +#define PYBIND11_VERSION_PATCH 0a0 /* -- end version constants -- */ #if !defined(Py_PACK_FULL_VERSION) @@ -632,7 +632,9 @@ struct nonsimple_values_and_holders { uint8_t *status; }; -/// The 'instance' type which needs to be standard layout (need to be able to use 'offsetof') +/// The 'instance' type which needs to be standard layout (need to be able to use 'offsetof'). +/// Changes to this struct or to the semantics of its members require incrementing +/// `PYBIND11_INTERNALS_VERSION`. struct instance { PyObject_HEAD /// Storage for pointers and holder; see simple_layout, below, for a description diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index 295485ffab..83b05a7e05 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -39,11 +39,11 @@ /// further ABI-incompatible changes may be made before the ABI is officially /// changed to the new version. #ifndef PYBIND11_INTERNALS_VERSION -# define PYBIND11_INTERNALS_VERSION 12 +# define PYBIND11_INTERNALS_VERSION 13 #endif -#if PYBIND11_INTERNALS_VERSION < 12 -# error "PYBIND11_INTERNALS_VERSION 12 is the minimum for all platforms for pybind11 v3.1.0" +#if PYBIND11_INTERNALS_VERSION < 13 +# error "PYBIND11_INTERNALS_VERSION 13 is the minimum for all platforms for pybind11 v3.2.0" #endif PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) From 9468fce7cd36a6818b0b20e7eac691656baf062d Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Wed, 2 Sep 2026 12:16:17 -0700 Subject: [PATCH 06/19] Revert "fix: bump internals version to 13" This reverts commit 14e32ae23af529df8d82681c2d3064884b259a3c. --- include/pybind11/detail/common.h | 10 ++++------ include/pybind11/detail/internals.h | 6 +++--- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/include/pybind11/detail/common.h b/include/pybind11/detail/common.h index 179f84e925..d9ab8b751f 100644 --- a/include/pybind11/detail/common.h +++ b/include/pybind11/detail/common.h @@ -18,16 +18,16 @@ // See also: https://github.com/python/cpython/blob/HEAD/Include/patchlevel.h /* -- start version constants -- */ #define PYBIND11_VERSION_MAJOR 3 -#define PYBIND11_VERSION_MINOR 2 +#define PYBIND11_VERSION_MINOR 1 #define PYBIND11_VERSION_MICRO 0 // ALPHA = 0xA, BETA = 0xB, GAMMA = 0xC (release candidate), FINAL = 0xF (stable release) // - The release level is set to "alpha" for development versions. // Use 0xA0 (LEVEL=0xA, SERIAL=0) for development versions. // - For stable releases, set the serial to 0. -#define PYBIND11_VERSION_RELEASE_LEVEL PY_RELEASE_LEVEL_ALPHA +#define PYBIND11_VERSION_RELEASE_LEVEL PY_RELEASE_LEVEL_FINAL #define PYBIND11_VERSION_RELEASE_SERIAL 0 // String version of (micro, release level, release serial), e.g.: 0a0, 0b1, 0rc1, 0 -#define PYBIND11_VERSION_PATCH 0a0 +#define PYBIND11_VERSION_PATCH 0 /* -- end version constants -- */ #if !defined(Py_PACK_FULL_VERSION) @@ -632,9 +632,7 @@ struct nonsimple_values_and_holders { uint8_t *status; }; -/// The 'instance' type which needs to be standard layout (need to be able to use 'offsetof'). -/// Changes to this struct or to the semantics of its members require incrementing -/// `PYBIND11_INTERNALS_VERSION`. +/// The 'instance' type which needs to be standard layout (need to be able to use 'offsetof') struct instance { PyObject_HEAD /// Storage for pointers and holder; see simple_layout, below, for a description diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index 83b05a7e05..295485ffab 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -39,11 +39,11 @@ /// further ABI-incompatible changes may be made before the ABI is officially /// changed to the new version. #ifndef PYBIND11_INTERNALS_VERSION -# define PYBIND11_INTERNALS_VERSION 13 +# define PYBIND11_INTERNALS_VERSION 12 #endif -#if PYBIND11_INTERNALS_VERSION < 13 -# error "PYBIND11_INTERNALS_VERSION 13 is the minimum for all platforms for pybind11 v3.2.0" +#if PYBIND11_INTERNALS_VERSION < 12 +# error "PYBIND11_INTERNALS_VERSION 12 is the minimum for all platforms for pybind11 v3.1.0" #endif PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) From bda11516e2dab21a34592739f852c499a96e2a7f Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Wed, 2 Sep 2026 13:45:53 -0700 Subject: [PATCH 07/19] fix: recover from legacy constructor storage collisions --- include/pybind11/detail/type_caster_base.h | 108 ++++++++++++++++++-- tests/CMakeLists.txt | 2 +- tests/pybind11_cross_module_tests.cpp | 51 ++++++++++ tests/test_class.cpp | 60 +++++++++++ tests/test_class.py | 111 +++++++++++++++++++++ 5 files changed, 321 insertions(+), 11 deletions(-) diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index bf6175162b..b722adf0b4 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -74,19 +74,86 @@ class loader_life_support { return lhs.inst == rhs.inst && lhs.vh == rhs.vh; } - void cleanup_old_style_init_storage() { - if (old_style_init_storage == nullptr) { + static void report_cleanup_error(const char *message, PyObject *context) noexcept { + error_scope scope; + PyErr_SetString(PyExc_RuntimeError, message); + PyErr_WriteUnraisable(context); + } + + // Invoke the deallocator selected by the DSO which registered `type` without exposing the + // private pointer through the real Python instance. In particular, this preserves matching + // class-specific and aligned operator new/delete behavior. + static void deallocate_unconstructed_storage(const type_info *type, void *storage) { + instance storage_instance{}; + storage_instance.owned = true; + storage_instance.simple_layout = true; + storage_instance.simple_holder_constructed = false; + storage_instance.simple_value_holder[0] = storage; + value_and_holder storage_v_h(&storage_instance, type, 0, 0); + type->dealloc(storage_v_h); + } + + static void deallocate_instance_value(value_and_holder &v_h) { + if (v_h.instance_registered()) { + if (!deregister_instance(v_h.inst, v_h.value_ptr(), v_h.type)) { + pybind11_fail("loader_life_support: could not deregister colliding storage"); + } + v_h.set_instance_registered(false); + } + v_h.type->dealloc(v_h); + } + + void cleanup_old_style_init_storage() noexcept { + void *const storage = old_style_init_storage; + if (storage == nullptr) { return; } + old_style_init_storage = nullptr; + auto v_h = *old_style_init_self; - scoped_critical_section lock( - handle(reinterpret_cast(old_style_init_self->inst))); - if (v_h.value_ptr() != nullptr) { - pybind11_fail("loader_life_support: old-style constructor storage collision"); + bool colliding_storage_cleanup_failed = false; + bool private_storage_is_published = false; + { + scoped_critical_section lock(handle(reinterpret_cast(v_h.inst))); + if (v_h.value_ptr() != nullptr) { + private_storage_is_published = v_h.value_ptr() == storage; + try { + // Roll back storage published by stale inline caster code before another + // overload candidate is attempted. If the private reservation itself was + // published, deallocate that allocation only once. + deallocate_instance_value(v_h); + } catch (...) { + // A throwing deallocator cannot be retried safely: it may already have partly + // destroyed the holder or released the allocation. Abandon the slot so that + // construction-scope or instance cleanup does not invoke it a second time. + v_h.set_holder_constructed(false); + v_h.set_instance_registered(false); + v_h.value_ptr() = nullptr; + colliding_storage_cleanup_failed = true; + } + } + } + + bool private_storage_cleanup_failed = false; + if (!private_storage_is_published) { + try { + deallocate_unconstructed_storage(v_h.type, storage); + } catch (...) { + private_storage_cleanup_failed = true; + } + } + + // PyErr_WriteUnraisable() can invoke user Python code. Keep it separate from the cleanup + // state mutation above. + auto *const context = reinterpret_cast(v_h.inst); + if (colliding_storage_cleanup_failed) { + report_cleanup_error( + "loader_life_support: failed to clean up colliding constructor storage", context); + } + if (private_storage_cleanup_failed) { + report_cleanup_error( + "loader_life_support: failed to clean up private constructor storage", context); } - v_h.value_ptr() = old_style_init_storage; - old_style_init_storage = nullptr; - v_h.type->dealloc(v_h); // Frees the storage and nulls the value pointer. } public: @@ -208,9 +275,30 @@ class loader_life_support { auto v_h = *frame->old_style_init_self; scoped_critical_section lock(handle(reinterpret_cast(v_h.inst))); - if (!v_h.value_constructing() || v_h.value_ptr() != nullptr) { + if (!v_h.value_constructing()) { pybind11_fail("loader_life_support: invalid old-style constructor commit"); } + + if (v_h.value_ptr() != nullptr) { + // A stale v12 caster can publish a second allocation while the updated constructor's + // storage is private. Roll the whole constructor transaction back, using the + // registered holder/deallocator to clean up the successfully placement-constructed + // private value just as it would clean up a normal instance. + void *const private_storage = frame->old_style_init_storage; + // From here onward the private value has been constructed. Do not let loader cleanup + // mistake it for raw storage if one of the rollback operations throws. + frame->old_style_init_storage = nullptr; + if (v_h.value_ptr() != private_storage) { + deallocate_instance_value(v_h); + v_h.value_ptr() = private_storage; + } + if (!v_h.holder_constructed()) { + v_h.type->init_instance(v_h.inst, nullptr); + } + deallocate_instance_value(v_h); + pybind11_fail("loader_life_support: old-style constructor storage collision"); + } + v_h.value_ptr() = frame->old_style_init_storage; frame->old_style_init_storage = nullptr; v_h.type->init_instance(v_h.inst, nullptr); diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index d6415b98bc..6d9138ebef 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -243,7 +243,7 @@ list(SORT PYBIND11_PYTEST_FILES) # built; if none of these are built (i.e. because TEST_OVERRIDE is used and # doesn't include them) the second module doesn't get built. tests_extra_targets( - "test_class_cross_module_use_after_one_module_dealloc.py;test_exceptions.py;test_local_bindings.py;test_stl.py;test_stl_binders.py" + "test_class.py;test_class_cross_module_use_after_one_module_dealloc.py;test_exceptions.py;test_local_bindings.py;test_stl.py;test_stl_binders.py" "pybind11_cross_module_tests") # And add additional targets for other tests. diff --git a/tests/pybind11_cross_module_tests.cpp b/tests/pybind11_cross_module_tests.cpp index 9a00c00ddc..0ed6b12c22 100644 --- a/tests/pybind11_cross_module_tests.cpp +++ b/tests/pybind11_cross_module_tests.cpp @@ -13,6 +13,8 @@ #include "pybind11_tests.h" #include "test_exceptions.h" +#include +#include #include #include @@ -25,9 +27,58 @@ class CrossDSOClass { CrossDSOClass::~CrossDSOClass() = default; +// Emulates the relevant lazy-publication fragment of stale inline type-caster code in an +// extension compiled with pybind11 v3.1 before PR #6157 (f90c430c). Such a module shares internals +// v12, but does not know about value_constructing and publishes raw storage when the value pointer +// is null. +class LegacyV12TypeCasterGeneric : public py::detail::type_caster_generic { +public: + using type_caster_generic::type_caster_generic; + + bool load(py::handle src, bool convert) { + return load_impl(src, convert); + } + + void load_value(py::detail::value_and_holder &&v_h) { + auto *&vptr = v_h.value_ptr(); + if (vptr == nullptr) { + const auto *type = v_h.type != nullptr ? v_h.type : typeinfo; + if (type->operator_new != nullptr) { + vptr = type->operator_new(type->type_size); + } else { +#if defined(__cpp_aligned_new) + if (type->type_align > __STDCPP_DEFAULT_NEW_ALIGNMENT__) { + vptr = ::operator new(type->type_size, std::align_val_t(type->type_align)); + } else { + vptr = ::operator new(type->type_size); + } +#else + vptr = ::operator new(type->type_size); +#endif + } + } + value = vptr; + } +}; + PYBIND11_MODULE(pybind11_cross_module_tests, m, py::mod_gil_not_used()) { m.doc() = "pybind11 cross-module test module"; + // test_old_style_init_legacy_v12_storage_collision + m.def("legacy_v12_pointer_only_load", [](py::handle src) { + // Deliberately resolve the registration by Python type rather than this DSO's typeid: the + // regression targets the historical shared-instance storage protocol, not RTTI lookup. + auto *tinfo = py::detail::get_type_info(Py_TYPE(src.ptr())); + if (tinfo == nullptr) { + throw py::type_error("No pybind11 type registration found"); + } + LegacyV12TypeCasterGeneric caster(tinfo); + if (!caster.load(src, false)) { + throw py::type_error("Legacy v12 type caster rejected the object"); + } + return reinterpret_cast(caster.value); + }); + // test_local_bindings.py tests: // // Definitions here are tested by importing both this module and the diff --git a/tests/test_class.cpp b/tests/test_class.cpp index 47e363b1d3..b0a174238d 100644 --- a/tests/test_class.cpp +++ b/tests/test_class.cpp @@ -98,6 +98,43 @@ struct OldStyleInit { virtual int v_data() const { return m_data; } }; +// test_old_style_init_legacy_v12_storage_collision +struct OldStyleInitCollisionStats { + int allocations = 0; + int deallocations = 0; + int constructions = 0; + int destructions = 0; +}; + +OldStyleInitCollisionStats &old_style_init_collision_stats() { + static OldStyleInitCollisionStats stats; + return stats; +} + +struct OldStyleInitCollision { + int m_data; + + explicit OldStyleInitCollision(int data) : m_data(data) { + ++old_style_init_collision_stats().constructions; + } + ~OldStyleInitCollision() { ++old_style_init_collision_stats().destructions; } + + static void *operator new(size_t size) { + ++old_style_init_collision_stats().allocations; + return ::operator new(size); + } + static void operator delete(void *ptr) noexcept { + ++old_style_init_collision_stats().deallocations; + ::operator delete(ptr); + } + + int data() const { return m_data; } +}; + +struct OldStyleInitCollisionSmart : OldStyleInitCollision { + explicit OldStyleInitCollisionSmart(int data) : OldStyleInitCollision(data) {} +}; + // test_reentrant_load_during_mixed_style_init struct MixedStyleInit { int m_data; @@ -660,6 +697,29 @@ TEST_SUBMODULE(class_, m) { }); old_style_init.def("data", &OldStyleInit::data).def("v_data", &OldStyleInit::v_data); + py::class_ old_style_init_collision(m, "OldStyleInitCollision"); + ignoreOldStyleInitWarnings([&old_style_init_collision]() { + old_style_init_collision.def("__init__", [](OldStyleInitCollision &self, int x) { + ::new (static_cast(&self)) OldStyleInitCollision(x); + }); + }); + old_style_init_collision.def("data", &OldStyleInitCollision::data); + py::class_ old_style_init_collision_smart( + m, "OldStyleInitCollisionSmart"); + ignoreOldStyleInitWarnings([&old_style_init_collision_smart]() { + old_style_init_collision_smart.def( + "__init__", [](OldStyleInitCollisionSmart &self, int x) { + ::new (static_cast(&self)) OldStyleInitCollisionSmart(x); + }); + }); + old_style_init_collision_smart.def("data", &OldStyleInitCollisionSmart::data); + m.def("reset_old_style_init_collision_stats", []() { old_style_init_collision_stats() = {}; }); + m.def("old_style_init_collision_stats", []() { + const auto &stats = old_style_init_collision_stats(); + return py::make_tuple( + stats.allocations, stats.deallocations, stats.constructions, stats.destructions); + }); + py::class_ mixed_style_init(m, "MixedStyleInit"); mixed_style_init.def(py::init()); ignoreOldStyleInitWarnings([&mixed_style_init]() { diff --git a/tests/test_class.py b/tests/test_class.py index 0fa54f730e..e67c1989a3 100644 --- a/tests/test_class.py +++ b/tests/test_class.py @@ -318,6 +318,117 @@ def test_failed_old_style_init_does_not_leave_lazy_storage(): assert obj.v_data() == 42 +def test_old_style_init_legacy_v12_storage_collision(): + """A stale v12 caster can publish competing storage while an updated old-style constructor + keeps its storage private. For owning default and smart holders, collision rollback must not + throw from loader_life_support's destructor, leak either allocation, or prevent a retry.""" + env.check_script_success_in_subprocess( + """ + import gc + import sys + + import pybind11_cross_module_tests as cm + from pybind11_tests import class_ as m + + + def counts(): + return m.old_style_init_collision_stats() + + + def collect(): + gc.collect() + gc.collect() + + + unraisable = [] + sys.unraisablehook = lambda args: unraisable.append(str(args.exc_value)) + + # If conversion fails after the stale caster publishes storage, both raw allocations are + # rolled back without replacing normal conversion failure with a cleanup error or process + # termination. The same object remains usable. + m.reset_old_style_init_collision_stats() + obj = m.OldStyleInitCollision.__new__(m.OldStyleInitCollision) + seen = {} + + class ReenterThenFail: + def __index__(self): + seen["address"] = cm.legacy_v12_pointer_only_load(obj) + raise TypeError("stop the constructor") + + try: + obj.__init__(ReenterThenFail()) + except TypeError: + pass + else: + raise AssertionError("argument conversion unexpectedly succeeded") + # Rollback invalidates the stale address; observing the integer proves only that the + # legacy publication path ran. Arbitrary escaped v12 pointers cannot be made safe here. + assert isinstance(seen.get("address"), int) + assert counts() == (2, 2, 0, 0) + + obj.__init__(41) + assert obj.data() == 41 + assert counts() == (3, 2, 1, 0) + del obj + collect() + assert counts() == (3, 3, 1, 1) + + # If the C++ callback completed before the collision is detected, rollback must construct + # the real holder temporarily so that the private C++ value's destructor runs exactly once. + m.reset_old_style_init_collision_stats() + obj = m.OldStyleInitCollision.__new__(m.OldStyleInitCollision) + seen = {} + + class ReenterThenSucceed: + def __index__(self): + seen["address"] = cm.legacy_v12_pointer_only_load(obj) + return 42 + + try: + obj.__init__(ReenterThenSucceed()) + except RuntimeError as exc: + assert "old-style constructor storage collision" in str(exc) + else: + raise AssertionError("storage collision unexpectedly committed") + assert isinstance(seen.get("address"), int) + assert counts() == (2, 2, 1, 1) + + obj.__init__(43) + assert obj.data() == 43 + assert counts() == (3, 2, 2, 1) + del obj + collect() + assert counts() == (3, 3, 2, 2) + + # Exercise the same rollback through smart_holder, whose ownership machinery differs from + # the default holder and must be initialized before a constructed private value is retired. + m.reset_old_style_init_collision_stats() + obj = m.OldStyleInitCollisionSmart.__new__(m.OldStyleInitCollisionSmart) + + class ReenterSmart: + def __index__(self): + cm.legacy_v12_pointer_only_load(obj) + return 44 + + try: + obj.__init__(ReenterSmart()) + except RuntimeError as exc: + assert "old-style constructor storage collision" in str(exc) + else: + raise AssertionError("smart-holder storage collision unexpectedly committed") + assert counts() == (2, 2, 1, 1) + + obj.__init__(45) + assert obj.data() == 45 + del obj + collect() + assert counts() == (3, 3, 2, 2) + assert unraisable == [] + """, + rerun=1, + ) + + 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.""" From 23f2d0a7354552eb69552b4f3d8f1e64355ed218 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Wed, 2 Sep 2026 14:26:07 -0700 Subject: [PATCH 08/19] test: fix collision subprocess imports --- tests/test_class.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/tests/test_class.py b/tests/test_class.py index e67c1989a3..5cbbd0b89c 100644 --- a/tests/test_class.py +++ b/tests/test_class.py @@ -1,6 +1,7 @@ from __future__ import annotations import gc +import os import pickle import sys import threading @@ -323,10 +324,12 @@ def test_old_style_init_legacy_v12_storage_collision(): keeps its storage private. For owning default and smart holders, collision rollback must not throw from loader_life_support's destructor, leak either allocation, or prevent a retry.""" env.check_script_success_in_subprocess( - """ + f""" import gc import sys + sys.path.insert(0, {os.path.dirname(env.__file__)!r}) + import pybind11_cross_module_tests as cm from pybind11_tests import class_ as m @@ -348,7 +351,7 @@ def collect(): # termination. The same object remains usable. m.reset_old_style_init_collision_stats() obj = m.OldStyleInitCollision.__new__(m.OldStyleInitCollision) - seen = {} + seen = {{}} class ReenterThenFail: def __index__(self): @@ -377,7 +380,7 @@ def __index__(self): # the real holder temporarily so that the private C++ value's destructor runs exactly once. m.reset_old_style_init_collision_stats() obj = m.OldStyleInitCollision.__new__(m.OldStyleInitCollision) - seen = {} + seen = {{}} class ReenterThenSucceed: def __index__(self): From 89a5f72e7b0f134e166967f80cc2f8c7de8291b2 Mon Sep 17 00:00:00 2001 From: Henry Schreiner Date: Thu, 3 Sep 2026 00:26:08 -0400 Subject: [PATCH 09/19] refactor: simplify old-style constructor storage tracking The loader frame already identifies the constructor candidate, so the one-shot `self` permission only needs a frame match and a claimed flag. This removes both argument guard classes, the changes to cast.h, and the per-call TLS lookups they added. Also: - Hoist deallocate_instance_value to a detail free function and use it from instance_construction_scope. - Take the dispatcher's constructor lock before the construction scope and drop the nested critical sections it made redundant. - Keep the non-constructor path inline: the loader destructor checks for storage before the out-of-line cleanup, and the construction scope defaults to not started. - Commit old-style storage once in cpp_function::initialize, gated on is_constructor. - Share one __index__ probe across the reentrancy tests and turn the subprocess script into a plain function. Assisted-by: ClaudeCode:claude-fable-5-1 --- include/pybind11/cast.h | 11 +- include/pybind11/detail/type_caster_base.h | 216 +++++++-------------- include/pybind11/pybind11.h | 47 +++-- tests/pybind11_cross_module_tests.cpp | 16 +- tests/test_class.py | 209 ++++++++------------ 5 files changed, 181 insertions(+), 318 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 2c517cec69..1d857a0ed5 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -2179,12 +2179,6 @@ class argument_loader { private: static bool load_impl_sequence(function_call &, index_sequence<>) { return true; } - template - bool load_one(function_call &call) { - loader_life_support::argument_load_guard guard(I == 0); - return std::get(argcasters).load(call.args[I], call.args_convert[I]); - } - template bool load_impl_sequence(function_call &call, index_sequence) { PYBIND11_WARNING_PUSH @@ -2193,11 +2187,11 @@ class argument_loader { PYBIND11_WARNING_DISABLE_GCC("-Warray-bounds") #endif #ifdef __cpp_fold_expressions - if ((... || !load_one(call))) { + if ((... || !std::get(argcasters).load(call.args[Is], call.args_convert[Is]))) { return false; } #else - for (bool r : {load_one(call)...}) { + for (bool r : {std::get(argcasters).load(call.args[Is], call.args_convert[Is])...}) { if (!r) { return false; } @@ -2209,7 +2203,6 @@ class argument_loader { template Return call_impl(Func &&f, index_sequence, Guard &&) && { - loader_life_support::old_style_init_call_guard old_style_init_guard; return std::forward(f)(cast_op(std::move(std::get(argcasters)))...); } diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index b722adf0b4..8f034f4d88 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -39,6 +39,18 @@ PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) PYBIND11_NAMESPACE_BEGIN(detail) +/// Discards a value that was published in `v_h` but whose construction did not complete. +inline void deallocate_instance_value(value_and_holder &v_h) { + if (v_h.instance_registered()) { + if (!deregister_instance(v_h.inst, v_h.value_ptr(), v_h.type)) { + pybind11_fail( + "deallocate_instance_value(): Tried to deallocate unregistered instance!"); + } + v_h.set_instance_registered(false); + } + v_h.type->dealloc(v_h); +} + /// A life support system for temporary objects created by `type_caster::load()`. /// Adding a patient will keep it alive up until the enclosing function returns. class loader_life_support { @@ -64,10 +76,10 @@ class loader_life_support { // Old-style placement-new constructors need raw storage while loading their `self` // argument. Keep it private to the exact overload candidate until its C++ callable returns. + // The dispatcher holds the instance critical section for the lifetime of such a frame. value_and_holder *old_style_init_self = nullptr; void *old_style_init_storage = nullptr; - bool old_style_init_self_load_allowed = false; - bool old_style_init_self_load_claimed = false; + bool old_style_init_self_claimed = false; static bool is_same_value_and_holder(const value_and_holder &lhs, const value_and_holder &rhs) { @@ -93,67 +105,42 @@ class loader_life_support { type->dealloc(storage_v_h); } - static void deallocate_instance_value(value_and_holder &v_h) { - if (v_h.instance_registered()) { - if (!deregister_instance(v_h.inst, v_h.value_ptr(), v_h.type)) { - pybind11_fail("loader_life_support: could not deregister colliding storage"); - } - v_h.set_instance_registered(false); - } - v_h.type->dealloc(v_h); - } - - void cleanup_old_style_init_storage() noexcept { + PYBIND11_NOINLINE void cleanup_old_style_init_storage() noexcept { void *const storage = old_style_init_storage; - if (storage == nullptr) { - return; - } old_style_init_storage = nullptr; auto v_h = *old_style_init_self; - bool colliding_storage_cleanup_failed = false; + auto *const context = reinterpret_cast(v_h.inst); bool private_storage_is_published = false; - { - scoped_critical_section lock(handle(reinterpret_cast(v_h.inst))); - if (v_h.value_ptr() != nullptr) { - private_storage_is_published = v_h.value_ptr() == storage; - try { - // Roll back storage published by stale inline caster code before another - // overload candidate is attempted. If the private reservation itself was - // published, deallocate that allocation only once. - deallocate_instance_value(v_h); - } catch (...) { - // A throwing deallocator cannot be retried safely: it may already have partly - // destroyed the holder or released the allocation. Abandon the slot so that - // construction-scope or instance cleanup does not invoke it a second time. - v_h.set_holder_constructed(false); - v_h.set_instance_registered(false); - v_h.value_ptr() = nullptr; - colliding_storage_cleanup_failed = true; - } + if (v_h.value_ptr() != nullptr) { + private_storage_is_published = v_h.value_ptr() == storage; + try { + // Roll back storage published by stale inline caster code before another + // overload candidate is attempted. If the private reservation itself was + // published, deallocate that allocation only once. + deallocate_instance_value(v_h); + } catch (...) { + // A throwing deallocator cannot be retried safely: it may already have partly + // destroyed the holder or released the allocation. Abandon the slot so that + // construction-scope or instance cleanup does not invoke it a second time. + // Reset the slot before PyErr_WriteUnraisable() can run user Python code. + v_h.set_holder_constructed(false); + v_h.set_instance_registered(false); + v_h.value_ptr() = nullptr; + report_cleanup_error( + "loader_life_support: failed to clean up colliding constructor storage", + context); } } - - bool private_storage_cleanup_failed = false; if (!private_storage_is_published) { try { deallocate_unconstructed_storage(v_h.type, storage); } catch (...) { - private_storage_cleanup_failed = true; + report_cleanup_error( + "loader_life_support: failed to clean up private constructor storage", + context); } } - - // PyErr_WriteUnraisable() can invoke user Python code. Keep it separate from the cleanup - // state mutation above. - auto *const context = reinterpret_cast(v_h.inst); - if (colliding_storage_cleanup_failed) { - report_cleanup_error( - "loader_life_support: failed to clean up colliding constructor storage", context); - } - if (private_storage_cleanup_failed) { - report_cleanup_error( - "loader_life_support: failed to clean up private constructor storage", context); - } } public: @@ -172,77 +159,29 @@ class loader_life_support { pybind11_fail("loader_life_support: internal error"); } frame = parent; - cleanup_old_style_init_storage(); + if (old_style_init_storage != nullptr) { + cleanup_old_style_init_storage(); + } for (auto *item : keep_alive) { Py_DECREF(item); } } - /// Restricts the special old-style constructor permission to argument zero of the current - /// candidate. A nested bound call has its own loader frame and cannot inherit this permission. - class argument_load_guard { - public: - explicit argument_load_guard(bool is_first_argument) { - auto *current = tls_current_frame(); - if (is_first_argument && current != nullptr && current->old_style_init_self != nullptr - && !current->old_style_init_self_load_claimed) { - frame = current; - frame->old_style_init_self_load_allowed = true; - } - } - ~argument_load_guard() { - if (frame != nullptr) { - frame->old_style_init_self_load_allowed = false; - } - } - argument_load_guard(const argument_load_guard &) = delete; - argument_load_guard &operator=(const argument_load_guard &) = delete; - - private: - loader_life_support *frame = nullptr; - }; - - /// Some legacy `__setstate__` implementations accept `self` as `py::object` and perform the - /// typed cast inside the C++ callable. At that point all other arguments have finished - /// loading, so granting the same exact, one-shot permission is safe. Reentrant bound calls - /// still get a separate loader frame. - class old_style_init_call_guard { - public: - old_style_init_call_guard() { - auto *current = tls_current_frame(); - if (current != nullptr && current->old_style_init_self != nullptr - && !current->old_style_init_self_load_claimed) { - frame = current; - frame->old_style_init_self_load_allowed = true; - } - } - ~old_style_init_call_guard() { - if (frame != nullptr) { - frame->old_style_init_self_load_allowed = false; - } - } - old_style_init_call_guard(const old_style_init_call_guard &) = delete; - old_style_init_call_guard &operator=(const old_style_init_call_guard &) = delete; - - private: - loader_life_support *frame = nullptr; - }; - - /// Claims and allocates the private storage for the exact old-style constructor `self` load. + /// Claims and allocates the private storage for the one old-style constructor `self` load of + /// the current frame; returns nullptr for every other load. `self` is loaded first, or cast + /// inside a legacy `py::object` callback after all arguments were loaded. Nested bound calls + /// and other threads have their own frames and never qualify. /// The permission is consumed before invoking a potentially user-defined operator new. - static bool try_reserve_old_style_init_storage(value_and_holder &v_h, - const type_info *type, - void *&value) { + static void *try_reserve_old_style_init_storage(value_and_holder &v_h, const type_info *type) { auto *frame = tls_current_frame(); - if (frame == nullptr || !frame->old_style_init_self_load_allowed - || frame->old_style_init_self_load_claimed || frame->old_style_init_self == nullptr + if (frame == nullptr || frame->old_style_init_self == nullptr + || frame->old_style_init_self_claimed || !is_same_value_and_holder(v_h, *frame->old_style_init_self) || v_h.value_ptr() != nullptr) { - return false; + return nullptr; } - frame->old_style_init_self_load_claimed = true; - frame->old_style_init_self_load_allowed = false; + frame->old_style_init_self_claimed = true; if (type->operator_new) { frame->old_style_init_storage = type->operator_new(type->type_size); } else { @@ -260,8 +199,7 @@ class loader_life_support { if (frame->old_style_init_storage == nullptr) { throw std::bad_alloc(); } - value = frame->old_style_init_storage; - return true; + return frame->old_style_init_storage; } /// Publishes a successfully placement-constructed value and immediately finalizes its holder. @@ -274,7 +212,6 @@ class loader_life_support { } auto v_h = *frame->old_style_init_self; - scoped_critical_section lock(handle(reinterpret_cast(v_h.inst))); if (!v_h.value_constructing()) { pybind11_fail("loader_life_support: invalid old-style constructor commit"); } @@ -762,15 +699,28 @@ PYBIND11_NOINLINE void instance::deallocate_layout() { /// Marks the exact value slot targeted by a constructor. This state is never permission to load /// the value: every load is rejected until construction finishes, apart from the one-shot /// old-style constructor `self` permission maintained by `loader_life_support`. +/// The dispatcher creates and destroys this scope while holding the instance critical section. class instance_construction_scope { public: explicit instance_construction_scope(value_and_holder *v_h) { - if (v_h == nullptr) { - return; + if (v_h != nullptr) { + start(*v_h); + } + } + ~instance_construction_scope() { + if (started_) { + finish(); } - v_h_ = *v_h; - started_ = false; - scoped_critical_section lock(handle(reinterpret_cast(v_h_.inst))); + } + instance_construction_scope(const instance_construction_scope &) = delete; + instance_construction_scope &operator=(const instance_construction_scope &) = delete; + + bool started() const { return started_; } + bool already_registered() const { return already_registered_; } + +private: + PYBIND11_NOINLINE void start(const value_and_holder &v_h) { + v_h_ = v_h; if (v_h_.value_constructing()) { return; } @@ -782,32 +732,17 @@ class instance_construction_scope { v_h_.set_value_constructing(); started_ = true; } - ~instance_construction_scope() { - if (!started_ || v_h_.inst == nullptr) { - return; - } - - scoped_critical_section lock(handle(reinterpret_cast(v_h_.inst))); + PYBIND11_NOINLINE void finish() { // A failed new-style constructor can have published a value without completing its // holder. Preserve the existing cleanup guarantee for that case. if (value_was_null_ && !v_h_.holder_constructed() && v_h_.value_ptr() != nullptr) { - if (v_h_.instance_registered()) { - deregister_instance(v_h_.inst, v_h_.value_ptr(), v_h_.type); - v_h_.set_instance_registered(false); - } - v_h_.type->dealloc(v_h_); + deallocate_instance_value(v_h_); } v_h_.set_value_constructing(false); } - instance_construction_scope(const instance_construction_scope &) = delete; - instance_construction_scope &operator=(const instance_construction_scope &) = delete; - bool started() const { return started_; } - bool already_registered() const { return already_registered_; } - -private: value_and_holder v_h_; - bool started_ = true; + bool started_ = false; bool already_registered_ = false; bool value_was_null_ = false; }; @@ -1410,14 +1345,13 @@ class type_caster_generic { // A non-null value pointer is not sufficient while a constructor is running: old-style // placement-new storage may exist before the C++ object's lifetime has begun. Only the - // exact argument-zero load of the current old-style constructor may access private raw + // one `self` load of the current old-style constructor candidate may access private raw // storage; reentrant, cross-base, nested, and cross-thread loads must all fail. if (v_h.value_constructing()) { - void *reserved_value = nullptr; const auto *type = v_h.type ? v_h.type : typeinfo; - if (loader_life_support::try_reserve_old_style_init_storage( - v_h, type, reserved_value)) { - value = reserved_value; + if (void *reserved + = loader_life_support::try_reserve_old_style_init_storage(v_h, type)) { + value = reserved; return; } throw value_error("Missing value for wrapped C++ type `" diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index ae4c45a802..1edd8fa7bd 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -510,19 +510,16 @@ class cpp_function : public function { = return_value_policy_override::policy(call.func.policy); /* Perform the function call */ - handle result; - if (call.func.is_setter) { - (void) std::move(args_converter).template call(f); - loader_life_support::complete_old_style_init(); - result = none().release(); - } else { - auto &&cpp_result = std::move(args_converter).template call(f); + auto &&cpp_result = std::move(args_converter).template call(f); + if (call.func.is_constructor) { + // Publish old-style constructor storage before return-value conversion can observe + // the instance. loader_life_support::complete_old_style_init(); - result = cast_out::cast( - std::forward(cpp_result), policy, call.parent); } - - return result; + if (call.func.is_setter) { + return none().release(); + } + return cast_out::cast(std::forward(cpp_result), policy, call.parent); } protected: @@ -998,10 +995,17 @@ class cpp_function : public function { self_value_and_holder = pi->get_value_and_holder(tinfo, true); } + // On free-threaded Python, serialize the complete constructor transaction. Python + // critical sections are suspended around blocking operations, allowing another thread to + // enter, observe `value_constructing`, and reject access without racing status-byte + // updates. + scoped_critical_section constructor_lock(overloads->is_constructor ? parent : handle{}); + detail::instance_construction_scope construction_scope( overloads->is_constructor ? &self_value_and_holder : nullptr); if (overloads->is_constructor) { - // Invoking __init__ repeatedly on an already constructed value remains a no-op. + // If this value is already registered it must mean __init__ is invoked multiple times; + // we really can't support that in C++, so just ignore the second __init__. if (construction_scope.already_registered()) { return none().release().ptr(); } @@ -1013,11 +1017,11 @@ class cpp_function : public function { } } - // On free-threaded Python, serialize the complete constructor transaction. Python - // critical sections are suspended around blocking operations, allowing another thread to - // enter, observe `value_constructing`, and reject access without racing status-byte - // updates. - scoped_critical_section constructor_lock(overloads->is_constructor ? parent : handle{}); + // Old-style constructors reserve private `self` storage through their loader frame. + auto old_style_init_self = [&](const function_record &f) -> value_and_holder * { + return f.is_constructor && !f.is_new_style_constructor ? &self_value_and_holder + : nullptr; + }; try { // We do this in two passes: in the first pass, we load arguments with `convert=false`; @@ -1251,9 +1255,7 @@ class cpp_function : public function { // 6. Call the function. try { - loader_life_support guard{func.is_constructor && !func.is_new_style_constructor - ? &self_value_and_holder - : nullptr}; + loader_life_support guard{old_style_init_self(func)}; result = func.impl(call); } catch (reference_cast_error &) { result = PYBIND11_TRY_NEXT_OVERLOAD; @@ -1284,10 +1286,7 @@ class cpp_function : public function { // allowed for (auto &call : second_pass) { try { - loader_life_support guard{call.func.is_constructor - && !call.func.is_new_style_constructor - ? &self_value_and_holder - : nullptr}; + loader_life_support guard{old_style_init_self(call.func)}; result = call.func.impl(call); } catch (reference_cast_error &) { result = PYBIND11_TRY_NEXT_OVERLOAD; diff --git a/tests/pybind11_cross_module_tests.cpp b/tests/pybind11_cross_module_tests.cpp index 0ed6b12c22..c2fa29bdf8 100644 --- a/tests/pybind11_cross_module_tests.cpp +++ b/tests/pybind11_cross_module_tests.cpp @@ -42,20 +42,10 @@ class LegacyV12TypeCasterGeneric : public py::detail::type_caster_generic { void load_value(py::detail::value_and_holder &&v_h) { auto *&vptr = v_h.value_ptr(); if (vptr == nullptr) { + // The stale code also has an over-aligned fallback, which the test types never reach. const auto *type = v_h.type != nullptr ? v_h.type : typeinfo; - if (type->operator_new != nullptr) { - vptr = type->operator_new(type->type_size); - } else { -#if defined(__cpp_aligned_new) - if (type->type_align > __STDCPP_DEFAULT_NEW_ALIGNMENT__) { - vptr = ::operator new(type->type_size, std::align_val_t(type->type_align)); - } else { - vptr = ::operator new(type->type_size); - } -#else - vptr = ::operator new(type->type_size); -#endif - } + vptr = type->operator_new != nullptr ? type->operator_new(type->type_size) + : ::operator new(type->type_size); } value = vptr; } diff --git a/tests/test_class.py b/tests/test_class.py index 5cbbd0b89c..5c43686814 100644 --- a/tests/test_class.py +++ b/tests/test_class.py @@ -266,6 +266,23 @@ def _assert_uninitialized_load_rejected(seen): assert isinstance(seen.get("error"), ValueError) +class _LoadOnIndex: + """Constructor argument whose int conversion runs `function(obj)` while the C++ value is + still unconstructed, then returns `result` or, if that is None, aborts the constructor.""" + + def __init__(self, seen, function, obj, result=None): + self.seen = seen + self.function = function + self.obj = obj + self.result = result + + def __index__(self): + _record_uninitialized_load(self.seen, self.function, self.obj) + if self.result is None: + raise TypeError("stop the constructor") + return self.result + + def test_new_bypasses_init(): """`__new__` allocates the Python object but not the C++ one; using the instance before `__init__` has run must raise instead of segfaulting.""" @@ -319,114 +336,78 @@ def test_failed_old_style_init_does_not_leave_lazy_storage(): assert obj.v_data() == 42 -def test_old_style_init_legacy_v12_storage_collision(): - """A stale v12 caster can publish competing storage while an updated old-style constructor - keeps its storage private. For owning default and smart holders, collision rollback must not - throw from loader_life_support's destructor, leak either allocation, or prevent a retry.""" - env.check_script_success_in_subprocess( - f""" - import gc - import sys +def _check_legacy_v12_storage_collision(): + """Body of test_old_style_init_legacy_v12_storage_collision, run in a subprocess.""" + import pybind11_cross_module_tests as cm - sys.path.insert(0, {os.path.dirname(env.__file__)!r}) - - import pybind11_cross_module_tests as cm - from pybind11_tests import class_ as m - - - def counts(): - return m.old_style_init_collision_stats() + unraisable = [] + sys.unraisablehook = lambda args: unraisable.append(str(args.exc_value)) + class LegacyLoadOnIndex: + """Loads `obj` through the stale caster during argument conversion.""" - def collect(): - gc.collect() - gc.collect() - - - unraisable = [] - sys.unraisablehook = lambda args: unraisable.append(str(args.exc_value)) - - # If conversion fails after the stale caster publishes storage, both raw allocations are - # rolled back without replacing normal conversion failure with a cleanup error or process - # termination. The same object remains usable. - m.reset_old_style_init_collision_stats() - obj = m.OldStyleInitCollision.__new__(m.OldStyleInitCollision) - seen = {{}} + def __init__(self, obj, seen, result): + self.obj = obj + self.seen = seen + self.result = result - class ReenterThenFail: - def __index__(self): - seen["address"] = cm.legacy_v12_pointer_only_load(obj) + def __index__(self): + self.seen["address"] = cm.legacy_v12_pointer_only_load(self.obj) + if self.result is None: raise TypeError("stop the constructor") - - try: - obj.__init__(ReenterThenFail()) - except TypeError: - pass - else: - raise AssertionError("argument conversion unexpectedly succeeded") + return self.result + + def stats(): + return m.old_style_init_collision_stats() + + for cls, result, exc, constructed in [ + # Conversion fails after the stale caster publishes storage: both raw allocations are + # rolled back without replacing the conversion failure with a cleanup error. + (m.OldStyleInitCollision, None, TypeError, 0), + # The C++ callback completed before the collision is detected: rollback constructs the + # real holder temporarily so that the private value's destructor runs exactly once. + (m.OldStyleInitCollision, 42, RuntimeError, 1), + # smart_holder ownership must be initialized before a constructed private value is retired. + (m.OldStyleInitCollisionSmart, 44, RuntimeError, 1), + ]: + m.reset_old_style_init_collision_stats() + obj = cls.__new__(cls) + seen = {} + # A failing `__index__` surfaces as the generic overload-resolution TypeError. + match = "storage collision" if exc is RuntimeError else None + with pytest.raises(exc, match=match): + obj.__init__(LegacyLoadOnIndex(obj, seen, result)) # Rollback invalidates the stale address; observing the integer proves only that the # legacy publication path ran. Arbitrary escaped v12 pointers cannot be made safe here. assert isinstance(seen.get("address"), int) - assert counts() == (2, 2, 0, 0) + assert stats() == (2, 2, constructed, constructed) - obj.__init__(41) - assert obj.data() == 41 - assert counts() == (3, 2, 1, 0) + # The same object remains usable. + obj.__init__(43) + assert obj.data() == 43 + assert stats() == (3, 2, constructed + 1, constructed) del obj - collect() - assert counts() == (3, 3, 1, 1) + gc.collect() + gc.collect() + assert stats() == (3, 3, constructed + 1, constructed + 1) - # If the C++ callback completed before the collision is detected, rollback must construct - # the real holder temporarily so that the private C++ value's destructor runs exactly once. - m.reset_old_style_init_collision_stats() - obj = m.OldStyleInitCollision.__new__(m.OldStyleInitCollision) - seen = {{}} - - class ReenterThenSucceed: - def __index__(self): - seen["address"] = cm.legacy_v12_pointer_only_load(obj) - return 42 + assert unraisable == [] - try: - obj.__init__(ReenterThenSucceed()) - except RuntimeError as exc: - assert "old-style constructor storage collision" in str(exc) - else: - raise AssertionError("storage collision unexpectedly committed") - assert isinstance(seen.get("address"), int) - assert counts() == (2, 2, 1, 1) - obj.__init__(43) - assert obj.data() == 43 - assert counts() == (3, 2, 2, 1) - del obj - collect() - assert counts() == (3, 3, 2, 2) +def test_old_style_init_legacy_v12_storage_collision(): + """A stale v12 caster can publish competing storage while an updated old-style constructor + keeps its storage private. For owning default and smart holders, collision rollback must not + throw from loader_life_support's destructor, leak either allocation, or prevent a retry. + A regression can terminate the process, so the checks run in a subprocess.""" + env.check_script_success_in_subprocess( + f""" + import sys - # Exercise the same rollback through smart_holder, whose ownership machinery differs from - # the default holder and must be initialized before a constructed private value is retired. - m.reset_old_style_init_collision_stats() - obj = m.OldStyleInitCollisionSmart.__new__(m.OldStyleInitCollisionSmart) + sys.path.insert(0, {os.path.dirname(env.__file__)!r}) - class ReenterSmart: - def __index__(self): - cm.legacy_v12_pointer_only_load(obj) - return 44 + import test_class - try: - obj.__init__(ReenterSmart()) - except RuntimeError as exc: - assert "old-style constructor storage collision" in str(exc) - else: - raise AssertionError("smart-holder storage collision unexpectedly committed") - assert counts() == (2, 2, 1, 1) - - obj.__init__(45) - assert obj.data() == 45 - del obj - collect() - assert counts() == (3, 3, 2, 2) - assert unraisable == [] + test_class._check_legacy_v12_storage_collision() """, rerun=1, ) @@ -437,17 +418,8 @@ def test_reentrant_load_during_new_style_init(): to another bound function while `__init__` runs must raise, not hand out garbage.""" obj = m.NewNoInit.__new__(m.NewNoInit) seen = {} - - class Evil: - def __index__(self): - # Runs during int conversion of the constructor argument while the C++ value is - # unconstructed. A new-style constructor never authorizes lazy allocation for it. - _record_uninitialized_load(seen, m.accept_new_no_init, obj) - raise TypeError("stop the constructor") - with pytest.raises(TypeError): - obj.__init__(Evil()) - + obj.__init__(_LoadOnIndex(seen, m.accept_new_no_init, obj)) _assert_uninitialized_load_rejected(seen) @@ -456,15 +428,8 @@ def test_reentrant_load_during_old_style_init_argument_conversion(): converting a later argument must not be able to load that storage as a C++ object.""" obj = m.OldStyleInit.__new__(m.OldStyleInit) seen = {} - - class ReenterThenFail: - def __index__(self): - _record_uninitialized_load(seen, m.accept_old_style_init, obj) - raise TypeError("stop the constructor") - with pytest.raises(TypeError): - obj.__init__(ReenterThenFail()) - + obj.__init__(_LoadOnIndex(seen, m.accept_old_style_init, obj)) _assert_uninitialized_load_rejected(seen) with pytest.raises(ValueError): m.accept_old_style_init(obj) @@ -479,13 +444,7 @@ def test_reentrant_load_during_mixed_style_init(): overload chain is being tried.""" obj = m.MixedStyleInit.__new__(m.MixedStyleInit) seen = {} - - class Reenter: - def __index__(self): - _record_uninitialized_load(seen, m.accept_mixed_style_init, obj) - return 42 - - obj.__init__(Reenter()) + obj.__init__(_LoadOnIndex(seen, m.accept_mixed_style_init, obj, 42)) _assert_uninitialized_load_rejected(seen) assert obj.data() == 42 @@ -498,13 +457,7 @@ def test_reentrant_load_during_old_style_setstate(): callback before placement-new must still see the instance as unconstructed.""" obj = m.OldStyleInit.__new__(m.OldStyleInit) seen = {} - - class Reenter: - def __index__(self): - _record_uninitialized_load(seen, m.accept_old_style_init, obj) - return 43 - - obj.__setstate__(Reenter()) + obj.__setstate__(_LoadOnIndex(seen, m.accept_old_style_init, obj, 43)) _assert_uninitialized_load_rejected(seen) assert obj.data() == 43 @@ -540,13 +493,7 @@ class PythonMI(m.OldStyleInit, m.NewNoInit): obj = PythonMI.__new__(PythonMI) seen = {} - - class Reenter: - def __index__(self): - _record_uninitialized_load(seen, m.accept_new_no_init, obj) - return 45 - - m.OldStyleInit.__init__(obj, Reenter()) + m.OldStyleInit.__init__(obj, _LoadOnIndex(seen, m.accept_new_no_init, obj, 45)) _assert_uninitialized_load_rejected(seen) assert obj.data() == 45 From 7a7e9f3473fd398a2e84643712fef4bb1e947b0b Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 3 Sep 2026 10:04:48 -0700 Subject: [PATCH 10/19] test: reject later self alias during old-style init --- tests/test_class.cpp | 8 ++++++++ tests/test_class.py | 19 +++++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/tests/test_class.cpp b/tests/test_class.cpp index b0a174238d..6793df22d5 100644 --- a/tests/test_class.cpp +++ b/tests/test_class.cpp @@ -694,6 +694,14 @@ TEST_SUBMODULE(class_, m) { int x = state.cast(); new (&self) OldStyleInit(x); }); + old_style_init.def( + "__init__", [](const py::object &, const OldStyleInit &, py::list entered) { + // Reaching this callback means that the later argument was exposed as a C++ + // reference before an OldStyleInit object's lifetime began. Do not inspect that + // reference: keep the regression test itself free of undefined behavior. + entered.append("entered"); + throw std::runtime_error("later-alias constructor callback entered"); + }); }); old_style_init.def("data", &OldStyleInit::data).def("v_data", &OldStyleInit::v_data); diff --git a/tests/test_class.py b/tests/test_class.py index 5c43686814..1363edf5ae 100644 --- a/tests/test_class.py +++ b/tests/test_class.py @@ -336,6 +336,25 @@ def test_failed_old_style_init_does_not_leave_lazy_storage(): assert obj.v_data() == 42 +def test_old_style_init_does_not_authorize_later_self_alias(): + """A later typed argument that aliases a Python-typed `self` must not claim the old-style + constructor's private storage and reach C++ before the object's lifetime has begun.""" + obj = m.OldStyleInit.__new__(m.OldStyleInit) + entered = [] + + with pytest.raises((ValueError, RuntimeError)) as exc_info: + obj.__init__(obj, entered) + + # This is the decisive assertion: the callback's typed argument would refer to raw storage. + assert entered == [] + assert isinstance(exc_info.value, ValueError) + assert "still being constructed" in str(exc_info.value) + + # Rejection must leave the object retryable. + obj.__init__(42) + assert obj.data() == 42 + + def _check_legacy_v12_storage_collision(): """Body of test_old_style_init_legacy_v12_storage_collision, run in a subprocess.""" import pybind11_cross_module_tests as cm From 955cb1935b6a77a4bcfe071d317ae8bba31081b7 Mon Sep 17 00:00:00 2001 From: "Andrew M. James" Date: Fri, 4 Sep 2026 18:16:42 -0500 Subject: [PATCH 11/19] fix: restrict old-style constructor self permission to self's own load phase The one-shot `self` permission granted by `loader_life_support` matched only the frame and the value slot, not the *phase* of the load. The slot is identified by the instance, so any later argument that aliases the same, still-unconstructed `self` matched too, consumed the reservation, and reached C++ over raw storage. Track the frame's phase instead: authorize the load of positional argument 0 (the typed-`self` variant) and casts performed from within the C++ callable (the legacy `py::object`-self variant), and deny every load during conversion of positional arguments >= 1. `argument_loader` reports the argument index, and the dispatcher flips the frame to the callable phase once loading is done. The frame pointer is resolved once in the dispatcher, where `is_constructor` is known, so no non-constructor call pays a thread-local lookup. This restores the restriction that 89a5f72e dropped, re-enabling the regression test added in 7a7e9f34, and adds three more tests. Both new negative tests assert the decisive observable rather than only that the callback was entered, and both use types chosen so that a build where the guard has regressed reports an assertion failure instead of crashing during teardown: - A later argument typed as a *base* of the class under construction. It shares the value slot, so it matched, and the reservation was then sized from the base's `type_info`: 16 bytes for a 144-byte derived object. Because the claim is one-shot it also denied `self` its own storage, so the callback could not placement-new at all; the value was nevertheless committed, and destroying it ran a virtual destructor over never-constructed memory. The test asserts that no reservation was made at the base's size. - The still-unconstructed `self` reached through a container argument. Here `stl.h`'s element caster copy-constructs, so the read of uninitialized memory happens inside pybind11 and no binding author can guard against it. The test counts copy constructions whose source was raw storage and asserts zero; the instrumented copy constructor does not read its source, so the test itself performs no uninitialized read. - A positive test pinning the case that must keep working: a later argument that is a different, already-constructed instance of the same class. Assisted-by: ClaudeCode:claude-opus-5 --- include/pybind11/cast.h | 29 +++++-- include/pybind11/detail/type_caster_base.h | 36 ++++++++ include/pybind11/pybind11.h | 12 ++- tests/test_class.cpp | 96 ++++++++++++++++++++++ tests/test_class.py | 58 +++++++++++++ 5 files changed, 225 insertions(+), 6 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 1d857a0ed5..ef02d80844 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -2160,7 +2160,12 @@ class argument_loader { static constexpr auto arg_names = ::pybind11::detail::concat(type_descr(make_caster::name)...); - bool load_args(function_call &call) { return load_impl_sequence(call, indices{}); } + /// `old_style_init_frame` is non-null only for an old-style constructor candidate, whose + /// `self` (positional argument 0) may reach not-yet-constructed storage. Every other call + /// passes nullptr and pays nothing for the per-argument bookkeeping. + bool load_args(function_call &call, loader_life_support *old_style_init_frame = nullptr) { + return load_impl_sequence(call, indices{}, old_style_init_frame); + } template // NOLINTNEXTLINE(readability-const-return-type) @@ -2177,21 +2182,35 @@ class argument_loader { } private: - static bool load_impl_sequence(function_call &, index_sequence<>) { return true; } + static bool load_impl_sequence(function_call &, index_sequence<>, loader_life_support *) { + return true; + } + + // Loads one positional argument, telling an old-style constructor frame (if any) which + // argument is being loaded: only argument 0 is the constructor's `self`. + template + bool load_one(loader_life_support *old_style_init_frame, function_call &call) { + if (old_style_init_frame != nullptr) { + old_style_init_frame->begin_argument_load(I); + } + return std::get(argcasters).load(call.args[I], call.args_convert[I]); + } template - bool load_impl_sequence(function_call &call, index_sequence) { + bool load_impl_sequence(function_call &call, + index_sequence, + loader_life_support *old_style_init_frame) { PYBIND11_WARNING_PUSH #if !defined(__clang__) && defined(__GNUC__) && __GNUC__ >= 13 // Work around a GCC -Warray-bounds false positive in argument_vector usage. PYBIND11_WARNING_DISABLE_GCC("-Warray-bounds") #endif #ifdef __cpp_fold_expressions - if ((... || !std::get(argcasters).load(call.args[Is], call.args_convert[Is]))) { + if ((... || !load_one(old_style_init_frame, call))) { return false; } #else - for (bool r : {std::get(argcasters).load(call.args[Is], call.args_convert[Is])...}) { + for (bool r : {load_one(old_style_init_frame, call)...}) { if (!r) { return false; } diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index 8f034f4d88..befeeb82e5 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -81,6 +81,24 @@ class loader_life_support { void *old_style_init_storage = nullptr; bool old_style_init_self_claimed = false; + // Tracks what the frame is currently doing, so that only the old-style constructor's own + // `self` may reach still-unconstructed storage. The `self` of an old-style constructor is + // always the first positional argument; a later argument that merely aliases the same, + // still-unconstructed Python object must never be exposed as a C++ reference. + enum class load_phase : std::uint8_t { + before_arguments, // No argument load in progress yet. + self_argument, // Loading positional argument 0, i.e. `self`. + later_argument, // Loading positional argument 1 or later. + callable // All arguments loaded; the C++ callable is running. + }; + load_phase phase = load_phase::before_arguments; + + bool old_style_init_self_load_authorized() const { + // Either the one `self` argument load, or a cast performed by the C++ callable itself + // (the legacy `py::object self` pattern, which casts inside the callback body). + return phase == load_phase::self_argument || phase == load_phase::callable; + } + static bool is_same_value_and_holder(const value_and_holder &lhs, const value_and_holder &rhs) { return lhs.inst == rhs.inst && lhs.vh == rhs.vh; @@ -167,6 +185,23 @@ class loader_life_support { } } + /// Returns the current frame only if it is an old-style constructor frame, i.e. only if + /// argument-load bookkeeping is needed at all. Returns nullptr for every other call. + static loader_life_support *current_old_style_init_frame() { + auto *frame = tls_current_frame(); + return (frame != nullptr && frame->old_style_init_self != nullptr) ? frame : nullptr; + } + + /// Called by `argument_loader` before loading positional argument `index`. + // NOLINTNEXTLINE(readability-make-member-function-const) + void begin_argument_load(std::size_t index) { + phase = (index == 0) ? load_phase::self_argument : load_phase::later_argument; + } + + /// Called once all arguments loaded successfully, before the C++ callable is invoked. + // NOLINTNEXTLINE(readability-make-member-function-const) + void finish_argument_loading() { phase = load_phase::callable; } + /// Claims and allocates the private storage for the one old-style constructor `self` load of /// the current frame; returns nullptr for every other load. `self` is loaded first, or cast /// inside a legacy `py::object` callback after all arguments were loaded. Nested bound calls @@ -176,6 +211,7 @@ class loader_life_support { auto *frame = tls_current_frame(); if (frame == nullptr || frame->old_style_init_self == nullptr || frame->old_style_init_self_claimed + || !frame->old_style_init_self_load_authorized() || !is_same_value_and_holder(v_h, *frame->old_style_init_self) || v_h.value_ptr() != nullptr) { return nullptr; diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 1edd8fa7bd..0f82b37c21 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -501,9 +501,19 @@ class cpp_function : public function { = make_caster::value, void_type, Return>>; ArgsConverter args_converter; - if (!args_converter.load_args(call)) { + // Only an old-style constructor candidate has a frame that can hand out storage for a + // not-yet-constructed value; the loader needs to know which argument is `self`. + auto *old_style_init_frame = call.func.is_constructor + ? loader_life_support::current_old_style_init_frame() + : nullptr; + if (!args_converter.load_args(call, old_style_init_frame)) { return PYBIND11_TRY_NEXT_OVERLOAD; } + if (old_style_init_frame != nullptr) { + // Argument loading is over: from here on, only the C++ callable itself may perform + // the one authorized `self` cast of a legacy `py::object`-self old-style constructor. + old_style_init_frame->finish_argument_loading(); + } /* Override policy for rvalues -- usually to enforce rvp::move on an rvalue */ return_value_policy policy diff --git a/tests/test_class.cpp b/tests/test_class.cpp index 6793df22d5..9827504af7 100644 --- a/tests/test_class.cpp +++ b/tests/test_class.cpp @@ -143,6 +143,51 @@ struct MixedStyleInit { int data() const { return m_data; } }; +// test_old_style_init_does_not_authorize_base_typed_later_alias +// The value slot of a single-inheritance instance is shared by base and derived, so a later +// argument typed as a base of the class under construction matches the same `value_and_holder`. +// Reserving storage for it sizes the allocation from the *base*, which is smaller. +struct AliasStealBase { + std::int32_t marker = 0; + virtual ~AliasStealBase() = default; + static std::size_t &reservations() { + static std::size_t n = 0; + return n; + } + static void *operator new(std::size_t n) { + if (n == sizeof(AliasStealBase)) { + ++reservations(); + } + return ::operator new(n); + } + static void operator delete(void *p) { ::operator delete(p); } +}; + +struct AliasStealDerived : AliasStealBase { + std::int64_t payload[16]; + explicit AliasStealDerived(int x) : payload{} { marker = x; } + int data() const { return marker; } +}; + +// test_old_style_init_does_not_authorize_self_alias_inside_container +// Deliberately trivially destructible: on a build where the guard has regressed the element +// caster copy-constructs from raw storage and the never-constructed value is then committed, so +// the test must report an assertion failure rather than crash during teardown. +struct ContainerAliasItem { + int value; + explicit ContainerAliasItem(int v) : value(v) {} + // Does not read `other`: the regression test must not itself perform the uninitialized + // read. That this runs at all proves the copy constructor was invoked with `other` bound to + // storage whose lifetime had not begun. + ContainerAliasItem(const ContainerAliasItem &) : value(-1) { ++copies_from_source(); } + ContainerAliasItem &operator=(const ContainerAliasItem &) = delete; + static std::size_t &copies_from_source() { + static std::size_t n = 0; + return n; + } + int data() const { return value; } +}; + TEST_SUBMODULE(class_, m) { m.def("obj_class_name", [](py::handle obj) { return py::detail::obj_class_name(obj.ptr()); }); @@ -676,6 +721,28 @@ TEST_SUBMODULE(class_, m) { return NewNoInit(t[0].cast()); })); + py::class_(m, "AliasStealBase"); + py::class_ alias_steal(m, "AliasStealDerived"); + ignoreOldStyleInitWarnings([&alias_steal]() { + alias_steal + .def("__init__", + [](AliasStealDerived &self, int x) { + ::new (static_cast(&self)) AliasStealDerived(x); + }) + .def("__init__", [](const py::object &, const AliasStealBase &, py::list entered) { + // Reaching this callback means a base-typed later argument was exposed as a + // C++ reference over storage sized for the base, not the derived class. + // Do not inspect that reference: keep the test itself free of UB. + entered.append("entered"); + throw std::runtime_error("base-typed later-alias callback entered"); + }); + }); + alias_steal.def("data", &AliasStealDerived::data); + m.def("alias_steal_sizes", + []() { return py::make_tuple(sizeof(AliasStealBase), sizeof(AliasStealDerived)); }); + m.def("alias_steal_reservations", []() { return AliasStealBase::reservations(); }); + m.def("alias_steal_reset", []() { AliasStealBase::reservations() = 0; }); + py::class_ old_style_init(m, "OldStyleInit"); ignoreOldStyleInitWarnings([&old_style_init]() { old_style_init @@ -694,6 +761,11 @@ TEST_SUBMODULE(class_, m) { int x = state.cast(); new (&self) OldStyleInit(x); }); + // A later argument that is a DIFFERENT, fully constructed instance of the same class + // must keep working: the guard only rejects loads of the still-unconstructed `self`. + old_style_init.def("__init__", [](OldStyleInit &self, const OldStyleInit &other) { + new (&self) OldStyleInit(other.data() * 10); + }); old_style_init.def( "__init__", [](const py::object &, const OldStyleInit &, py::list entered) { // Reaching this callback means that the later argument was exposed as a C++ @@ -705,6 +777,30 @@ TEST_SUBMODULE(class_, m) { }); old_style_init.def("data", &OldStyleInit::data).def("v_data", &OldStyleInit::v_data); + py::class_ container_alias(m, "ContainerAliasItem"); + ignoreOldStyleInitWarnings([&container_alias]() { + container_alias + .def("__init__", + [](ContainerAliasItem &self, int v) { + ::new (static_cast(&self)) ContainerAliasItem(v); + }) + .def("__init__", + [](const py::object &, + const std::vector &loaded, + py::list entered) { + // Reaching this callback means the element caster copy-constructed from + // storage whose lifetime had not begun. Unlike a bare reference argument, + // the binding author cannot avoid that read: it happens inside the + // container caster itself. + entered.append("entered"); + entered.append(py::int_(static_cast(loaded.size()))); + throw std::runtime_error("container-alias constructor callback entered"); + }); + }); + container_alias.def("data", &ContainerAliasItem::data); + m.def("container_alias_copies", []() { return ContainerAliasItem::copies_from_source(); }); + m.def("container_alias_reset", []() { ContainerAliasItem::copies_from_source() = 0; }); + py::class_ old_style_init_collision(m, "OldStyleInitCollision"); ignoreOldStyleInitWarnings([&old_style_init_collision]() { old_style_init_collision.def("__init__", [](OldStyleInitCollision &self, int x) { diff --git a/tests/test_class.py b/tests/test_class.py index 1363edf5ae..03073e38ae 100644 --- a/tests/test_class.py +++ b/tests/test_class.py @@ -336,6 +336,14 @@ def test_failed_old_style_init_does_not_leave_lazy_storage(): assert obj.v_data() == 42 +def test_old_style_init_accepts_later_distinct_instance_of_same_class(): + """A later argument that is a different, already-constructed instance must still load.""" + src = m.OldStyleInit(7) + dst = m.OldStyleInit.__new__(m.OldStyleInit) + dst.__init__(src) + assert dst.data() == 70 + + def test_old_style_init_does_not_authorize_later_self_alias(): """A later typed argument that aliases a Python-typed `self` must not claim the old-style constructor's private storage and reach C++ before the object's lifetime has begun.""" @@ -355,6 +363,56 @@ def test_old_style_init_does_not_authorize_later_self_alias(): assert obj.data() == 42 +def test_old_style_init_does_not_authorize_base_typed_later_alias(): + """A later argument typed as a *base* of the class under construction shares the same value + slot, so it must not be able to claim the old-style constructor's storage: the reservation + would be sized from the base, and the constructor's own placement-new would overflow it.""" + base_size, derived_size = m.alias_steal_sizes() + assert base_size < derived_size # an undersized reservation is actually observable + + m.alias_steal_reset() + obj = m.AliasStealDerived.__new__(m.AliasStealDerived) + entered = [] + + with pytest.raises((ValueError, RuntimeError)) as exc_info: + obj.__init__(obj, entered) + + assert entered == [] + assert isinstance(exc_info.value, ValueError) + assert "still being constructed" in str(exc_info.value) + # No storage was reserved at the base's (too small) size. + assert m.alias_steal_reservations() == 0 + + # Rejection must leave the object retryable. + obj.__init__(42) + assert obj.data() == 42 + + +def test_old_style_init_does_not_authorize_self_alias_inside_container(): + """The still-unconstructed `self` reached through a container argument must be rejected too. + + This is stricter than a bare reference argument: `stl.h`'s element caster copy-constructs the + value, so the read of uninitialized storage happens inside pybind11 rather than in the + callback, and no binding author can guard against it. + """ + m.container_alias_reset() + obj = m.ContainerAliasItem.__new__(m.ContainerAliasItem) + entered = [] + + with pytest.raises((ValueError, RuntimeError)) as exc_info: + obj.__init__([obj], entered) + + assert entered == [] + # The decisive assertion: no copy constructor ran with its source over raw storage. + assert m.container_alias_copies() == 0 + assert isinstance(exc_info.value, ValueError) + assert "still being constructed" in str(exc_info.value) + + # Rejection must leave the object retryable. + obj.__init__(42) + assert obj.data() == 42 + + def _check_legacy_v12_storage_collision(): """Body of test_old_style_init_legacy_v12_storage_collision, run in a subprocess.""" import pybind11_cross_module_tests as cm From 607d3c67d4b4bf10f579f62c4112d4b3858e490c Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Tue, 8 Sep 2026 20:59:50 +0000 Subject: [PATCH 12/19] style: pre-commit fixes --- include/pybind11/detail/type_caster_base.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index befeeb82e5..71abc35bef 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -210,8 +210,7 @@ class loader_life_support { static void *try_reserve_old_style_init_storage(value_and_holder &v_h, const type_info *type) { auto *frame = tls_current_frame(); if (frame == nullptr || frame->old_style_init_self == nullptr - || frame->old_style_init_self_claimed - || !frame->old_style_init_self_load_authorized() + || frame->old_style_init_self_claimed || !frame->old_style_init_self_load_authorized() || !is_same_value_and_holder(v_h, *frame->old_style_init_self) || v_h.value_ptr() != nullptr) { return nullptr; From ad4544b450cbbf4c4330387db92a3e7d511a2d04 Mon Sep 17 00:00:00 2001 From: "Andrew M. James" Date: Thu, 10 Sep 2026 18:54:49 -0500 Subject: [PATCH 13/19] style: clang-tidy fixes The Clang-Tidy job failed on two `modernize-use-default-member-init` diagnostics in tests/test_class.cpp, both introduced by this branch: tests/test_class.cpp:167:18: error: use default member initializer for 'payload' [modernize-use-default-member-init,-warnings-as-errors] tests/test_class.cpp:177:9: error: use default member initializer for 'value' [modernize-use-default-member-init,-warnings-as-errors] Applied exactly the replacements clang-tidy emitted: - `AliasStealDerived::payload` gains a `{}` default member initializer and drops `payload{}` from the constructor initializer list. Both value-initialize the array. - `ContainerAliasItem::value` gains a `{-1}` default member initializer and the copy constructor drops `value(-1)`. The converting constructor keeps `value(v)`, which overrides the default, so both constructors still produce the values the container-alias test asserts on. No behavior change; clang-tidy is not installed locally, so the fix-its were transcribed from the CI diagnostic rather than auto-applied, and the translation unit was compiled clean at -std=c++17 with the CI warning set. Assisted-by: ClaudeCode:claude-opus-5 --- tests/test_class.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/test_class.cpp b/tests/test_class.cpp index 9827504af7..2112cceb71 100644 --- a/tests/test_class.cpp +++ b/tests/test_class.cpp @@ -164,8 +164,8 @@ struct AliasStealBase { }; struct AliasStealDerived : AliasStealBase { - std::int64_t payload[16]; - explicit AliasStealDerived(int x) : payload{} { marker = x; } + std::int64_t payload[16]{}; + explicit AliasStealDerived(int x) { marker = x; } int data() const { return marker; } }; @@ -174,12 +174,12 @@ struct AliasStealDerived : AliasStealBase { // caster copy-constructs from raw storage and the never-constructed value is then committed, so // the test must report an assertion failure rather than crash during teardown. struct ContainerAliasItem { - int value; + int value{-1}; explicit ContainerAliasItem(int v) : value(v) {} // Does not read `other`: the regression test must not itself perform the uninitialized // read. That this runs at all proves the copy constructor was invoked with `other` bound to // storage whose lifetime had not begun. - ContainerAliasItem(const ContainerAliasItem &) : value(-1) { ++copies_from_source(); } + ContainerAliasItem(const ContainerAliasItem &) { ++copies_from_source(); } ContainerAliasItem &operator=(const ContainerAliasItem &) = delete; static std::size_t &copies_from_source() { static std::size_t n = 0; From f061048d9c07abe758463455667ea419c68308f8 Mon Sep 17 00:00:00 2001 From: "Andrew M. James" Date: Thu, 10 Sep 2026 18:55:46 -0500 Subject: [PATCH 14/19] fix: GraalPY exceptions Both GraalPy jobs failed on the same assertion in the legacy-v12 collision test: tests/test_class.py:469: assert stats() == (3, 3, constructed + 1, constructed + 1) That is the final check, reached after `del obj` and two `gc.collect()` calls. GraalPy is not refcounted and does not guarantee finalization from `gc.collect()`, so the destruction counters lag and the assertion fails while every preceding assertion in the loop passes. Gate only that assertion behind `if not env.GRAALPY:`. This follows the existing convention in the suite, where GC-timing-dependent checks are exempted on GraalPy with the same "Cannot reliably trigger GC" reason (test_call_policies.py, test_callbacks.py, test_class_sh_trampoline_shared_ptr_cpp_arg.py, and others). Nothing this branch introduces stops being tested on GraalPy. The gated line only observes ordinary teardown of a normal, fully constructed retry object. The rollback properties the test exists for are pinned by the assertions above it, which still run everywhere: after rollback both collision allocations are freed with no spurious destruction, and after the retry exactly one allocation is live with the private value's destructor having run neither early nor twice. Assisted-by: ClaudeCode:claude-opus-5 --- tests/test_class.py | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/tests/test_class.py b/tests/test_class.py index 03073e38ae..caf5ae4b9f 100644 --- a/tests/test_class.py +++ b/tests/test_class.py @@ -464,9 +464,14 @@ def stats(): assert obj.data() == 43 assert stats() == (3, 2, constructed + 1, constructed) del obj - gc.collect() - gc.collect() - assert stats() == (3, 3, constructed + 1, constructed + 1) + if not env.GRAALPY: # Cannot reliably trigger GC. + # Only ordinary teardown of the live retry object is checked here. The rollback + # properties this test exists for are pinned by the assertions above, which run + # everywhere: no leaked collision storage, and no early or double destruction of + # the private value. + gc.collect() + gc.collect() + assert stats() == (3, 3, constructed + 1, constructed + 1) assert unraisable == [] From bf7f96f347071636db227f953efb259b3f342785 Mon Sep 17 00:00:00 2001 From: "Andrew M. James" Date: Thu, 10 Sep 2026 21:10:41 -0500 Subject: [PATCH 15/19] test: pin the two gaps identified in the load-phase review Adds coverage for the two limitations called out in the review of the load-phase restriction. Both tests pass, pinning today's behavior; both fail against `master`'s headers, which is what makes them meaningful. test_old_style_init_value_error_hides_later_overload Two old-style candidates take the same two Python arguments. The first one's argument 1 is the `self` alias that the construction guard rejects; the second matches the same call and constructs. The guard reports rejection with `value_error`, and only `reference_cast_error` becomes PYBIND11_TRY_NEXT_OVERLOAD, so the throw escapes the overload loop and the second candidate is never attempted. Verified counterfactual, same test files built against master's headers: master reaches the second candidate and constructs (`entered == ["second candidate entered"]`); here the call raises ValueError with `entered == []`. Note this is a new trigger for pre-existing behavior rather than a new behavior: master's casters already throw `value_error` from load paths with the same non-fallthrough consequence. test_old_style_init_callable_phase_grant_is_not_self_specific While the callable runs, the one-shot grant is keyed on the value slot, not on the `self` handle, so a cast of `stash[0]` claims the reservation and the genuine `self` cast then fails. Narrowing the grant to "a cast of the `self` object" would not close this: the claiming cast targets the same Python object as `self`, so the two are indistinguishable at cast time. Verified counterfactual: on master both casts succeed (`["stash cast claimed the reservation", "self cast succeeded"]`) because every load lazily allocates. The one-shot reservation is therefore a narrowing of master's behavior, and this gap is the residue rather than a regression. Neither callback inspects the reference it obtains over storage whose lifetime has not begun, so the tests themselves stay free of undefined behavior. Both verify the object is still retryable afterwards. Assisted-by: ClaudeCode:claude-opus-5 --- tests/test_class.cpp | 69 ++++++++++++++++++++++++++++++++++++++++++++ tests/test_class.py | 63 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 132 insertions(+) diff --git a/tests/test_class.cpp b/tests/test_class.cpp index 2112cceb71..c752828006 100644 --- a/tests/test_class.cpp +++ b/tests/test_class.cpp @@ -169,6 +169,23 @@ struct AliasStealDerived : AliasStealBase { int data() const { return marker; } }; +// test_old_style_init_value_error_hides_later_overload +// Two old-style candidates that both match a two-argument call. The first one's *later* +// argument is the alias that the construction guard rejects; the second would construct +// normally. Documents which of the two the dispatcher reaches today. +struct OverloadFallthrough { + int m_data; + explicit OverloadFallthrough(int data) : m_data(data) {} + int data() const { return m_data; } +}; + +// test_old_style_init_callable_phase_grant_is_not_self_specific +struct CallablePhaseGrant { + int m_data; + explicit CallablePhaseGrant(int data) : m_data(data) {} + int data() const { return m_data; } +}; + // test_old_style_init_does_not_authorize_self_alias_inside_container // Deliberately trivially destructible: on a build where the guard has regressed the element // caster copy-constructs from raw storage and the never-constructed value is then committed, so @@ -777,6 +794,58 @@ TEST_SUBMODULE(class_, m) { }); old_style_init.def("data", &OldStyleInit::data).def("v_data", &OldStyleInit::v_data); + py::class_ overload_fallthrough(m, "OverloadFallthrough"); + ignoreOldStyleInitWarnings([&overload_fallthrough]() { + // First candidate. Its `self` is Python-typed, so argument 0 loads harmlessly; the + // alias that the construction guard rejects is argument 1, and the argument that + // would have rejected this candidate on its own (a py::int_ given a list) comes after + // it. Before the guard existed, the alias load succeeded, the py::int_ conversion then + // failed, and overload resolution moved on to the next candidate. + // Both candidates must take the same number of Python arguments, or the first is + // skipped on arity alone and the fall-through says nothing about the guard. + overload_fallthrough.def( + "__init__", [](const py::object &, const OverloadFallthrough &, py::int_) { + // Reaching this callback would mean the alias was exposed as a C++ reference + // before the object's lifetime began. Do not inspect it; the distinctive + // exception message is how the test detects that it ran. + throw std::runtime_error("first candidate entered"); + }); + // Second candidate. Matches the same call and constructs normally. + overload_fallthrough.def( + "__init__", [](OverloadFallthrough &self, const py::object &, py::list entered) { + entered.append("second candidate entered"); + ::new (static_cast(&self)) OverloadFallthrough(99); + }); + }); + overload_fallthrough.def("data", &OverloadFallthrough::data); + + py::class_ callable_phase_grant(m, "CallablePhaseGrant"); + ignoreOldStyleInitWarnings([&callable_phase_grant]() { + callable_phase_grant + .def("__init__", + [](CallablePhaseGrant &self, int v) { + ::new (static_cast(&self)) CallablePhaseGrant(v); + }) + .def("__init__", [](const py::object &self, py::list stash, py::list entered) { + // The callable-phase grant is keyed on the value slot, not on the `self` + // handle. `stash[0]` is the very same Python object as `self`, so this cast is + // indistinguishable from the sanctioned one and consumes the one-shot + // reservation. Do not inspect the reference it returns: it denotes storage + // whose lifetime has not begun. + try { + stash[0].cast(); + entered.append("stash cast claimed the reservation"); + } catch (const std::exception &e) { + entered.append(std::string("stash cast rejected: ") + e.what()); + } + // The genuine `self` cast now finds the permission already spent. + auto &self_ref = self.cast(); + ::new (static_cast(&self_ref)) CallablePhaseGrant(7); + entered.append("self cast succeeded"); + }); + }); + callable_phase_grant.def("data", &CallablePhaseGrant::data); + py::class_ container_alias(m, "ContainerAliasItem"); ignoreOldStyleInitWarnings([&container_alias]() { container_alias diff --git a/tests/test_class.py b/tests/test_class.py index caf5ae4b9f..e90afb5269 100644 --- a/tests/test_class.py +++ b/tests/test_class.py @@ -388,6 +388,69 @@ def test_old_style_init_does_not_authorize_base_typed_later_alias(): assert obj.data() == 42 +def test_old_style_init_value_error_hides_later_overload(): + """Documents a behavior change: rejecting an alias aborts overload resolution. + + The construction guard reports rejection by throwing `value_error`. Only + `reference_cast_error` is translated into `PYBIND11_TRY_NEXT_OVERLOAD`, so the throw + escapes the whole overload loop and a later candidate that would have matched is never + reached. Before the guard existed, the first candidate's alias argument loaded + successfully, its *next* argument then failed to convert, and resolution moved on to the + second candidate, which constructed the object. + + This is not a behavior introduced by the construction guard as such: casters on `master` + already throw `value_error` from load paths with the same non-fallthrough consequence. + The guard adds a new trigger for it. Pinning the current outcome here so that a + deliberate decision to make the guard fall through instead shows up as a test change. + """ + obj = m.OverloadFallthrough.__new__(m.OverloadFallthrough) + entered = [] + + # Both candidates take two Python arguments, so the first is not skipped on arity: it is + # reached, and its argument 1 is the alias the guard rejects. + with pytest.raises(ValueError, match="still being constructed"): + obj.__init__(obj, entered) + + # Neither candidate ran: the first was rejected by the guard before its callback (which + # would have raised RuntimeError("first candidate entered")), and the second - which + # matches this call and would have constructed the object - was never attempted. + assert entered == [] + + # The rejection still leaves the object retryable through the second candidate. + obj.__init__(None, entered) + assert entered == ["second candidate entered"] + assert obj.data() == 99 + + +def test_old_style_init_callable_phase_grant_is_not_self_specific(): + """Documents a known limitation: inside the callable, the one-shot grant is not tied to + the `self` handle. + + While the C++ callable runs, any load of the slot under construction may claim the + reservation, not only a cast of `self`. Narrowing the grant to "a cast of the `self` + object" would not close this: the claiming cast below targets `stash[0]`, which *is* the + same Python object as `self`, so the two are indistinguishable at cast time. Supporting + the legacy `py::object`-self pattern requires the permission to stay live for the whole + callable phase, and the callable body is user code. + + The consequence is bounded. The grant is one-shot, so the genuine `self` cast then fails + and the constructor raises; nothing is published, and the object stays retryable. The + reference only reaches raw storage because the callback explicitly asked to cast it. + """ + obj = m.CallablePhaseGrant.__new__(m.CallablePhaseGrant) + entered = [] + + with pytest.raises(ValueError, match="still being constructed"): + obj.__init__([obj], entered) + + # The non-`self` cast consumed the reservation; the sanctioned one then found it spent. + assert entered == ["stash cast claimed the reservation"] + + # No storage escaped and no state is stuck: the object still constructs normally. + obj.__init__(42) + assert obj.data() == 42 + + def test_old_style_init_does_not_authorize_self_alias_inside_container(): """The still-unconstructed `self` reached through a container argument must be rejected too. From d1fbf07279f0f2f84e08968502de447cc8f63ff8 Mon Sep 17 00:00:00 2001 From: "Andrew M. James" Date: Thu, 10 Sep 2026 21:19:22 -0500 Subject: [PATCH 16/19] perf: only test the old-style frame pointer where the phase can change Addresses the review suggestion to stop paying the null check once per argument. The literal form suggested, `I == 0 &&`, is not safe: `begin_argument_load` is what moves the frame from `self_argument` to `later_argument`, so skipping it for arguments 1 and up leaves the phase at `self_argument` for the whole argument list. That re-opens exactly the hole 955cb193 closed. It regresses four tests: test_old_style_init_does_not_authorize_later_self_alias test_old_style_init_does_not_authorize_base_typed_later_alias test_old_style_init_does_not_authorize_self_alias_inside_container test_old_style_init_value_error_hides_later_overload Gate on `I < 2` instead. The phase only changes at argument 0 and argument 1; from argument 2 on it is already `later_argument`, so those arguments need no call and no test. Two checks per call rather than one, but it is the minimum that preserves the invariant. All 55 tests pass. `I` is a template parameter, so no `if constexpr` is needed and none can be used: pybind11 still supports C++11 and `if constexpr` is a C++17 extension there. A plain `if` on a constant condition already folds completely. clang -O2, the `I == 5` instantiation of a reduction of this function tail-calls straight through with no pointer test emitted, while `I == 0` and `I == 1` keep theirs. MSVC C4127 (constant conditional) is already disabled file-wide at the top of cast.h, and the header compiles clean at -std=c++11/14/17/20 with -Wall -Wextra -Wpedantic -Wconversion -Werror. Assisted-by: ClaudeCode:claude-opus-5 --- include/pybind11/cast.h | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index ef02d80844..65a053637a 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -2190,7 +2190,11 @@ class argument_loader { // argument is being loaded: only argument 0 is the constructor's `self`. template bool load_one(loader_life_support *old_style_init_frame, function_call &call) { - if (old_style_init_frame != nullptr) { + // The phase only *changes* at argument 0 (`self_argument`) and argument 1 + // (`later_argument`); from argument 2 on it is already `later_argument`. `I` is a + // template parameter, so `I < 2` folds at compile time and arguments 2 and beyond + // do not even test the pointer. + if (I < 2 && old_style_init_frame != nullptr) { old_style_init_frame->begin_argument_load(I); } return std::get(argcasters).load(call.args[I], call.args_convert[I]); From ab56b0fdba9501e37ef7702f486543b0964658f4 Mon Sep 17 00:00:00 2001 From: "Andrew M. James" Date: Thu, 10 Sep 2026 21:52:16 -0500 Subject: [PATCH 17/19] docs: describe status_value_constructing with the other status bits The non-simple layout comment enumerated status_holder_constructed and status_instance_registered but not status_value_constructing, which was added alongside them. Addresses the review comment on that block. Also states what the bit means for readers of the value pointer: while it is set, the pointer must not be treated as denoting a live C++ object. That is the invariant the rest of this change depends on, and the status byte is where someone will look for it. Comment-only. Longest line is 94 columns, within the 99-column limit, so clang-format does not reflow it. Assisted-by: ClaudeCode:claude-opus-5 --- include/pybind11/detail/common.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/include/pybind11/detail/common.h b/include/pybind11/detail/common.h index d9ab8b751f..ed2156ecfd 100644 --- a/include/pybind11/detail/common.h +++ b/include/pybind11/detail/common.h @@ -664,8 +664,10 @@ struct instance { * the [bb...] block (but not independently allocated). * * Status bits indicate whether the associated holder is constructed (& - * status_holder_constructed) and whether the value pointer is registered (& - * status_instance_registered) in `registered_instances`. + * status_holder_constructed), whether the value pointer is registered (& + * status_instance_registered) in `registered_instances`, and whether a constructor is + * currently constructing the C++ value in that slot (& status_value_constructing), during + * which the value pointer must not be treated as denoting a live C++ object. */ bool simple_layout : 1; /// For simple layout, tracks whether the holder has been constructed From c10fcc5c29f1390102b3a64ce2b88883ca553129 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Fri, 11 Sep 2026 04:19:37 +0000 Subject: [PATCH 18/19] style: pre-commit fixes --- tests/test_class.cpp | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/tests/test_class.cpp b/tests/test_class.cpp index c752828006..3c2ecbb12a 100644 --- a/tests/test_class.cpp +++ b/tests/test_class.cpp @@ -803,13 +803,14 @@ TEST_SUBMODULE(class_, m) { // failed, and overload resolution moved on to the next candidate. // Both candidates must take the same number of Python arguments, or the first is // skipped on arity alone and the fall-through says nothing about the guard. - overload_fallthrough.def( - "__init__", [](const py::object &, const OverloadFallthrough &, py::int_) { - // Reaching this callback would mean the alias was exposed as a C++ reference - // before the object's lifetime began. Do not inspect it; the distinctive - // exception message is how the test detects that it ran. - throw std::runtime_error("first candidate entered"); - }); + overload_fallthrough.def("__init__", + [](const py::object &, const OverloadFallthrough &, py::int_) { + // Reaching this callback would mean the alias was exposed as + // a C++ reference before the object's lifetime began. Do not + // inspect it; the distinctive exception message is how the + // test detects that it ran. + throw std::runtime_error("first candidate entered"); + }); // Second candidate. Matches the same call and constructs normally. overload_fallthrough.def( "__init__", [](OverloadFallthrough &self, const py::object &, py::list entered) { From 182a9d094d840f5d6fa9b443db9a69affd9ca483 Mon Sep 17 00:00:00 2001 From: "Andrew M. James" Date: Thu, 10 Sep 2026 23:49:28 -0500 Subject: [PATCH 19/19] style: clang-tidy/format --- tests/test_class.cpp | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/test_class.cpp b/tests/test_class.cpp index 3c2ecbb12a..a93b5ed832 100644 --- a/tests/test_class.cpp +++ b/tests/test_class.cpp @@ -803,14 +803,14 @@ TEST_SUBMODULE(class_, m) { // failed, and overload resolution moved on to the next candidate. // Both candidates must take the same number of Python arguments, or the first is // skipped on arity alone and the fall-through says nothing about the guard. - overload_fallthrough.def("__init__", - [](const py::object &, const OverloadFallthrough &, py::int_) { - // Reaching this callback would mean the alias was exposed as - // a C++ reference before the object's lifetime began. Do not - // inspect it; the distinctive exception message is how the - // test detects that it ran. - throw std::runtime_error("first candidate entered"); - }); + overload_fallthrough.def( + "__init__", [](const py::object &, const OverloadFallthrough &, const py::int_ &) { + // Reaching this callback would mean the alias was exposed as + // a C++ reference before the object's lifetime began. Do not + // inspect it; the distinctive exception message is how the + // test detects that it ran. + throw std::runtime_error("first candidate entered"); + }); // Second candidate. Matches the same call and constructs normally. overload_fallthrough.def( "__init__", [](OverloadFallthrough &self, const py::object &, py::list entered) { @@ -827,7 +827,7 @@ TEST_SUBMODULE(class_, m) { [](CallablePhaseGrant &self, int v) { ::new (static_cast(&self)) CallablePhaseGrant(v); }) - .def("__init__", [](const py::object &self, py::list stash, py::list entered) { + .def("__init__", [](const py::object &self, const py::list &stash, py::list entered) { // The callable-phase grant is keyed on the value slot, not on the `self` // handle. `stash[0]` is the very same Python object as `self`, so this cast is // indistinguishable from the sanctioned one and consumes the one-shot