rust-ndarray / ndarray

ndarray: an N-dimensional array with array views, multidimensional slicing, and efficient operations
https://docs.rs/ndarray/
Apache License 2.0
3.62k stars 307 forks source link

Unsoundness in `last_mut`for `Arc`- and `CowArray`s #1427

Closed akern40 closed 2 months ago

akern40 commented 2 months ago

I believe that last_mut may be unsound for ArcArray and CowArray, as it neither checks nor enforces the uniqueness guarantee necessary for mutating ArcArrays and CowArrays. last_mut's body on 0.16.1 is as follows:

pub fn last_mut(&mut self) -> Option<&mut A>
where S: DataMut
{
    if self.is_empty() {
        None
    } else {
        let mut index = self.raw_dim();
        for ax in 0..index.ndim() {
            index[ax] -= 1;
        }
        Some(unsafe { self.uget_mut(index) })
    }
}

Compare that to first_mut's:

pub fn first_mut(&mut self) -> Option<&mut A>
where S: DataMut
{
    if self.is_empty() {
        None
    } else {
        Some(unsafe { &mut *self.as_mut_ptr() })
    }
}

They both seem to use some unsafety, which is fine, but the key is that first_mut's call to .as_mut_ptr contains a call to .try_ensure_unique, while the call to .uget_mut does not. In fact, uget_mut explicitly states in its safety section that the data must be uniquely held by the array, which last_mut doesn't guarantee. As a result, I believe that the mutable reference of last_mut, if obtained from a shared ArcArray and actually mutated, is unsound.

bluss commented 2 months ago

Protected by debug assertions but still a bad bug of course. It means that tests would have caught this, and users would have seen the assertion, in debug builds.