Prevent view creation with underflowed size

[m-ogita]

Checklist

• The title and commit message(s) are descriptive.
• Small commits made to fix your PR have been squashed to avoid history pollution.
• Tests have been added for new features or bug fixes.
• API of new functions and classes are documented.

Description

When a slice with conditions such as start > stop && step > 0 (or vice versa) is used to create a view (sliced or strided), the resulting array view can have an enormous size. Accessing its elements then leads to undefined behavior.

Here is a minimal example:

#include <iostream>
#include <xtensor/containers/xarray.hpp>
#include <xtensor/views/xview.hpp>

int main()
{
    xt::xarray<int>::shape_type shape = {5};
    xt::xarray<int> a(shape);

    // v := a[3:0:1]
    auto slice = xt::range(3, 0, 1);
    auto v = xt::view(a, slice);

    // Print the shape of v
    for (auto e : v.shape())
    {
        std::cout << e << " ";
    }
    std::cout << std::endl;
}

The expected output is 0, but the actual output is 18446744073709551613.

The cause of this issue appears to be the size calculation in the xstepped_range constructor in xtensor/include/xtensor/views/xslice.hpp, shown below:

template <class T>
inline xstepped_range<T>::xstepped_range(size_type start_val, size_type stop_val, size_type step) noexcept
    : m_start(start_val)
    , m_size(size_type(0))
    , m_step(step)
{
    size_type n = stop_val - start_val;
    m_size = n / step + (((n < 0) ^ (step > 0)) && (n % step));
}

When start_val is greater than stop_val (or more precisely, when stop_val - start_val has the opposite sign from step), m_size can become negative. This subsequently causes the resulting size to underflow.

One simple way to fix this issue is to clamp m_size to a minimum value of 0:

template <class T>
inline xstepped_range<T>::xstepped_range(size_type start_val, size_type stop_val, size_type step) noexcept
    : m_start(start_val)
    , m_size(size_type(0))
    , m_step(step)
{
    size_type n = stop_val - start_val;
    m_size = std::max(n / step + (((n < 0) ^ (step > 0)) && (n % step)), size_type(0));
}

Note on terminology: > I used the term "underflow" to describe "integer underflow", the behavior where subtracting from a small unsigned integer results in a huge value.

[JohanMabille]

Thanks for the fix. No need to perform the computation if stop_val is greater than start_val though.

[JohanMabille]

Suggested change

	m_size = std::max(n / step + (((n < 0) ^ (step > 0)) && (n % step)), size_type(0));
	m_size = stop_val > start_val ? n / step + (((n < 0) ^ (step > 0)) && (n % step)) : 0u;

[m-ogita]

Thanks. Fixed to use the ternary operator.

        m_size = (n > 0) ^ (step > 0) ? 0 : n / step + static_cast<bool>(n % step);

• When n == 0: The result will always be 0. (If step > 0, the condition is true and returns 0. If step < 0, the condition is false, but 0 / step + 0 evaluates to 0).
• When n and step have opposite signs ((n > 0) ^ (step > 0) is true): This is the bug-fix case. It now correctly evaluates to 0 immediately.
• When n and step have the same sign: The condition is false, so the latter expression is evaluated. This is equivalent to the original expression since the logical adjustment ((n < 0) ^ (step > 0)) in the original code is false.

[JohanMabille]

if size_type is unsigned, n and step are always positive. Besides, this makes the code harder to read than a simple comparison of bounds as I suggested. Can you fix this code accordingly, and please add a unit test for this use case?

[m-ogita]

Thanks for the feedback. You are absolutely right that we need to be careful with unsigned types. Both of my suggestions wouldn't work in such cases.

However, if we use your suggested approach, it doesn't quite handle cases such as slice being xt::range(0, 3, -1), where step is negative and the size should be 0. If we try to fix that within that structure, the conditional expression would become quite complex and hard to read:

m_size = ((step > 0 && start_val > stop_val) || (step < 0 && start_val < stop_val))
         ? n / step + (((n < 0) ^ (step > 0)) && (n % step))
         : 0u;

If you don't mind adding a few more lines, we can use an if statement to make it cleaner:

if ((step > 0 && start_val >= stop_val) || (step < 0 && start_val <= stop_val)) return 0;
size_type n = stop_val - start_val;
return n / step + (((n < 0) ^ (step > 0)) && (n % step));

The last line can be simplified to n / step + static_cast<bool>(n % step) as noted before. Another variant is n / step + (n % step != 0).

Can we go forward with this approach? And which implementation choices of the last line do you prefer?

P.S. I will add a unit test.

[JohanMabille]

Ah indeed, good catch for the negative step! We should keep the code simple and readable, so let's go with the if statement and the simplified statement.