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..d9ab8b751f 100644 --- a/include/pybind11/detail/common.h +++ b/include/pybind11/detail/common.h @@ -676,6 +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; + /// 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) @@ -693,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 161b9884fa..8f034f4d88 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 @@ -38,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 { @@ -61,9 +74,79 @@ 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. + // 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_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; + } + + 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); + } + + PYBIND11_NOINLINE void cleanup_old_style_init_storage() noexcept { + void *const storage = old_style_init_storage; + old_style_init_storage = nullptr; + + auto v_h = *old_style_init_self; + auto *const context = reinterpret_cast(v_h.inst); + bool private_storage_is_published = false; + 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); + } + } + if (!private_storage_is_published) { + try { + deallocate_unconstructed_storage(v_h.type, storage); + } catch (...) { + report_cleanup_error( + "loader_life_support: failed to clean up private constructor storage", + context); + } + } + } + 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 +159,89 @@ class loader_life_support { pybind11_fail("loader_life_support: internal error"); } frame = parent; + if (old_style_init_storage != nullptr) { + cleanup_old_style_init_storage(); + } for (auto *item : keep_alive) { Py_DECREF(item); } } + /// 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 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 + || !is_same_value_and_holder(v_h, *frame->old_style_init_self) + || v_h.value_ptr() != nullptr) { + return nullptr; + } + + frame->old_style_init_self_claimed = true; + 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(); + } + return frame->old_style_init_storage; + } + + /// 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; + 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); + 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,6 +686,7 @@ PYBIND11_NOINLINE void instance::allocate_layout() { = reinterpret_cast(&nonsimple.values_and_holders[flags_at]); } owned = true; + simple_value_constructing = false; } // NOLINTNEXTLINE(readability-make-member-function-const) @@ -534,6 +696,57 @@ 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) { + start(*v_h); + } + } + ~instance_construction_scope() { + if (started_) { + finish(); + } + } + 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; + } + if (v_h_.instance_registered()) { + already_registered_ = true; + return; + } + value_was_null_ = v_h_.value_ptr() == nullptr; + v_h_.set_value_constructing(); + started_ = true; + } + 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) { + deallocate_instance_value(v_h_); + } + v_h_.set_value_constructing(false); + } + + value_and_holder v_h_; + bool started_ = false; + bool already_registered_ = false; + bool value_was_null_ = false; +}; + PYBIND11_NOINLINE bool isinstance_generic(handle obj, const std::type_info &tp) { handle type = detail::get_type_handle(tp, false); if (!type) { @@ -1128,6 +1341,24 @@ 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 + // 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()) { + const auto *type = v_h.type ? v_h.type : typeinfo; + 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 `" + + 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; @@ -1138,22 +1369,12 @@ class type_caster_generic { } } auto *&vptr = v_h.value_ptr(); - // Lazy allocation for unallocated values: if (vptr == nullptr) { - 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 f57514ae28..1edd8fa7bd 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -510,16 +510,16 @@ class cpp_function : public function { = return_value_policy_override::policy(call.func.policy); /* Perform the function call */ - handle result; + 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(); + } if (call.func.is_setter) { - (void) std::move(args_converter).template call(f); - result = none().release(); - } else { - result = cast_out::cast( - std::move(args_converter).template call(f), policy, call.parent); + return none().release(); } - - return result; + return cast_out::cast(std::forward(cpp_result), policy, call.parent); } protected: @@ -993,14 +993,36 @@ 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); + } + + // 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) { // 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()) { + 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; + } } + // 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`; // in the second, we allow conversion (except for arguments with an explicit @@ -1060,8 +1082,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); } @@ -1232,7 +1255,7 @@ class cpp_function : public function { // 6. Call the function. try { - loader_life_support guard{}; + loader_life_support guard{old_style_init_self(func)}; result = func.impl(call); } catch (reference_cast_error &) { result = PYBIND11_TRY_NEXT_OVERLOAD; @@ -1263,7 +1286,7 @@ class cpp_function : public function { // allowed for (auto &call : second_pass) { try { - loader_life_support guard{}; + 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/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..c2fa29bdf8 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,48 @@ 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) { + // 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; + vptr = type->operator_new != nullptr ? type->operator_new(type->type_size) + : ::operator new(type->type_size); + } + 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 e520f29ec5..6793df22d5 100644 --- a/tests/test_class.cpp +++ b/tests/test_class.cpp @@ -77,6 +77,72 @@ 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_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_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; + 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()); }); @@ -597,6 +663,86 @@ 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()); + })); + + 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( + "__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); + + 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]() { + 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 201c7e339e..1363edf5ae 100644 --- a/tests/test_class.py +++ b/tests/test_class.py @@ -1,7 +1,10 @@ from __future__ import annotations import gc +import os +import pickle import sys +import threading from unittest import mock import pytest @@ -251,6 +254,315 @@ 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) + + +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.""" + + class PythonDerived(m.NewNoInit): + pass + + for cls in (m.NewNoInit, PythonDerived): + obj = cls.__new__(cls) + with pytest.raises(ValueError) as exc_info: + 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 + + +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 + + +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 reserved storage for `self`. Without cleanup, a later load can + # mistake that storage for a constructed C++ object. + with pytest.raises(ValueError, match="uninitialized"): + m.accept_old_style_init(obj) + + # A successful retry is still allowed. + obj.__init__(42) + 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 + + unraisable = [] + sys.unraisablehook = lambda args: unraisable.append(str(args.exc_value)) + + class LegacyLoadOnIndex: + """Loads `obj` through the stale caster during argument conversion.""" + + def __init__(self, obj, seen, result): + self.obj = obj + self.seen = seen + self.result = result + + def __index__(self): + self.seen["address"] = cm.legacy_v12_pointer_only_load(self.obj) + if self.result is None: + raise TypeError("stop the constructor") + 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 stats() == (2, 2, constructed, constructed) + + # The same object remains usable. + obj.__init__(43) + 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) + + assert unraisable == [] + + +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 + + sys.path.insert(0, {os.path.dirname(env.__file__)!r}) + + import test_class + + test_class._check_legacy_v12_storage_collision() + """, + 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.""" + obj = m.NewNoInit.__new__(m.NewNoInit) + seen = {} + with pytest.raises(TypeError): + obj.__init__(_LoadOnIndex(seen, m.accept_new_no_init, obj)) + _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 = {} + with pytest.raises(TypeError): + 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) + + # 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 = {} + obj.__init__(_LoadOnIndex(seen, m.accept_mixed_style_init, obj, 42)) + _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 = {} + obj.__setstate__(_LoadOnIndex(seen, m.accept_old_style_init, obj, 43)) + _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 = {} + m.OldStyleInit.__init__(obj, _LoadOnIndex(seen, m.accept_new_no_init, obj, 45)) + _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) + + +@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 + 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( "mock_return_value", [None, (1, 2, 3), m.Pet("Polly", "parrot"), m.Dog("Molly")] )