From f251dc446f753edc0797bfcc5ed48ad8f477e9ad Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 19 Sep 2020 11:08:47 +0200 Subject: [PATCH 1/4] VecDeque: fix incorrect &mut aliasing in IterMut::next/next_back --- library/alloc/src/collections/vec_deque.rs | 31 +++++++++++++++++----- 1 file changed, 24 insertions(+), 7 deletions(-) diff --git a/library/alloc/src/collections/vec_deque.rs b/library/alloc/src/collections/vec_deque.rs index adb08543334f..39b4223b0fd7 100644 --- a/library/alloc/src/collections/vec_deque.rs +++ b/library/alloc/src/collections/vec_deque.rs @@ -14,6 +14,7 @@ use core::cmp::{self, Ordering}; use core::fmt; use core::hash::{Hash, Hasher}; use core::iter::{repeat_with, FromIterator, FusedIterator}; +use core::marker::PhantomData; use core::mem::{self, replace, ManuallyDrop}; use core::ops::{Index, IndexMut, Range, RangeBounds, Try}; use core::ptr::{self, NonNull}; @@ -982,7 +983,12 @@ impl VecDeque { /// ``` #[stable(feature = "rust1", since = "1.0.0")] pub fn iter_mut(&mut self) -> IterMut<'_, T> { - IterMut { tail: self.tail, head: self.head, ring: unsafe { self.buffer_as_mut_slice() } } + IterMut { + tail: self.tail, + head: self.head, + ring: unsafe { self.buffer_as_mut_slice() }, + phantom: PhantomData, + } } /// Returns a pair of slices which contain, in order, the contents of the @@ -1175,6 +1181,7 @@ impl VecDeque { head, // The shared reference we have in &mut self is maintained in the '_ of IterMut. ring: unsafe { self.buffer_as_mut_slice() }, + phantom: PhantomData, } } @@ -2662,15 +2669,19 @@ impl FusedIterator for Iter<'_, T> {} /// [`iter_mut`]: VecDeque::iter_mut #[stable(feature = "rust1", since = "1.0.0")] pub struct IterMut<'a, T: 'a> { - ring: &'a mut [T], + ring: *mut [T], tail: usize, head: usize, + phantom: PhantomData<&'a mut [T]>, } #[stable(feature = "collection_debug", since = "1.17.0")] impl fmt::Debug for IterMut<'_, T> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - let (front, back) = RingSlices::ring_slices(&*self.ring, self.head, self.tail); + // FIXME: this creates a reference to the full ring, including the part + // to which we already handed out mutable references via `next()`. This + // is an aliasing violation. + let (front, back) = RingSlices::ring_slices(unsafe { &*self.ring }, self.head, self.tail); f.debug_tuple("IterMut").field(&front).field(&back).finish() } } @@ -2689,7 +2700,7 @@ impl<'a, T> Iterator for IterMut<'a, T> { unsafe { let elem = self.ring.get_unchecked_mut(tail); - Some(&mut *(elem as *mut _)) + Some(&mut *elem) } } @@ -2703,7 +2714,10 @@ impl<'a, T> Iterator for IterMut<'a, T> { where F: FnMut(Acc, Self::Item) -> Acc, { - let (front, back) = RingSlices::ring_slices(self.ring, self.head, self.tail); + // FIXME: this creates a reference to the full ring, including the part + // to which we already handed out mutable references via `next()`. This + // is an aliasing violation. + let (front, back) = RingSlices::ring_slices(unsafe { &mut *self.ring }, self.head, self.tail); accum = front.iter_mut().fold(accum, &mut f); back.iter_mut().fold(accum, &mut f) } @@ -2735,7 +2749,7 @@ impl<'a, T> DoubleEndedIterator for IterMut<'a, T> { unsafe { let elem = self.ring.get_unchecked_mut(self.head); - Some(&mut *(elem as *mut _)) + Some(&mut *elem) } } @@ -2743,7 +2757,10 @@ impl<'a, T> DoubleEndedIterator for IterMut<'a, T> { where F: FnMut(Acc, Self::Item) -> Acc, { - let (front, back) = RingSlices::ring_slices(self.ring, self.head, self.tail); + // FIXME: this creates a reference to the full ring, including the part + // to which we already handed out mutable references via `next()`. This + // is an aliasing violation. + let (front, back) = RingSlices::ring_slices(unsafe { &mut *self.ring }, self.head, self.tail); accum = back.iter_mut().rfold(accum, &mut f); front.iter_mut().rfold(accum, &mut f) } From e4c1a3867f6380f4e0fe48119306ae5b56d74470 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 19 Sep 2020 11:26:58 +0200 Subject: [PATCH 2/4] VecDeque: avoid more aliasing issues by working with raw pointers instead of references --- library/alloc/src/collections/vec_deque.rs | 43 ++++++++++++++++------ 1 file changed, 31 insertions(+), 12 deletions(-) diff --git a/library/alloc/src/collections/vec_deque.rs b/library/alloc/src/collections/vec_deque.rs index 39b4223b0fd7..0f34786b18af 100644 --- a/library/alloc/src/collections/vec_deque.rs +++ b/library/alloc/src/collections/vec_deque.rs @@ -2500,6 +2500,25 @@ impl RingSlices for &mut [T] { } } +impl RingSlices for *mut [T] { + fn slice(self, from: usize, to: usize) -> Self { + assert!(from <= to && to < self.len()); + // Not using `get_unchecked_mut` to keep this a safe operation. + let len = to - from; + ptr::slice_from_raw_parts_mut(self.as_mut_ptr().wrapping_add(from), len) + } + + fn split_at(self, mid: usize) -> (Self, Self) { + let len = self.len(); + let ptr = self.as_mut_ptr(); + assert!(mid <= len); + ( + ptr::slice_from_raw_parts_mut(ptr, mid), + ptr::slice_from_raw_parts_mut(ptr.wrapping_add(mid), len - mid), + ) + } +} + /// Calculate the number of elements left to be read in the buffer #[inline] fn count(tail: usize, head: usize, size: usize) -> usize { @@ -2678,10 +2697,10 @@ pub struct IterMut<'a, T: 'a> { #[stable(feature = "collection_debug", since = "1.17.0")] impl fmt::Debug for IterMut<'_, T> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - // FIXME: this creates a reference to the full ring, including the part - // to which we already handed out mutable references via `next()`. This - // is an aliasing violation. - let (front, back) = RingSlices::ring_slices(unsafe { &*self.ring }, self.head, self.tail); + let (front, back) = RingSlices::ring_slices(self.ring, self.head, self.tail); + // SAFETY: these are the elements we have not handed out yet, so aliasing is fine. + // We also ensure everything is dereferencable and in-bounds. + let (front, back) = unsafe { (&*front, &*back) }; f.debug_tuple("IterMut").field(&front).field(&back).finish() } } @@ -2714,10 +2733,10 @@ impl<'a, T> Iterator for IterMut<'a, T> { where F: FnMut(Acc, Self::Item) -> Acc, { - // FIXME: this creates a reference to the full ring, including the part - // to which we already handed out mutable references via `next()`. This - // is an aliasing violation. - let (front, back) = RingSlices::ring_slices(unsafe { &mut *self.ring }, self.head, self.tail); + let (front, back) = RingSlices::ring_slices(self.ring, self.head, self.tail); + // SAFETY: these are the elements we have not handed out yet, so aliasing is fine. + // We also ensure everything is dereferencable and in-bounds. + let (front, back) = unsafe { (&mut *front, &mut *back) }; accum = front.iter_mut().fold(accum, &mut f); back.iter_mut().fold(accum, &mut f) } @@ -2757,10 +2776,10 @@ impl<'a, T> DoubleEndedIterator for IterMut<'a, T> { where F: FnMut(Acc, Self::Item) -> Acc, { - // FIXME: this creates a reference to the full ring, including the part - // to which we already handed out mutable references via `next()`. This - // is an aliasing violation. - let (front, back) = RingSlices::ring_slices(unsafe { &mut *self.ring }, self.head, self.tail); + let (front, back) = RingSlices::ring_slices(self.ring, self.head, self.tail); + // SAFETY: these are the elements we have not handed out yet, so aliasing is fine. + // We also ensure everything is dereferencable and in-bounds. + let (front, back) = unsafe { (&mut *front, &mut *back) }; accum = back.iter_mut().rfold(accum, &mut f); front.iter_mut().rfold(accum, &mut f) } From 69669cbdb2a98bcf8c49062b9606e14ce51ecf71 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 19 Sep 2020 11:55:45 +0200 Subject: [PATCH 3/4] make IterMut Send/Sync again --- library/alloc/src/collections/vec_deque.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/library/alloc/src/collections/vec_deque.rs b/library/alloc/src/collections/vec_deque.rs index 0f34786b18af..e8e9dae01118 100644 --- a/library/alloc/src/collections/vec_deque.rs +++ b/library/alloc/src/collections/vec_deque.rs @@ -2694,6 +2694,13 @@ pub struct IterMut<'a, T: 'a> { phantom: PhantomData<&'a mut [T]>, } +// SAFETY: we do nothing thread-local and there is no interior mutability, +// so the usual structural `Send`/`Sync` apply. +#[stable(feature = "rust1", since = "1.0.0")] +unsafe impl Send for IterMut<'_, T> {} +#[stable(feature = "rust1", since = "1.0.0")] +unsafe impl Sync for IterMut<'_, T> {} + #[stable(feature = "collection_debug", since = "1.17.0")] impl fmt::Debug for IterMut<'_, T> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { From fa6a4f7d374f2773a58eb10bf0cfe8e00d359039 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 6 Oct 2020 09:40:48 +0200 Subject: [PATCH 4/4] avoid unnecessary intermediate reference and improve safety comments --- library/alloc/src/collections/vec_deque.rs | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/library/alloc/src/collections/vec_deque.rs b/library/alloc/src/collections/vec_deque.rs index e8e9dae01118..ff9b1553bf2f 100644 --- a/library/alloc/src/collections/vec_deque.rs +++ b/library/alloc/src/collections/vec_deque.rs @@ -983,10 +983,12 @@ impl VecDeque { /// ``` #[stable(feature = "rust1", since = "1.0.0")] pub fn iter_mut(&mut self) -> IterMut<'_, T> { + // SAFETY: The internal `IterMut` safety invariant is established because the + // `ring` we create is a dereferencable slice for lifetime '_. IterMut { tail: self.tail, head: self.head, - ring: unsafe { self.buffer_as_mut_slice() }, + ring: ptr::slice_from_raw_parts_mut(self.ptr(), self.cap()), phantom: PhantomData, } } @@ -1176,11 +1178,13 @@ impl VecDeque { R: RangeBounds, { let (tail, head) = self.range_tail_head(range); + + // SAFETY: The internal `IterMut` safety invariant is established because the + // `ring` we create is a dereferencable slice for lifetime '_. IterMut { tail, head, - // The shared reference we have in &mut self is maintained in the '_ of IterMut. - ring: unsafe { self.buffer_as_mut_slice() }, + ring: ptr::slice_from_raw_parts_mut(self.ptr(), self.cap()), phantom: PhantomData, } } @@ -2688,6 +2692,7 @@ impl FusedIterator for Iter<'_, T> {} /// [`iter_mut`]: VecDeque::iter_mut #[stable(feature = "rust1", since = "1.0.0")] pub struct IterMut<'a, T: 'a> { + // Internal safety invariant: the entire slice is dereferencable. ring: *mut [T], tail: usize, head: usize, @@ -2706,7 +2711,7 @@ impl fmt::Debug for IterMut<'_, T> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { let (front, back) = RingSlices::ring_slices(self.ring, self.head, self.tail); // SAFETY: these are the elements we have not handed out yet, so aliasing is fine. - // We also ensure everything is dereferencable and in-bounds. + // The `IterMut` invariant also ensures everything is dereferencable. let (front, back) = unsafe { (&*front, &*back) }; f.debug_tuple("IterMut").field(&front).field(&back).finish() } @@ -2742,7 +2747,7 @@ impl<'a, T> Iterator for IterMut<'a, T> { { let (front, back) = RingSlices::ring_slices(self.ring, self.head, self.tail); // SAFETY: these are the elements we have not handed out yet, so aliasing is fine. - // We also ensure everything is dereferencable and in-bounds. + // The `IterMut` invariant also ensures everything is dereferencable. let (front, back) = unsafe { (&mut *front, &mut *back) }; accum = front.iter_mut().fold(accum, &mut f); back.iter_mut().fold(accum, &mut f) @@ -2785,7 +2790,7 @@ impl<'a, T> DoubleEndedIterator for IterMut<'a, T> { { let (front, back) = RingSlices::ring_slices(self.ring, self.head, self.tail); // SAFETY: these are the elements we have not handed out yet, so aliasing is fine. - // We also ensure everything is dereferencable and in-bounds. + // The `IterMut` invariant also ensures everything is dereferencable. let (front, back) = unsafe { (&mut *front, &mut *back) }; accum = back.iter_mut().rfold(accum, &mut f); front.iter_mut().rfold(accum, &mut f)