diff --git a/include/pybind11/detail/argument_vector.h b/include/pybind11/detail/argument_vector.h index 6e2c2ec481..cbd17a7148 100644 --- a/include/pybind11/detail/argument_vector.h +++ b/include/pybind11/detail/argument_vector.h @@ -54,6 +54,25 @@ union inline_array_or_vector { bool is_inline = true; std::uint32_t size = 0; std::array arr; + + inline_array() = default; + inline_array(const inline_array &) = default; + inline_array &operator=(const inline_array &) = default; + ~inline_array() = default; + + // Moving leaves the source empty, to match std::vector and so that + // owning users (e.g. ref_small_vector) do not release twice. + inline_array(inline_array &&rhs) noexcept : size(rhs.size), arr(std::move(rhs.arr)) { + rhs.size = 0; + } + inline_array &operator=(inline_array &&rhs) noexcept { + if (this != &rhs) { + size = rhs.size; + arr = std::move(rhs.arr); + rhs.size = 0; + } + return *this; + } }; struct heap_vector { bool is_inline = false; @@ -80,6 +99,8 @@ union inline_array_or_vector { inline_array_or_vector(const inline_array_or_vector &) = delete; inline_array_or_vector &operator=(const inline_array_or_vector &) = delete; + // Both branches leave rhs empty: inline_array does so explicitly, and the + // std::vector move constructor is guaranteed to. inline_array_or_vector(inline_array_or_vector &&rhs) noexcept { if (rhs.is_inline()) { new (&iarray) inline_array(std::move(rhs.iarray)); @@ -374,9 +395,8 @@ class ref_small_vector { ref_small_vector &operator=(const ref_small_vector &) = delete; // Move is allowed - ref_small_vector(ref_small_vector &&other) noexcept : m_ptrs(std::move(other.m_ptrs)) { - // other.m_ptrs is now empty, so its destructor won't decref anything - } + // small_vector leaves the source empty, so other's destructor decrefs nothing. + ref_small_vector(ref_small_vector &&other) noexcept : m_ptrs(std::move(other.m_ptrs)) {} ref_small_vector &operator=(ref_small_vector &&other) noexcept { if (this != &other) { diff --git a/tests/test_with_catch/test_argument_vector.cpp b/tests/test_with_catch/test_argument_vector.cpp index 9cf302a9b1..3124188294 100644 --- a/tests/test_with_catch/test_argument_vector.cpp +++ b/tests/test_with_catch/test_argument_vector.cpp @@ -92,3 +92,80 @@ TEST_CASE("argument_vector reserve then push_back") { })); } } + +namespace { + +// Extra references so that a buggy double-decref cannot free the object under +// test and turn a refcount mismatch into a crash. +constexpr int kRefPadding = 100; + +using ref_small_vector = py::detail::ref_small_vector<2>; + +void fill_with_borrowed(ref_small_vector &vec, PyObject *obj, std::size_t count) { + for (std::size_t ii = 0; ii < count; ++ii) { + vec.push_back_borrow(obj); + } +} + +void check_move_construct(std::size_t count) { + py::list obj; + for (int ii = 0; ii < kRefPadding; ++ii) { + Py_INCREF(obj.ptr()); + } + const auto initial = obj.ref_count(); + { + ref_small_vector src; + fill_with_borrowed(src, obj.ptr(), count); + REQUIRE(obj.ref_count() == initial + static_cast(count)); + + ref_small_vector dst(std::move(src)); + REQUIRE(dst.size() == count); + // NOLINTNEXTLINE(bugprone-use-after-move,clang-analyzer-cplusplus.Move) + REQUIRE(src.size() == 0); + REQUIRE(obj.ref_count() == initial + static_cast(count)); + } + REQUIRE(obj.ref_count() == initial); + for (int ii = 0; ii < kRefPadding; ++ii) { + Py_DECREF(obj.ptr()); + } +} + +void check_move_assign(std::size_t count) { + py::list obj; + for (int ii = 0; ii < kRefPadding; ++ii) { + Py_INCREF(obj.ptr()); + } + const auto initial = obj.ref_count(); + { + ref_small_vector src; + fill_with_borrowed(src, obj.ptr(), count); + ref_small_vector dst; + fill_with_borrowed(dst, obj.ptr(), 1); + dst = std::move(src); + REQUIRE(dst.size() == count); + // NOLINTNEXTLINE(bugprone-use-after-move,clang-analyzer-cplusplus.Move) + REQUIRE(src.size() == 0); + REQUIRE(obj.ref_count() == initial + static_cast(count)); + } + REQUIRE(obj.ref_count() == initial); + for (int ii = 0; ii < kRefPadding; ++ii) { + Py_DECREF(obj.ptr()); + } +} + +} // namespace + +TEST_CASE("ref_small_vector move construction releases each reference once") { + // 0..2 use the inline array, 3 and 5 use the heap vector. + for (std::size_t count : + {std::size_t(0), std::size_t(1), std::size_t(2), std::size_t(3), std::size_t(5)}) { + check_move_construct(count); + } +} + +TEST_CASE("ref_small_vector move assignment releases each reference once") { + for (std::size_t count : + {std::size_t(0), std::size_t(1), std::size_t(2), std::size_t(3), std::size_t(5)}) { + check_move_assign(count); + } +} diff --git a/tests/test_with_catch/test_interpreter.cpp b/tests/test_with_catch/test_interpreter.cpp index e39f51c274..0eb029a3cc 100644 --- a/tests/test_with_catch/test_interpreter.cpp +++ b/tests/test_with_catch/test_interpreter.cpp @@ -360,13 +360,17 @@ TEST_CASE("Enum module survives restart") { // Added in PR #6015 PYBIND11_CATCH2_SKIP_IF(PY_MAJOR_VERSION == 3 && PY_MINOR_VERSION == 12, "Pre-existing crash in enum cleanup during finalize on Python 3.12"); - auto enum_mod = py::module_::import("enum_module"); - REQUIRE(enum_mod.attr("SomeEnum").attr("value1").attr("name").cast() == "value1"); + { + // Scoped so that no reference to the old interpreter's module survives the restart. + auto enum_mod = py::module_::import("enum_module"); + REQUIRE(enum_mod.attr("SomeEnum").attr("value1").attr("name").cast() + == "value1"); + } py::finalize_interpreter(); py::initialize_interpreter(); - enum_mod = py::module_::import("enum_module"); + auto enum_mod = py::module_::import("enum_module"); REQUIRE(enum_mod.attr("SomeEnum").attr("value2").attr("name").cast() == "value2"); }