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
diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index 3cdd3a9..4657a89 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h
@@ -535,29 +535,38 @@ } } -/// 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 dc50d6d..9e4a91b 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h
@@ -1001,13 +1001,23 @@ } } - // 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<detail::instance *>(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 1bdc41c..1521780 100644 --- a/tests/test_class.cpp +++ b/tests/test_class.cpp
@@ -89,6 +89,15 @@ 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 @@ } return NewNoInit(t[0].cast<int>()); })); + + py::class_<OldStyleInit>(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 <int N>
diff --git a/tests/test_class.py b/tests/test_class.py index ad84715..bb606ee 100644 --- a/tests/test_class.py +++ b/tests/test_class.py
@@ -285,6 +285,46 @@ 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")] )