Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix some aliasing issues in Vec #70558

Merged
merged 9 commits into from
Apr 5, 2020
71 changes: 66 additions & 5 deletions src/liballoc/tests/vec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1351,24 +1351,85 @@ fn test_try_reserve_exact() {
}

#[test]
fn test_stable_push_pop() {
fn test_stable_pointers() {
/// Pull an element from the iterator, then drop it.
/// Useful to cover both the `next` and `drop` paths of an iterator.
fn next_then_drop<I: Iterator>(mut i: I) {
i.next().unwrap();
drop(i);
}

// Test that, if we reserved enough space, adding and removing elements does not
// invalidate references into the vector (such as `v0`). This test also
// runs in Miri, which would detect such problems.
let mut v = Vec::with_capacity(10);
let mut v = Vec::with_capacity(128);
v.push(13);

// laundering the lifetime -- we take care that `v` does not reallocate, so that's okay.
let v0 = unsafe { &*(&v[0] as *const _) };

// Laundering the lifetime -- we take care that `v` does not reallocate, so that's okay.
let v0 = &mut v[0];
let v0 = unsafe { &mut *(v0 as *mut _) };
// Now do a bunch of things and occasionally use `v0` again to assert it is still valid.

// Pushing/inserting and popping/removing
v.push(1);
v.push(2);
v.insert(1, 1);
assert_eq!(*v0, 13);
v.remove(1);
v.pop().unwrap();
assert_eq!(*v0, 13);
v.push(1);
v.swap_remove(1);
assert_eq!(v.len(), 2);
v.swap_remove(1); // swap_remove the last element
assert_eq!(*v0, 13);

// Appending
v.append(&mut vec![27, 19]);
assert_eq!(*v0, 13);

// Extending
v.extend_from_slice(&[1, 2]);
v.extend(&[1, 2]); // `slice::Iter` (with `T: Copy`) specialization
v.extend(vec![2, 3]); // `vec::IntoIter` specialization
v.extend(std::iter::once(3)); // `TrustedLen` specialization
v.extend(std::iter::empty::<i32>()); // `TrustedLen` specialization with empty iterator
v.extend(std::iter::once(3).filter(|_| true)); // base case
v.extend(std::iter::once(&3)); // `cloned` specialization
assert_eq!(*v0, 13);

// Truncation
v.truncate(2);
assert_eq!(*v0, 13);

// Resizing
v.resize_with(v.len() + 10, || 42);
assert_eq!(*v0, 13);
v.resize_with(2, || panic!());
assert_eq!(*v0, 13);

// No-op reservation
v.reserve(32);
v.reserve_exact(32);
assert_eq!(*v0, 13);

// Partial draining
v.resize_with(10, || 42);
next_then_drop(v.drain(5..));
assert_eq!(*v0, 13);

// Splicing
v.resize_with(10, || 42);
next_then_drop(v.splice(5.., vec![1, 2, 3, 4, 5])); // empty tail after range
assert_eq!(*v0, 13);
next_then_drop(v.splice(5..8, vec![1])); // replacement is smaller than original range
assert_eq!(*v0, 13);
next_then_drop(v.splice(5..6, vec![1; 10].into_iter().filter(|_| true))); // lower bound not exact
assert_eq!(*v0, 13);

// Smoke test that would fire even outside Miri if an actual relocation happened.
*v0 -= 13;
assert_eq!(v[0], 0);
}

// https://github.com/rust-lang/rust/pull/49496 introduced specialization based on:
Expand Down
22 changes: 13 additions & 9 deletions src/liballoc/vec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -739,7 +739,8 @@ impl<T> Vec<T> {
if len > self.len {
return;
}
let s = self.get_unchecked_mut(len..) as *mut _;
let remaining_len = self.len - len;
let s = slice::from_raw_parts_mut(self.as_mut_ptr().add(len), remaining_len);
self.len = len;
ptr::drop_in_place(s);
}
Expand Down Expand Up @@ -962,13 +963,15 @@ impl<T> Vec<T> {
#[inline]
#[stable(feature = "rust1", since = "1.0.0")]
pub fn swap_remove(&mut self, index: usize) -> T {
let len = self.len();
assert!(index < len);
unsafe {
// We replace self[index] with the last element. Note that if the
// bounds check on hole succeeds there must be a last element (which
// bounds check above succeeds there must be a last element (which
// can be self[index] itself).
let hole: *mut T = &mut self[index];
let last = ptr::read(self.get_unchecked(self.len - 1));
self.len -= 1;
let last = ptr::read(self.as_ptr().add(len - 1));
let hole: *mut T = self.as_mut_ptr().add(index);
self.set_len(len - 1);
ptr::replace(hole, last)
}
}
Expand Down Expand Up @@ -1199,7 +1202,7 @@ impl<T> Vec<T> {
} else {
unsafe {
self.len -= 1;
Some(ptr::read(self.get_unchecked(self.len())))
Some(ptr::read(self.as_ptr().add(self.len())))
}
}
}
Expand Down Expand Up @@ -2019,7 +2022,7 @@ where
let (lower, _) = iterator.size_hint();
let mut vector = Vec::with_capacity(lower.saturating_add(1));
unsafe {
ptr::write(vector.get_unchecked_mut(0), element);
ptr::write(vector.as_mut_ptr(), element);
vector.set_len(1);
}
vector
Expand Down Expand Up @@ -2122,8 +2125,9 @@ where
self.reserve(slice.len());
unsafe {
let len = self.len();
let dst_slice = slice::from_raw_parts_mut(self.as_mut_ptr().add(len), slice.len());
dst_slice.copy_from_slice(slice);
self.set_len(len + slice.len());
self.get_unchecked_mut(len..).copy_from_slice(slice);
Copy link
Member Author

@RalfJung RalfJung Mar 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately there is no good way to grep for all places where the implicit DerefMut coercion is used, like here. I grepped for get_unchecked_mut and get_unchecked and got rid of those.

Copy link
Contributor

@elichai elichai Apr 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could maybe print the backtrace in the DerefMut implementation and then run the tests and it will print what functions call DerefMut?(locally obviously hehe)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since DerefMut is called for every mutable indexing operation (&mut v[i]), I expect that to show tons of stacktraces and not be very useful.

}
}
}
Expand All @@ -2144,7 +2148,7 @@ impl<T> Vec<T> {
self.reserve(lower.saturating_add(1));
}
unsafe {
ptr::write(self.get_unchecked_mut(len), element);
ptr::write(self.as_mut_ptr().add(len), element);
// NB can't overflow since we would have had to alloc the address space
self.set_len(len + 1);
}
Expand Down