Clarify invariants and fix constructors and is_standard_layout #543

jturner314 · 2018-11-17T04:01:47Z

Various method and iterator implementations in ndarray implicitly made some assumptions that were not enforced. For example,

In a number of places, methods/iterators violated the safety constraints on .offset(). For example, AxisIterCore offsets the pointer along the axis of iteration even if one of the other axes is zero-length. As a result, before this PR, this program would have undefined behavior:
```
extern crate ndarray;

use ndarray::prelude::*;

fn main() {
    let a = ArrayView2::<i32>::from_shape((0, 3), &[1]).unwrap();
    for v in a.axis_iter(Axis(1)) {
        println!("{:?}", v);
    }
}
```
The stride of axis 1 is 1, so AxisIter calls ptr.offset(0 * 1), ptr.offset(1 * 1), and ptr.offset(2 * 1). The first and second offsets are fine because they're "in bounds or one byte past the end of the same allocated object", but the last offset is undefined behavior.

This PR states the invariant that offsets along all axes must be safe, even if there are zero-length axes, and it checks this property in array constructors.
In some places, implementations made the assumption that the axis lengths would always be less than or equal to isize::MAX, such as in the stride_offset function in the dimension module. However, it is possible to create a Vec/slice with length greater than isize::MAX of zero-size elements, and the implementation of size_checked allowed creation of arrays with axis lengths greater than isize::MAX (e.g. ArrayView2::<i32>::from_shape((isize::MAX as usize + 1, 0), &[]).unwrap(). I have trouble finding a place where this would cause undefined behavior, but these cases could cause unexpected panics in debug mode due to overflow, and the fact that I've seen code making this invalid assumption makes me uncomfortable that there may be a soundness issue I haven't found.

This PR changes the constructors to enforce that the product of non-zero axis lengths must not exceed isize::MAX.

This PR explicitly states the invariants that ArrayBase must uphold and methods can depend on. It also adds the necessary checks to enforce these invariants when arrays are created. The invariants are carefully designed so that slices/subviews/reshapes/etc. preserve them, so checks are needed only in the constructors and not in method/iterator implementations.

This PR does make some changes to the behavior of constructors, the default strides for empty arrays, and is_standard_layout in some edge cases. However, I would still consider it a backwards-compatible change because it's enforcing assumptions that were made in previous versions that should have been checked. In other words, it's a bug fix.

A few notes about possible alternatives:

We could drop the requirement that it must be safe to offset the pointer along all axes even in empty arrays. However, this would mean we'd need to add explicit checks for empty arrays in things like AxisIter and .subview_inplace(). These checks would be easy-to-forget, which is problematic because missing a check could result in undefined behavior. I much prefer performing the necessary checks in constructors so that all the other method/iterator implementations can be simple.
We could drop the requirement that the product of non-zero axis lengths must fit in isize. This would be slightly more flexible because, for example, it be possible possible to create a 1-D array of usize::MAX zero-size elements. However, this additional flexibility doesn't have practical applications and would require us to explicitly check for overflow in a lot more places.
We could drop the requirement that the pointer must be non-null even for empty arrays. There may even be some advantage to doing this because it would make ArrayPtr/ArrayPtrMut in Add raw array pointer types #496 more flexible. However, method implementations already make this assumption (e.g. .as_slice()), allowing null pointers isn't significantly more useful than allowing dangling pointers, and forbidding null pointers will allow us to switch ArrayBase.ptr to use NonNull in the future (which should remove the need for "reborrow" methods (Add .reborrow() methods to array views #412)).

bluss · 2018-11-17T13:02:27Z

src/array_serialize.rs

+                        let dim_size = dim
+                            .size_checked()
+                            .ok_or_else(|| d.error("overflow calculating array size"))?;
+                        if len != dim_size {


Nice! This reminds me, it's more than time to drop rustc-serialize I think :)

Yeah, I was thinking the same thing. :)

src/arraytraits.rs

bluss · 2018-11-17T13:05:14Z

src/dimension/dimension_trait.rs

+    /// length `size`, and `strides` are created with `self.default_strides()`
+    /// or `self.fortran_strides()`, then the invariants are met to construct
+    /// an owned `Array` from the `Vec`, `dim`, and `strides`. (See the docs of
+    /// `can_index_slice_not_custom` for a slightly more general case.)
    fn size_checked(&self) -> Option<usize> {


I don't really see why this is needed, yet

Ok, this seems good.

Isn't there an option to let this method just work with the overflow check and then check the isize::MAX requirement in constructors?

Oh, whoops. I didn't realize that size_checked was a public method.

Initially, I didn't do this because I thought size_checked was private, and I noticed that all places but one where .size_checked() was called needed to perform the isize::MAX check too. (The only exception is this line in indices_iter_f.)

I'll change it back to the original behavior and add a separate checking function for internal use.

src/free_functions.rs

src/impl_constructors.rs

bluss · 2018-11-17T13:17:29Z

I agree with your thoughts about the tradeoffs. We don't need to prioritize zero size elements much at all, and I'll look if we can do this without making docs and code so much more complicated.

bluss · 2018-11-17T13:18:25Z

src/impl_constructors.rs

-                _ => true,
-            }
-        });
+        debug_assert!(dimension::can_index_slice(&v, &dim, &strides).is_ok());


Is this now correct? There were some negative stride issues we couldn't check properly before (we'd have a false positive, which is fine, it was a "missing feature", but we can't debug assert on that).

See my comment below. If we want to allow negative strides, we need to provide some way for the user to specify the ptr since it's always different from vec.as_mut_ptr() when there are negative strides for axes with length > 1.

bluss · 2018-11-17T13:19:37Z

src/impl_constructors.rs

+    ///
+    /// 2. The product of non-zero axis lengths must not exceed `isize::MAX`.
+    ///
+    /// 3. For axes with length > 1, the stride must be nonnegative.


Not sure about why we require this

Shouldn't this be unchecked? We don't implement the proper debug check for negative strides, but if the strides are correct, they should be fine to be used here.

If any axes with length > 1 have negative strides, moving along those axes would result in offsetting the array's pointer backwards outside the Vec. (from_shape_vec_unchecked is implemented in terms of from_vec_dim_stride_unchecked, which creates the array's pointer from v.as_mut_ptr().) We could allow negative strides if we allow the user to specify the array's ptr (or, equivalently, the offset of ptr relative to v.as_mut_ptr()).

Oh great point, I just never considered that about these constructors. Thanks for the explanation.

bluss · 2018-11-17T13:23:01Z

src/lib.rs

-    /// A pointer into the buffer held by data, may point anywhere
-    /// in its range.
+    /// A non-null and aligned pointer into the buffer held by `data`; may
+    /// point anywhere in its range.


This is good

bluss · 2018-11-17T13:24:04Z

Very nice work on this PR 😄

bluss · 2018-11-17T13:25:19Z

src/lib.rs

+//      bounds or one byte past the end of a single allocation with element
+//      type `A`. The only exceptions are if the array is empty or the element
+//      type is zero-sized. In these cases, `ptr` may be dangling, but it must
+//      still be safe to [`.offset()`] the pointer along the axes.


Ok, I guess we must accept a dangling pointer, as in the one we get from an empty Vec

bluss · 2018-11-17T14:33:38Z

src/lib.rs

+// methods/functions on the array while it violates the constraints.
+//
+// Users of the `ndarray` crate cannot rely on these constraints because they
+// may change in the future.


Whole block is super

jturner314 · 2018-11-17T19:42:16Z

By the way, one thing that surprised me was that before this PR, can_index_slice required that strides be strictly positive for axes with length > 0 (or zero for length = 0). I didn't see anything that would require this more strict constraint, so I changed the implementation to allow any stride for axes with length <= 1.

bluss · 2018-11-17T20:58:29Z

@jturner314 ok about can_index_slice, so the argument for allowing more general strides for zero length axes would be that they can come up in normal operation somehow? Otherwise it seems good to ban them

jturner314 · 2018-11-17T21:13:29Z

the argument for allowing more general strides for zero length axes would be that they can come up in normal operation somehow?

Here's an example demonstrating that arbitrary strides can occur for zero-length axes in normal operation:

extern crate ndarray;

use ndarray::{array, s, Axis};

fn main() {
    let mut a = array![[1, 2, 3], [4, 5, 6]];
    a.invert_axis(Axis(0));
    println!("a =\n{:?}", a);
    let s = a.slice(s![0..0, ..]);
    println!("s =\n{:?}", s);
}

It prints:

a =
[[4, 5, 6],
 [1, 2, 3]] shape=[2, 3], strides=[-3, 1], layout=Custom (0x0)
s =
[[]] shape=[0, 3], strides=[-3, 1], layout=Custom (0x0)

The stride of axis 0 is -3 in the view s.

We could still ban them in constructors, but I don't see a good reason to if they can occur in normal operation.

bluss · 2018-11-17T21:14:44Z

@jturner314 sounds good

jturner314 · 2018-11-18T03:25:20Z

I think I've resolved all of the comments, and it finally passes CI. :)

I'll look if we can do this without making docs and code so much more complicated.

Making the assumption that slices contain at most isize::MAX bytes simplified things quite a bit, but it still is fairly complicated. I'd love any suggestions to simplify things. Writing this PR, designing the invariants, and correctly checking all the invariants was very difficult; there were a whole bunch of edge cases to worry about (zero-size types, arrays with some zero-length axes, arrays with all zero-length axes, arrays with zero axes, overflow in shape, overflow in offset counts, maintaining invariants after slicing/subviews/reshapes, etc.). I'm pretty satisfied with the result but would welcome improvements.

bluss · 2018-11-18T17:55:48Z

Monumental effort, it's great! I'll try my hand at adding a comment more

src/free_functions.rs

bluss · 2018-11-18T23:46:46Z

ready to merge when you think so

jturner314 · 2018-11-19T03:25:48Z

I fixed some minor issues (comments/docs) and merged the PR. 🎉

jturner314 force-pushed the clarify-invariants branch from 1b5d312 to f3c4441 Compare November 17, 2018 04:40