From a5ee76ed0d5cc2052fc9f98508f752770c956b03 Mon Sep 17 00:00:00 2001 From: Henry Schreiner Date: Sat, 14 Nov 2026 20:55:30 -0500 Subject: [PATCH 1/2] fix(stl): determine the sequence length only once in array_caster The element loops of `array_caster` asked the sequence for its length more than once: one time to size the storage, and again for the end of the iteration. A sequence that reports different lengths could cause out-of-bounds accesses (a write for `std::valarray`, a read for `std::array`). The loops now read the length one time and index up to it. Assisted-by: ClaudeCode:claude-fable-5-2 --- include/pybind11/stl.h | 25 ++++++++++++++++--------- tests/test_stl.cpp | 1 + tests/test_stl.py | 36 ++++++++++++++++++++++++++++++++++++ 3 files changed, 53 insertions(+), 9 deletions(-) diff --git a/include/pybind11/stl.h b/include/pybind11/stl.h index 0790074534..8f52fbb7b4 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->resize(size); + for (size_t i = 0; i < size; i++) { + const object item = l[i]; // Keeps the item alive while `conv` is in use. value_conv conv; - if (!conv.load(it, convert)) { + 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) { + temp.reserve(size); + for (size_t i = 0; i < size; i++) { + const object item = l[i]; // Keeps the item alive while `conv` is in use. value_conv conv; - if (!conv.load(it, convert)) { + 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() From d2e6a65b2a93aa076b0b5c02229d9d4692eaedc3 Mon Sep 17 00:00:00 2001 From: Henry Schreiner Date: Sat, 14 Nov 2026 21:18:57 -0500 Subject: [PATCH 2/2] fix(stl): construct each element caster before its item is fetched This keeps the order that the loops had when they iterated over the sequence. Assisted-by: ClaudeCode:claude-fable-5-2 --- include/pybind11/stl.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/pybind11/stl.h b/include/pybind11/stl.h index 8f52fbb7b4..b73d0e95b2 100644 --- a/include/pybind11/stl.h +++ b/include/pybind11/stl.h @@ -416,8 +416,8 @@ struct array_caster { // https://en.cppreference.com/w/cpp/named_req/NumericType 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. - value_conv conv; if (!conv.load(item, convert)) { return false; } @@ -441,8 +441,8 @@ struct array_caster { std::vector temp; 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. - value_conv conv; if (!conv.load(item, convert)) { return false; }