From 97d5867fe5bc2cb3042df57ceece6f232e014858 Mon Sep 17 00:00:00 2001 From: "Andrew M. James" Date: Fri, 28 Aug 2026 09:21:28 -0500 Subject: [PATCH 1/4] 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 2d9be0b94327c6f3b787def1a96192eee6881aec Mon Sep 17 00:00:00 2001 From: Henry Schreiner Date: Sat, 29 Aug 2026 14:08:37 -0400 Subject: [PATCH 2/4] 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 b90230288b0cd0db933b151b9e703faab4679a95 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sat, 12 Sep 2026 13:06:24 -0700 Subject: [PATCH 3/4] docs: define minimal uninitialized-instance guard scope --- docs/advanced/classes.rst | 10 ++++ include/pybind11/detail/common.h | 9 ++-- include/pybind11/detail/type_caster_base.h | 30 +++++------ include/pybind11/pybind11.h | 17 ++++--- tests/test_class.cpp | 29 +++++++---- tests/test_class.py | 59 +++++++++++++++++----- 6 files changed, 104 insertions(+), 50 deletions(-) diff --git a/docs/advanced/classes.rst b/docs/advanced/classes.rst index c4c32f7c38..0e36e47a4a 100644 --- a/docs/advanced/classes.rst +++ b/docs/advanced/classes.rst @@ -875,6 +875,16 @@ The ``__setstate__`` part of the ``py::pickle()`` definition follows the same rules as the single-argument version of ``py::init()``. The return type can be a value, pointer or holder type. See :ref:`custom_constructors` for details. +Calling ``__new__`` directly creates the Python wrapper without constructing its C++ value. +Passing such an uninitialized wrapper to bound C++ code raises ``ValueError``. Calling its +``__init__`` or a pickle-generated ``__setstate__`` can still finish construction normally. + +Deprecated placement-new ``__init__`` and ``__setstate__`` bindings retain their historical +lazy-allocation behavior for compatibility. The exception covers their complete constructor +overload chain and is not a general construction-safety boundary: reentrant loads while such a +chain is active remain the responsibility of the binding author. Prefer ``py::init()`` factories +and ``py::pickle()``, which return a constructed value, pointer, or holder. + An instance can now be pickled as follows: .. code-block:: python diff --git a/include/pybind11/detail/common.h b/include/pybind11/detail/common.h index 6f4323513c..6b1a2b88a1 100644 --- a/include/pybind11/detail/common.h +++ b/include/pybind11/detail/common.h @@ -676,11 +676,10 @@ 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; + /// If true, this instance is being dispatched through a constructor chain containing a + /// deprecated old-style placement-new `__init__`/`__setstate__`. Such chains retain the + /// historical ability to lazily allocate C++ value storage; see `old_style_init_scope`. + bool old_style_init_active : 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 4657a892ba..d2cbec6b2f 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -525,7 +525,7 @@ PYBIND11_NOINLINE void instance::allocate_layout() { = reinterpret_cast(&nonsimple.values_and_holders[flags_at]); } owned = true; - construction_in_progress = false; + old_style_init_active = false; } // NOLINTNEXTLINE(readability-make-member-function-const) @@ -535,37 +535,37 @@ 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. +/// RAII helper preserving lazy value allocation for a constructor chain containing a deprecated +/// old-style placement-new `__init__`/`__setstate__`. Passing `nullptr` makes this a no-op. The +/// compatibility window covers the whole chain; it does not attempt to distinguish the old-style +/// `self` load from reentrant or later-argument loads. Nesting restores the previous state. /// /// 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 { +class old_style_init_scope { public: - explicit instance_construction_scope(value_and_holder *v_h) : v_h_{v_h} { + explicit old_style_init_scope(value_and_holder *v_h) : v_h_{v_h} { if (v_h_ != nullptr) { - was_in_progress_ = v_h_->inst->construction_in_progress; + was_active_ = v_h_->inst->old_style_init_active; value_was_null_ = v_h_->value_ptr() == nullptr; - v_h_->inst->construction_in_progress = true; + v_h_->inst->old_style_init_active = true; } } - ~instance_construction_scope() { + ~old_style_init_scope() { if (v_h_ != nullptr) { - v_h_->inst->construction_in_progress = was_in_progress_; + v_h_->inst->old_style_init_active = was_active_; if (value_was_null_ && !v_h_->holder_constructed() && v_h_->value_ptr() != nullptr) { v_h_->type->dealloc(*v_h_); // Frees the storage and nulls the value pointer. } } } - instance_construction_scope(const instance_construction_scope &) = delete; - instance_construction_scope &operator=(const instance_construction_scope &) = delete; + old_style_init_scope(const old_style_init_scope &) = delete; + old_style_init_scope &operator=(const old_style_init_scope &) = delete; private: value_and_holder *v_h_; - bool was_in_progress_ = false; + bool was_active_ = false; bool value_was_null_ = false; }; @@ -1182,7 +1182,7 @@ class type_caster_generic { // 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) { + if (!v_h.inst->old_style_init_active) { throw value_error("Missing value for wrapped C++ type `" + clean_type_id(cpptype->name()) + "`: Python instance is uninitialized: the C++ object was " diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 9e4a91b405..0a3cdd505f 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1001,13 +1001,14 @@ class cpp_function : public function { } } - // 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. + // While a constructor chain containing 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) { @@ -1017,7 +1018,7 @@ class cpp_function : public function { } } } - detail::instance_construction_scope construction_scope(lazily_allocatable_v_h); + detail::old_style_init_scope old_style_init_guard(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 15217802d8..5358a00d1f 100644 --- a/tests/test_class.cpp +++ b/tests/test_class.cpp @@ -631,16 +631,25 @@ 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__", [](py::object self, int x) { + auto &typed_self = self.cast(); + new (&typed_self) OldStyleInit(x); + }); + }); + old_style_init.def("data", &OldStyleInit::data).def("v_data", &OldStyleInit::v_data); + // This probe intentionally does not dereference the pointer. It documents the narrow scope of + // this fix without itself reading storage before an OldStyleInit lifetime has begun. + m.def("accept_old_style_init", [](OldStyleInit *value) { return value != nullptr; }); } template diff --git a/tests/test_class.py b/tests/test_class.py index bb606eee65..38f0c2b1fb 100644 --- a/tests/test_class.py +++ b/tests/test_class.py @@ -255,18 +255,22 @@ def __init__(self): 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) + class PythonDerived(m.NewNoInit): + pass - # 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 + for cls in (m.NewNoInit, PythonDerived): + obj = cls.__new__(cls) + 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(): @@ -302,6 +306,37 @@ def test_failed_old_style_init_does_not_leave_lazy_storage(): assert obj.v_data() == 42 +def test_old_style_setstate_remains_supported(): + """Deprecated placement-new `__setstate__` may still obtain storage inside its callback.""" + obj = m.OldStyleInit.__new__(m.OldStyleInit) + obj.__setstate__(43) + assert obj.data() == 43 + + +def test_old_style_init_reentrant_load_is_out_of_scope(): + """The minimal fix retains the historical broad lazy-allocation window while an old-style + constructor chain is active. It does not promise to reject reentrant loads in that window.""" + obj = m.OldStyleInit.__new__(m.OldStyleInit) + seen = {} + + class LoadOnIndex: + def __index__(self): + seen["accepted"] = m.accept_old_style_init(obj) + raise TypeError("stop the constructor") + + with pytest.raises(TypeError): + obj.__init__(LoadOnIndex()) + + assert seen == {"accepted": True} + + # Failure cleanup removes the raw storage, so subsequent ordinary loads are rejected and a + # normal initialization retry remains possible. + with pytest.raises(ValueError, match="uninitialized"): + m.accept_old_style_init(obj) + obj.__init__(44) + assert obj.data() == 44 + + 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.""" @@ -310,8 +345,8 @@ 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. + # Runs during int conversion of a pure new-style constructor. Its chain has no + # compatibility window for lazy allocation. try: seen["data"] = obj.data() except ValueError as exc: From c47ae934813544ac105182159efc5c73e65dfd0f Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sat, 12 Sep 2026 14:31:39 -0700 Subject: [PATCH 4/4] test: avoid unnecessary py::object copy --- tests/test_class.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_class.cpp b/tests/test_class.cpp index 5358a00d1f..7d10ee6511 100644 --- a/tests/test_class.cpp +++ b/tests/test_class.cpp @@ -641,7 +641,7 @@ TEST_SUBMODULE(class_, m) { } new (&self) OldStyleInit(x); }) - .def("__setstate__", [](py::object self, int x) { + .def("__setstate__", [](const py::object &self, int x) { auto &typed_self = self.cast(); new (&typed_self) OldStyleInit(x); });