Fix oob get_unchecked, overlapping _nonoverlapping #21
Conversation
In quite a few places this code wants a pointer into a slice, and it gets that pointer by calling get_unchecked. This is potentially valid, but in general extremely dangerous because get_unchecked is a slice method even when called on a Vec, so regardless of whether the pointer would point to valid memory, if the index is out of bounds of the slice, that's UB. Offsetting pointers provides the less surprising semantics.
| ptr::copy_nonoverlapping(self.dropped.as_ptr(), &mut self.slice[self.write], self.dropped.len()); | ||
| ptr::copy_nonoverlapping(self.dropped.as_ptr(), self.slice.as_mut_ptr().add(self.write), self.dropped.len()); |
There was a problem hiding this comment.
In this case, the pointer created here is a pointer to a single element, but this call actually needs a pointer which has provenance over a range. This could be created with something like &mut self.slice[self.write..][..self.dropped.len()] as *mut T but just offsetting a pointer to the whole slice is simpler.
| ptr::copy_nonoverlapping(value, vec.get_unchecked_mut(old_len), 1); | ||
| ptr::copy_nonoverlapping(value, vec.as_mut_ptr().add(old_len), 1); |
There was a problem hiding this comment.
This get_unchecked_mut is always out-of-bounds by definition. If you can tolerate a recent Rust version, this could also be implemented with &mut vec.spare_capacity_mut()[0], though I think pointer offsetting alone is totally fine here.
| ptr::copy_nonoverlapping(slice.get_unchecked(source), slice.get_unchecked_mut(dest), 1); | ||
| let ptr = slice.as_mut_ptr(); | ||
| ptr::copy_nonoverlapping(ptr.add(source), ptr.add(dest), 1); |
There was a problem hiding this comment.
This change is only required with -Zmiri-tag-raw-pointers. But also, this could be implemented with slice::swap_unchecked if that were stabilized.
| s.slice.get_unchecked(s.write), | ||
| s.dropped.get_unchecked_mut(old_len), | ||
| s.slice.as_ptr().add(s.write), | ||
| s.dropped.as_mut_ptr().add(old_len), |
There was a problem hiding this comment.
Both of these indexes are out-of-bounds for the slice, but inbounds for the backing allocation.
| if read != s.write { | ||
| unsafe_copy(s.slice, read, s.write); | ||
| } |
There was a problem hiding this comment.
Some of the calls here to unsafe_copy pass 0 for both the read and write index, making this an overlapping copy.
|
|
Nearly all of the fixes here are actually found by the standard library's debug assertions, if you build with
-Zbuild-stdyou can turn them on. But for code like this, I think Miri is the more appropriate tool for ensuring that a validity regression doesn't occur, especially because the test cases here are small enough and provide good coverage to deal with the slowdown of a checking interpreter.