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

Author: amjames

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

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)

include/pybind11/detail/type_caster_base.h

@@ -525,6 +525,7 @@ PYBIND11_NOINLINE void instance::allocate_layout() {
             = reinterpret_cast<std::uint8_t *>(&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);

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<detail::instance *>(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

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_<NewNoInit>(m, "NewNoInit")
+        .def(py::init<int>())
+        .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<int>());
+                        }));
 }
 
 template <int N>

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 (lambda: obj.data(), lambda: obj.v_data(), lambda: 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")]
 )