diff --git a/include/pybind11/stl.h b/include/pybind11/stl.h index 0790074534..b73d0e95b2 100644 --- a/include/pybind11/stl.h +++ b/include/pybind11/stl.h @@ -401,22 +401,27 @@ struct array_caster { private: std::unique_ptr value; + // Both `convert_elements` overloads index `l` up to a size that is determined only once, + // rather than iterating over `l`: the accesses stay in bounds even if the length of `l` + // changes underneath (e.g. if another thread resizes a list in a free-threaded build). + template = 0> bool convert_elements(handle seq, bool convert) { auto l = reinterpret_borrow(seq); + const size_t size = l.size(); value.reset(new ArrayType{}); // Using `resize` to preserve the behavior exactly as it was before PR #5305 // For the `resize` to work, `Value` must be default constructible. // For `std::valarray`, this is a requirement: // https://en.cppreference.com/w/cpp/named_req/NumericType - value->resize(l.size()); - size_t ctr = 0; - for (const auto &it : l) { - value_conv conv; - if (!conv.load(it, convert)) { + value->resize(size); + for (size_t i = 0; i < size; i++) { + value_conv conv; // Constructed before the item is fetched. + const object item = l[i]; // Keeps the item alive while `conv` is in use. + if (!conv.load(item, convert)) { return false; } - (*value)[ctr++] = cast_op(std::move(conv)); + (*value)[i] = cast_op(std::move(conv)); } return true; } @@ -424,7 +429,8 @@ struct array_caster { template = 0> bool convert_elements(handle seq, bool convert) { auto l = reinterpret_borrow(seq); - if (l.size() != Size) { + const size_t size = l.size(); + if (size != Size) { return false; } // The `temp` storage is needed to support `Value` types that are not @@ -433,10 +439,11 @@ struct array_caster { // because the compile time overhead for the specializations is deemed // more significant than the runtime overhead for the `temp` storage. std::vector temp; - temp.reserve(l.size()); - for (auto it : l) { - value_conv conv; - if (!conv.load(it, convert)) { + temp.reserve(size); + for (size_t i = 0; i < size; i++) { + value_conv conv; // Constructed before the item is fetched. + const object item = l[i]; // Keeps the item alive while `conv` is in use. + if (!conv.load(item, convert)) { return false; } temp.emplace_back(cast_op(std::move(conv))); diff --git a/tests/test_stl.cpp b/tests/test_stl.cpp index b5d1979d04..9306b20670 100644 --- a/tests/test_stl.cpp +++ b/tests/test_stl.cpp @@ -225,6 +225,7 @@ TEST_SUBMODULE(stl, m) { m.def("load_valarray", [](const std::valarray &v) { return v.size() == 3 && v[0] == 1 && v[1] == 4 && v[2] == 9; }); + m.def("valarray_cast_sequence", [](std::valarray x) { return x; }); // test_map m.def("cast_map", []() { return std::map{{"key", "value"}}; }); diff --git a/tests/test_stl.py b/tests/test_stl.py index 32ee539f50..7c1f76daa6 100644 --- a/tests/test_stl.py +++ b/tests/test_stl.py @@ -585,6 +585,42 @@ def test_array_cast_sequence(): assert m.array_cast_sequence((1, 2, 3)) == [1, 2, 3] +class SequenceWithChangingLen: + """Reports `lens[0]` when first asked for its length, `lens[1]` after that.""" + + def __init__(self, items, lens): + self.items = items + self.lens = list(lens) + self.indices = [] + + def __len__(self): + return self.lens.pop(0) if len(self.lens) > 1 else self.lens[0] + + def __getitem__(self, index): + self.indices.append(index) + return self.items[index] + + +ARRAY_CASTER_LOADS = [m.array_cast_sequence, m.valarray_cast_sequence] + + +@pytest.mark.parametrize("load", ARRAY_CASTER_LOADS) +@pytest.mark.parametrize("len_change", [-1, 1]) +def test_array_caster_len_changes(load, len_change): + """The length is determined only once, which rules out out-of-bounds accesses.""" + seq = SequenceWithChangingLen(list(range(4)), [3, 3 + len_change]) + assert load(seq) == [0, 1, 2] + assert seq.indices == [0, 1, 2] + + +@pytest.mark.parametrize("load", ARRAY_CASTER_LOADS) +def test_array_caster_len_exceeds_items(load): + seq = SequenceWithChangingLen([0, 1], [3]) + with pytest.raises(IndexError): + load(seq) + assert seq.indices == [0, 1, 2] + + def test_issue_1561(): """check fix for issue #1561""" bar = m.Issue1561Outer()