Skip to content

Commit

Permalink
Auto merge of rust-lang#91616 - saethlin:sort_unchecked-sb-fix, r=Mar…
Browse files Browse the repository at this point in the history
…k-Simulacrum

Fix rust-lang#91306 by deriving all access from a single *mut T

Fixes rust-lang#91306.

The previous code is invalid because the first argument to `copy_nonoverlapping` is invalidated by the mutable borrow taken out to construct the second argument.

I believe this patch fixes that, and this code should now pass Miri with `-Ztag-raw-pointers`, ~~but I'm currently stuck trying to run my reproducer with a this patched version of the standard library (alternatively, running Miri on the standard library tests itself would suffice).~~ Ralf walked me through this on Zulip.

I've also added fixes for 7 more problems other than those I reported. Most of them are easy to hit by calling sort_unstable on random arrays. I don't have reproducers for every change, but they seem pretty clear-cut to me. But I did only start learning stacked borrows 2 days ago so that might be a large dash of Dunning-Kruger.
  • Loading branch information
bors committed Dec 16, 2021
2 parents 1d01550 + 3a0fa03 commit 5531927
Showing 1 changed file with 20 additions and 17 deletions.
37 changes: 20 additions & 17 deletions library/core/src/slice/sort.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,8 @@ where
F: FnMut(&T, &T) -> bool,
{
let len = v.len();
// SAFETY: The unsafe operations below involves indexing without a bound check (`get_unchecked` and `get_unchecked_mut`)
// and copying memory (`ptr::copy_nonoverlapping`).
// SAFETY: The unsafe operations below involves indexing without a bounds check (by offsetting a
// pointer) and copying memory (`ptr::copy_nonoverlapping`).
//
// a. Indexing:
// 1. We checked the size of the array to >=2.
Expand All @@ -55,17 +55,18 @@ where
// operation panics, `hole` will get dropped and automatically write the element back
// into the slice.
let mut tmp = mem::ManuallyDrop::new(ptr::read(v.get_unchecked(0)));
let mut hole = CopyOnDrop { src: &mut *tmp, dest: v.get_unchecked_mut(1) };
ptr::copy_nonoverlapping(v.get_unchecked(1), v.get_unchecked_mut(0), 1);
let v = v.as_mut_ptr();
let mut hole = CopyOnDrop { src: &mut *tmp, dest: v.add(1) };
ptr::copy_nonoverlapping(v.add(1), v.add(0), 1);

for i in 2..len {
if !is_less(v.get_unchecked(i), &*tmp) {
if !is_less(&*v.add(i), &*tmp) {
break;
}

// Move `i`-th element one place to the left, thus shifting the hole to the right.
ptr::copy_nonoverlapping(v.get_unchecked(i), v.get_unchecked_mut(i - 1), 1);
hole.dest = v.get_unchecked_mut(i);
ptr::copy_nonoverlapping(v.add(i), v.add(i - 1), 1);
hole.dest = v.add(i);
}
// `hole` gets dropped and thus copies `tmp` into the remaining hole in `v`.
}
Expand All @@ -78,8 +79,8 @@ where
F: FnMut(&T, &T) -> bool,
{
let len = v.len();
// SAFETY: The unsafe operations below involves indexing without a bound check (`get_unchecked` and `get_unchecked_mut`)
// and copying memory (`ptr::copy_nonoverlapping`).
// SAFETY: The unsafe operations below involves indexing without a bound check (by offsetting a
// pointer) and copying memory (`ptr::copy_nonoverlapping`).
//
// a. Indexing:
// 1. We checked the size of the array to >= 2.
Expand All @@ -100,17 +101,18 @@ where
// operation panics, `hole` will get dropped and automatically write the element back
// into the slice.
let mut tmp = mem::ManuallyDrop::new(ptr::read(v.get_unchecked(len - 1)));
let mut hole = CopyOnDrop { src: &mut *tmp, dest: v.get_unchecked_mut(len - 2) };
ptr::copy_nonoverlapping(v.get_unchecked(len - 2), v.get_unchecked_mut(len - 1), 1);
let v = v.as_mut_ptr();
let mut hole = CopyOnDrop { src: &mut *tmp, dest: v.add(len - 2) };
ptr::copy_nonoverlapping(v.add(len - 2), v.add(len - 1), 1);

for i in (0..len - 2).rev() {
if !is_less(&*tmp, v.get_unchecked(i)) {
if !is_less(&*tmp, &*v.add(i)) {
break;
}

// Move `i`-th element one place to the right, thus shifting the hole to the left.
ptr::copy_nonoverlapping(v.get_unchecked(i), v.get_unchecked_mut(i + 1), 1);
hole.dest = v.get_unchecked_mut(i);
ptr::copy_nonoverlapping(v.add(i), v.add(i + 1), 1);
hole.dest = v.add(i);
}
// `hole` gets dropped and thus copies `tmp` into the remaining hole in `v`.
}
Expand Down Expand Up @@ -302,7 +304,7 @@ where
if start_l == end_l {
// Trace `block_l` elements from the left side.
start_l = MaybeUninit::slice_as_mut_ptr(&mut offsets_l);
end_l = MaybeUninit::slice_as_mut_ptr(&mut offsets_l);
end_l = start_l;
let mut elem = l;

for i in 0..block_l {
Expand All @@ -328,7 +330,7 @@ where
if start_r == end_r {
// Trace `block_r` elements from the right side.
start_r = MaybeUninit::slice_as_mut_ptr(&mut offsets_r);
end_r = MaybeUninit::slice_as_mut_ptr(&mut offsets_r);
end_r = start_r;
let mut elem = r;

for i in 0..block_r {
Expand Down Expand Up @@ -579,7 +581,8 @@ where

// Swap the found pair of out-of-order elements.
r -= 1;
ptr::swap(v.get_unchecked_mut(l), v.get_unchecked_mut(r));
let ptr = v.as_mut_ptr();
ptr::swap(ptr.add(l), ptr.add(r));
l += 1;
}
}
Expand Down

0 comments on commit 5531927

Please sign in to comment.