From 6556549fa6d74791d1e3e155ae8a4d1480ea4481 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 23 Mar 2020 13:40:40 +0100 Subject: [PATCH 1/9] fix Vec::extend invalidating unrelated pointers --- src/liballoc/vec.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/liballoc/vec.rs b/src/liballoc/vec.rs index e171edef73609..361ce20376dae 100644 --- a/src/liballoc/vec.rs +++ b/src/liballoc/vec.rs @@ -2122,8 +2122,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); } } } From 86c1c434206d5fb9b58b0847e2f4deb20340d3c5 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 30 Mar 2020 10:38:21 +0200 Subject: [PATCH 2/9] fix pointer invalidation when extnding a vector from an untrusted iterator --- src/liballoc/vec.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/liballoc/vec.rs b/src/liballoc/vec.rs index 361ce20376dae..6f264399fa884 100644 --- a/src/liballoc/vec.rs +++ b/src/liballoc/vec.rs @@ -2019,6 +2019,8 @@ where let (lower, _) = iterator.size_hint(); let mut vector = Vec::with_capacity(lower.saturating_add(1)); unsafe { + // `vector` is new, cannot have aliases, so us getting exclusive references + // here is okay. ptr::write(vector.get_unchecked_mut(0), element); vector.set_len(1); } @@ -2145,7 +2147,7 @@ impl Vec { 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); } From fa6c8830740829d38f4ac7bfc8d8131ae44b9ade Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 30 Mar 2020 10:40:59 +0200 Subject: [PATCH 3/9] fix ptr invalidation in Vec::truncate --- src/liballoc/vec.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/liballoc/vec.rs b/src/liballoc/vec.rs index 6f264399fa884..1c8c4428169fc 100644 --- a/src/liballoc/vec.rs +++ b/src/liballoc/vec.rs @@ -739,7 +739,8 @@ impl Vec { 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); } From 4393923168fee76aea78907250642ad5474b1315 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 30 Mar 2020 11:53:36 +0200 Subject: [PATCH 4/9] add some tests --- src/liballoc/tests/vec.rs | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/src/liballoc/tests/vec.rs b/src/liballoc/tests/vec.rs index 9c4ac52acac2a..831bc0346c217 100644 --- a/src/liballoc/tests/vec.rs +++ b/src/liballoc/tests/vec.rs @@ -1351,17 +1351,18 @@ fn test_try_reserve_exact() { } #[test] -fn test_stable_push_pop() { +fn test_stable_pointers() { // 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. + // Laundering the lifetime -- we take care that `v` does not reallocate, so that's okay. let v0 = unsafe { &*(&v[0] as *const _) }; - // 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); @@ -1369,6 +1370,19 @@ fn test_stable_push_pop() { v.remove(1); v.pop().unwrap(); assert_eq!(*v0, 13); + + // Extending + 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::()); // `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); } // https://github.com/rust-lang/rust/pull/49496 introduced specialization based on: From 3411ade32ea8bb2e3f62140e8d47ad43c5a759ac Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 30 Mar 2020 11:57:31 +0200 Subject: [PATCH 5/9] test more mutating vector methods --- src/liballoc/tests/vec.rs | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/src/liballoc/tests/vec.rs b/src/liballoc/tests/vec.rs index 831bc0346c217..d43bd11ebd325 100644 --- a/src/liballoc/tests/vec.rs +++ b/src/liballoc/tests/vec.rs @@ -1371,7 +1371,12 @@ fn test_stable_pointers() { v.pop().unwrap(); 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 @@ -1383,6 +1388,31 @@ fn test_stable_pointers() { // 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); + drop(v.drain(5..)); + assert_eq!(*v0, 13); + + // Splicing + v.resize_with(10, || 42); + drop(v.splice(5.., vec![1, 2, 3, 4, 5])); // empty tail after range + assert_eq!(*v0, 13); + drop(v.splice(5..8, vec![1])); // replacement is smaller than original range + assert_eq!(*v0, 13); + drop(v.splice(5..6, vec![1; 10].into_iter().filter(|_| true))); // lower bound not exact + assert_eq!(*v0, 13); } // https://github.com/rust-lang/rust/pull/49496 introduced specialization based on: From 4eacf45c9cf6bd110dc019082c2a4a8fd9668d66 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 30 Mar 2020 13:01:15 +0200 Subject: [PATCH 6/9] also cover next() path of draining iterators --- src/liballoc/tests/vec.rs | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/liballoc/tests/vec.rs b/src/liballoc/tests/vec.rs index d43bd11ebd325..05fa7589639fc 100644 --- a/src/liballoc/tests/vec.rs +++ b/src/liballoc/tests/vec.rs @@ -1352,6 +1352,13 @@ fn test_try_reserve_exact() { #[test] 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(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. @@ -1402,16 +1409,16 @@ fn test_stable_pointers() { // Partial draining v.resize_with(10, || 42); - drop(v.drain(5..)); + next_then_drop(v.drain(5..)); assert_eq!(*v0, 13); // Splicing v.resize_with(10, || 42); - drop(v.splice(5.., vec![1, 2, 3, 4, 5])); // empty tail after range + next_then_drop(v.splice(5.., vec![1, 2, 3, 4, 5])); // empty tail after range assert_eq!(*v0, 13); - drop(v.splice(5..8, vec![1])); // replacement is smaller than original range + next_then_drop(v.splice(5..8, vec![1])); // replacement is smaller than original range assert_eq!(*v0, 13); - drop(v.splice(5..6, vec![1; 10].into_iter().filter(|_| true))); // lower bound not exact + next_then_drop(v.splice(5..6, vec![1; 10].into_iter().filter(|_| true))); // lower bound not exact assert_eq!(*v0, 13); } From 8f479e362fbfcb31e83396ef850ab5219a32821e Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 30 Mar 2020 13:11:37 +0200 Subject: [PATCH 7/9] fix aliasing in remove() also add smoke test to detect relocation even in rustc runs --- src/liballoc/tests/vec.rs | 7 ++++++- src/liballoc/vec.rs | 6 ++---- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/liballoc/tests/vec.rs b/src/liballoc/tests/vec.rs index 05fa7589639fc..a90bc58cbfd5d 100644 --- a/src/liballoc/tests/vec.rs +++ b/src/liballoc/tests/vec.rs @@ -1366,7 +1366,8 @@ fn test_stable_pointers() { 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 _) }; + 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 @@ -1420,6 +1421,10 @@ fn test_stable_pointers() { 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: diff --git a/src/liballoc/vec.rs b/src/liballoc/vec.rs index 1c8c4428169fc..dc2a246f81776 100644 --- a/src/liballoc/vec.rs +++ b/src/liballoc/vec.rs @@ -1200,7 +1200,7 @@ impl Vec { } else { unsafe { self.len -= 1; - Some(ptr::read(self.get_unchecked(self.len()))) + Some(ptr::read(self.as_ptr().add(self.len()))) } } } @@ -2020,9 +2020,7 @@ where let (lower, _) = iterator.size_hint(); let mut vector = Vec::with_capacity(lower.saturating_add(1)); unsafe { - // `vector` is new, cannot have aliases, so us getting exclusive references - // here is okay. - ptr::write(vector.get_unchecked_mut(0), element); + ptr::write(vector.as_mut_ptr(), element); vector.set_len(1); } vector From 5bbaac357dd85092ed0fb822947df7a4d60c1db9 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 30 Mar 2020 13:31:16 +0200 Subject: [PATCH 8/9] fix and test aliasing in swap_remove --- src/liballoc/tests/vec.rs | 5 +++++ src/liballoc/vec.rs | 7 ++++--- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/liballoc/tests/vec.rs b/src/liballoc/tests/vec.rs index a90bc58cbfd5d..6321e7154e7d0 100644 --- a/src/liballoc/tests/vec.rs +++ b/src/liballoc/tests/vec.rs @@ -1378,6 +1378,11 @@ fn test_stable_pointers() { 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]); diff --git a/src/liballoc/vec.rs b/src/liballoc/vec.rs index dc2a246f81776..c600a6b649f36 100644 --- a/src/liballoc/vec.rs +++ b/src/liballoc/vec.rs @@ -963,12 +963,13 @@ impl Vec { #[inline] #[stable(feature = "rust1", since = "1.0.0")] pub fn swap_remove(&mut self, index: usize) -> T { + assert!(index < self.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)); + let last = ptr::read(self.as_ptr().add(self.len - 1)); + let hole: *mut T = self.as_mut_ptr().add(index); self.len -= 1; ptr::replace(hole, last) } From 7e81c11aa8ddcebf64c01579754b44930ecf4d04 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 5 Apr 2020 08:40:40 +0200 Subject: [PATCH 9/9] tweak swap_remove --- src/liballoc/vec.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/liballoc/vec.rs b/src/liballoc/vec.rs index c600a6b649f36..aedb3724409fd 100644 --- a/src/liballoc/vec.rs +++ b/src/liballoc/vec.rs @@ -963,14 +963,15 @@ impl Vec { #[inline] #[stable(feature = "rust1", since = "1.0.0")] pub fn swap_remove(&mut self, index: usize) -> T { - assert!(index < self.len); + let len = self.len(); + assert!(index < len); unsafe { // We replace self[index] with the last element. Note that if the // bounds check above succeeds there must be a last element (which // can be self[index] itself). - let last = ptr::read(self.as_ptr().add(self.len - 1)); + let last = ptr::read(self.as_ptr().add(len - 1)); let hole: *mut T = self.as_mut_ptr().add(index); - self.len -= 1; + self.set_len(len - 1); ptr::replace(hole, last) } }