From 7fc7e24f7f1328a04e8a5a00b67d1ec8225ee1b0 Mon Sep 17 00:00:00 2001 From: Loic Gouarin Date: Thu, 29 Oct 2026 17:40:49 +0100 Subject: [PATCH] Fix svector use-after-free when the argument aliases an element svector::push_back, insert(pos, value) and assign(n, value) read their argument after calling grow(). grow() frees the old heap buffer, so when the argument refers to an element of the same svector and the call has to reallocate, the read is a heap-use-after-free. On a full heap-backed svector, v.push_back(v[0]), v.insert(v.begin(), v[0]) and v.assign(5, v[0]) store garbage, and AddressSanitizer reports the read. insert() already adjusted the argument pointer when it fell inside the shifted range. That covered the shift but not the reallocation. When the argument is a separate object, GCC 13 and 15 also follow the branch where it lies inside the vector and warn that the adjusted pointer reads past it (-Warray-bounds). The three functions now copy the value before anything can move or free it. push_back only makes the copy when it reallocates. insert() treats pos == end() like any other position, and the pointer adjustment is gone, along with its -Warray-bounds warnings. --- include/xtensor/containers/xstorage.hpp | 43 +++++++++-------- test/test_xstorage.cpp | 63 +++++++++++++++++++++++++ 2 files changed, 87 insertions(+), 19 deletions(-) diff --git a/include/xtensor/containers/xstorage.hpp b/include/xtensor/containers/xstorage.hpp index 26d4a8ccd..3f4b43027 100644 --- a/include/xtensor/containers/xstorage.hpp +++ b/include/xtensor/containers/xstorage.hpp @@ -897,12 +897,15 @@ namespace xt template inline void svector::assign(size_type n, const value_type& v) { + // v may refer to an element of this vector, and grow() may free the + // buffer that holds it. + const value_type value(v); if (n > N && n > capacity()) { grow(n); } m_end = m_begin + n; - std::fill(begin(), end(), v); + std::fill(begin(), end(), value); } template @@ -1022,9 +1025,16 @@ namespace xt { if (m_end >= m_capacity) { + // elt may refer to an element of this vector, and grow() may free + // the buffer that holds it. + T value(elt); grow(); + *(m_end++) = std::move(value); + } + else + { + *(m_end++) = elt; } - *(m_end++) = elt; } template @@ -1032,9 +1042,15 @@ namespace xt { if (m_end >= m_capacity) { + // Same as above: take the value out before grow() runs. + T value(std::move(elt)); grow(); + *(m_end++) = std::move(value); + } + else + { + *(m_end++) = std::move(elt); } - *(m_end++) = std::move(elt); } template @@ -1196,13 +1212,10 @@ namespace xt template inline auto svector::insert(const_iterator cit, const T& elt) -> iterator { + // elt may refer to an element of this vector: the shift below moves it + // and grow() may free the buffer that holds it. + T value(elt); auto it = const_cast(cit); - if (it == m_end) - { - push_back(elt); - return m_end - 1; - } - if (m_end >= m_capacity) { std::ptrdiff_t elt_no = it - m_begin; @@ -1210,17 +1223,9 @@ namespace xt it = m_begin + elt_no; } - (*m_end) = back(); - std::move_backward(it, m_end - 1, m_end); + std::move_backward(it, m_end, m_end + 1); ++m_end; - - // Update ref if element moved - const T* elt_ptr = &elt; - bool cond = it <= elt_ptr && elt_ptr < m_end; - // More complicated than incrementing elt_ptr, but this avoids - // false positive array-bounds warning on GCC 10 - const T* src_ptr = cond ? it + (elt_ptr - it) + std::ptrdiff_t(1) : elt_ptr; - *it = *src_ptr; + *it = std::move(value); return it; } diff --git a/test/test_xstorage.cpp b/test/test_xstorage.cpp index cb66a8c69..8c9feb870 100644 --- a/test/test_xstorage.cpp +++ b/test/test_xstorage.cpp @@ -145,6 +145,69 @@ namespace xt EXPECT_TRUE(std::equal(s.begin(), s.end(), v.begin())); } + // A full heap buffer: the next insertion reallocates and frees the old + // buffer, the one that holds the aliased argument in the tests below. + using small_svector_type = svector; + + TEST(svector, insert_aliased_element) + { + { + small_svector_type s = {1, 2, 3}; + ASSERT_FALSE(s.on_stack()); + ASSERT_EQ(s.size(), s.capacity()); + s.insert(s.begin(), s[0]); + EXPECT_EQ(s, small_svector_type({1, 1, 2, 3})); + } + + { + small_svector_type s = {1, 2, 3}; + ASSERT_EQ(s.size(), s.capacity()); + s.insert(s.begin() + 1, s[1]); + EXPECT_EQ(s, small_svector_type({1, 2, 2, 3})); + } + + { + small_svector_type s = {1, 2, 3}; + ASSERT_EQ(s.size(), s.capacity()); + s.insert(s.end(), s[0]); + EXPECT_EQ(s, small_svector_type({1, 2, 3, 1})); + } + + { + // No reallocation: the shift moves the aliased element. + small_svector_type s = {1, 2, 3}; + s.reserve(8); + s.insert(s.begin(), s[1]); + EXPECT_EQ(s, small_svector_type({2, 1, 2, 3})); + } + } + + TEST(svector, push_back_aliased_element) + { + { + small_svector_type s = {1, 2, 3}; + ASSERT_FALSE(s.on_stack()); + ASSERT_EQ(s.size(), s.capacity()); + s.push_back(s[0]); + EXPECT_EQ(s, small_svector_type({1, 2, 3, 1})); + } + + { + small_svector_type s = {1, 2, 3}; + ASSERT_EQ(s.size(), s.capacity()); + s.push_back(std::move(s[1])); + EXPECT_EQ(s, small_svector_type({1, 2, 3, 2})); + } + } + + TEST(svector, assign_aliased_element) + { + small_svector_type s = {7, 2, 3}; + ASSERT_FALSE(s.on_stack()); + s.assign(5, s[0]); + EXPECT_EQ(s, small_svector_type({7, 7, 7, 7, 7})); + } + TEST(svector, insert_range) { svector_type s0 = {1, 2, 3, 4};