Use wrapping_add() for ptr addition in Iterator::next()

[mtreinish]

When iterating over a view of a row major matrix and finishing a column during iteration it is possible for the pointer to be incremented outside the allocation when determining the current column has been finished. This is fine because the pointer never is actually reading from outside the allocation, the inner_end value used to track we've finished the column will match on the next call to next() and never actually attempt to read from outside the allocation for the matrix. However, this increment step is done using add() instead of wrapping_add() and per the wrapping_add() documentation [1] wrapping_add() should be used in situations where the pointer can be incremented outside the allocation. While using add() in these situations is immediately undefined behavior. This will also trigger miri failures when testing code that is building a MatrixView to a row major array and then iterating over it. This commit changes the use of pointer add() to wrapping_add() in the Iterator::next() method to fix this issue.

[1] https://doc.rust-lang.org/std/primitive.pointer.html#method.wrapping_add

[Ralith]

It's legal and routine for add to result in a pointer that is the first address past the end of the allocation. Is that not what's happening here?

[mtreinish]

That is what's happening here, but the documentation on pointers that I linked is pretty clear that using add() to advance the pointer outside the allocation is undefined behavior:

Compared to add, this method basically delays the requirement of staying within the same allocation: add is immediate Undefined Behavior when crossing object boundaries; wrapping_add produces a pointer but still leads to Undefined Behavior if a pointer is dereferenced when it is out-of-bounds of the object it is attached to

Additionally in the safety section in the add method's documentation says:

If the computed offset is non-zero, then self must be derived from a pointer to some allocation, and the entire memory range between self and the result must be in bounds of that allocation. In particular, this range must not “wrap around” the edge of the address space.

If you run iter() under miri it will error on the add call here and flag this as undefined behavior. wrapping_add() is the correct call to make if we need to increment the pointer outside the allocation like this.

[Ralith]

Read the wording you quoted more carefully:

the entire memory range between self and the result must be in bounds of that allocation

This is true when the result is the first address past the end of the allocation.

[mtreinish]

There are two reasons that you can know this is needed in this case. The first is miri is flagging it as undefined behavior when you run iteration on a row major array under miri. It is explicitly flagging that the pointer addition as going out of bounds and it being undefined behavior. The second is you can see how the pointer is being used in the current iteration code. We check inner_end and ptr in the code like:

if self.ptr == self.inner_end

and self.inner_end is set with:

// This might go past the end of the allocation,
// depending on the value of 'size'. We use
// `wrapping_offset` to avoid UB
self.inner_end = self.ptr.wrapping_offset(stride);

if we need to use wrapping_offset() to avoid UB on one side, we also need to use wrapping_add() if the values can be equal.

But also I think what you're potentially missing here is when I said it's row major not column major. So one address at this point in the code is the row stride size - 1 not just size of T. Depending on the shape of the array and the underlying allocation size you can see how it will go out of bounds.