PySliceContainer is unsound

[mejrs]

Continued from #347 (comment)

It and the functions using it derive pointers from Box, but then move the Box around which invalidates said pointers.

For example:

impl<T: Element> IntoPyArray for Box<[T]> {
    type Item = T;
    type Dim = Ix1;

    fn into_pyarray<'py>(self, py: Python<'py>) -> &'py PyArray<Self::Item, Self::Dim> {
        let dims = [self.len()];
        let strides = [mem::size_of::<T>() as npy_intp];
        let data_ptr = self.as_ptr();
        unsafe { PyArray::from_raw_parts(py, dims, strides.as_ptr(), data_ptr, self) }
    }
}

Here, self is moved when passed to this function, invalidating data_ptr.

That function later calls..

impl<T: Send> From<Box<[T]>> for PySliceContainer {
    fn from(data: Box<[T]>) -> Self {
        unsafe fn drop_boxed_slice<T>(ptr: *mut u8, len: usize, _cap: usize) {
            let _ = Box::from_raw(slice::from_raw_parts_mut(ptr as *mut T, len) as *mut [T]);
        }

        // FIXME(adamreichold): Use `Box::into_raw` when
        // `*mut [T]::{as_mut_ptr, len}` become stable and compatible with our MSRV.
        let ptr = data.as_ptr() as *mut u8;
        let len = data.len();
        let cap = 0;
        let drop = drop_boxed_slice::<T>;

        mem::forget(data);

        Self {
            ptr,
            len,
            cap,
            drop,
        }
    }
}

...where ptr is again invalidated when the box is moved into mem::forget.

This is problematic with Box especially because it has "noalias". What this means is that when you move it or take a mutable reference it asserts uniqueness, invalidating all other pointers to it.

Quoting std's documentation:

The aliasing rules for Box are the same as for &mut T. Box asserts uniqueness over its content. Using raw pointers derived from a box after that box has been mutated through, moved or borrowed as &mut T is not allowed. For more guidance on working with box from unsafe code, see rust-lang/unsafe-code-guidelines#326.

Effectively, this means dereferencing data_ptr causes undefined behavior and PyArray is basically impossible to use soundly.

As far as solutions go, there are some solutions:

• Use AliasableBox or invent one ourselves.
• Immediately dismantle the box before moving things around, with:

let box_ = ManuallyDrop::new(box_);
let (ptr, len) = (box_.as_mut_ptr(), box_.len());

Note that in addition to everything mentioned so far, there is another problem; data_ptr is created with let data_ptr = self.as_ptr(); (not as_mut_ptr), which means that mutating a PyArray created from a Box is also undefined behavior.

[adamreichold]

I have prepared #351 which will ideally become version 0.17.2 of this crate.

I don't think yanking 0.17.1 is required as we are luckily somewhat shielded from the effects of the unsoundness due these pointers always going through Python/NumPy API before Rust code touches them again (e.g. to perform modifications) so that the compiler should not be able to actually exploit Box's noalias attribute. (This seems to line up with the lack of reports of miscompilations due to this so far, but then again the absence of data is not data on the absence...)

[mejrs]

Thanks; that was quick :)

I agree with what you said; I think this is unlikely to cause problems in practice.