diff --git a/src/libcore/slice/sort.rs b/src/libcore/slice/sort.rs index be3e7aaa2e8..8b2ac294764 100644 --- a/src/libcore/slice/sort.rs +++ b/src/libcore/slice/sort.rs @@ -1,6 +1,6 @@ //! Slice sorting //! -//! This module contains an sort algorithm based on Orson Peters' pattern-defeating quicksort, +//! This module contains a sorting algorithm based on Orson Peters' pattern-defeating quicksort, //! published at: https://github.com/orlp/pdqsort //! //! Unstable sorting is compatible with libcore because it doesn't allocate memory, unlike our @@ -20,6 +20,9 @@ struct CopyOnDrop { impl Drop for CopyOnDrop { fn drop(&mut self) { + // SAFETY: This is a helper class. + // Please refer to its usage for correctness. + // Namely, one must be sure that `src` and `dst` does not overlap as required by `ptr::copy_nonoverlapping`. unsafe { ptr::copy_nonoverlapping(self.src, self.dest, 1); } @@ -32,6 +35,21 @@ 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`). + // + // a. Indexing: + // 1. We checked the size of the array to >=2. + // 2. All the indexing that we will do is always between {0 <= index < len} at most. + // + // b. Memory copying + // 1. We are obtaining pointers to references which are guaranteed to be valid. + // 2. They cannot overlap because we obtain pointers to difference indices of the slice. + // Namely, `i` and `i-1`. + // 3. If the slice is properly aligned, the elements are properly aligned. + // It is the caller's responsibility to make sure the slice is properly aligned. + // + // See comments below for further detail. unsafe { // If the first two elements are out-of-order... if len >= 2 && is_less(v.get_unchecked(1), v.get_unchecked(0)) { @@ -62,6 +80,21 @@ 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`). + // + // a. Indexing: + // 1. We checked the size of the array to >= 2. + // 2. All the indexing that we will do is always between `0 <= index < len-1` at most. + // + // b. Memory copying + // 1. We are obtaining pointers to references which are guaranteed to be valid. + // 2. They cannot overlap because we obtain pointers to difference indices of the slice. + // Namely, `i` and `i+1`. + // 3. If the slice is properly aligned, the elements are properly aligned. + // It is the caller's responsibility to make sure the slice is properly aligned. + // + // See comments below for further detail. unsafe { // If the last two elements are out-of-order... if len >= 2 && is_less(v.get_unchecked(len - 1), v.get_unchecked(len - 2)) { @@ -103,6 +136,8 @@ where let mut i = 1; for _ in 0..MAX_STEPS { + // SAFETY: We already explicitly did the bound checking with `i < len`. + // All our subsequent indexing is only in the range `0 <= index < len` unsafe { // Find the next pair of adjacent out-of-order elements. while i < len && !is_less(v.get_unchecked(i), v.get_unchecked(i - 1)) { @@ -220,6 +255,7 @@ where let mut offsets_l = [MaybeUninit::::uninit(); BLOCK]; // The current block on the right side (from `r.sub(block_r)` to `r`). + // SAFETY: The documentation for .add() specifically mention that `vec.as_ptr().add(vec.len())` is always safe` let mut r = unsafe { l.add(v.len()) }; let mut block_r = BLOCK; let mut start_r = ptr::null_mut(); @@ -268,6 +304,16 @@ where let mut elem = l; for i in 0..block_l { + // SAFETY: The unsafety operations below involve the usage of the `offset`. + // According to the conditions required by the function, we satisfy them because: + // 1. `offsets_l` is stack-allocated, and thus considered separate allocated object. + // 2. The function `is_less` returns a `bool`. + // Casting a `bool` will never overflow `isize`. + // 3. We have guaranteed that `block_l` will be `<= BLOCK`. + // Plus, `end_l` was initially set to the begin pointer of `offsets_` which was declared on the stack. + // Thus, we know that even in the worst case (all invocations of `is_less` returns false) we will only be at most 1 byte pass the end. + // Another unsafety operation here is dereferencing `elem`. + // However, `elem` was initially the begin pointer to the slice which is always valid. unsafe { // Branchless comparison. *end_l = i as u8; @@ -284,6 +330,17 @@ where let mut elem = r; for i in 0..block_r { + // SAFETY: The unsafety operations below involve the usage of the `offset`. + // According to the conditions required by the function, we satisfy them because: + // 1. `offsets_r` is stack-allocated, and thus considered separate allocated object. + // 2. The function `is_less` returns a `bool`. + // Casting a `bool` will never overflow `isize`. + // 3. We have guaranteed that `block_r` will be `<= BLOCK`. + // Plus, `end_r` was initially set to the begin pointer of `offsets_` which was declared on the stack. + // Thus, we know that even in the worst case (all invocations of `is_less` returns true) we will only be at most 1 byte pass the end. + // Another unsafety operation here is dereferencing `elem`. + // However, `elem` was initially `1 * sizeof(T)` past the end and we decrement it by `1 * sizeof(T)` before accessing it. + // Plus, `block_r` was asserted to be less than `BLOCK` and `elem` will therefore at most be pointing to the beginning of the slice. unsafe { // Branchless comparison. elem = elem.offset(-1); @@ -404,8 +461,13 @@ where // Find the first pair of out-of-order elements. let mut l = 0; let mut r = v.len(); + + // SAFETY: The unsafety below involves indexing an array. + // For the first one: We already do the bounds checking here with `l < r`. + // For the second one: We initially have `l == 0` and `r == v.len()` and we checked that `l < r` at every indexing operation. + // From here we know that `r` must be at least `r == l` which was shown to be valid from the first one. unsafe { - // Find the first element greater then or equal to the pivot. + // Find the first element greater than or equal to the pivot. while l < r && is_less(v.get_unchecked(l), pivot) { l += 1; } @@ -444,6 +506,7 @@ where // Read the pivot into a stack-allocated variable for efficiency. If a following comparison // operation panics, the pivot will be automatically written back into the slice. + // SAFETY: The pointer here is valid because it is obtained from a reference to a slice. let mut tmp = mem::ManuallyDrop::new(unsafe { ptr::read(pivot) }); let _pivot_guard = CopyOnDrop { src: &mut *tmp, dest: pivot }; let pivot = &*tmp; @@ -452,8 +515,12 @@ where let mut l = 0; let mut r = v.len(); loop { + // SAFETY: The unsafety below involves indexing an array. + // For the first one: We already do the bounds checking here with `l < r`. + // For the second one: We initially have `l == 0` and `r == v.len()` and we checked that `l < r` at every indexing operation. + // From here we know that `r` must be at least `r == l` which was shown to be valid from the first one. unsafe { - // Find the first element greater that the pivot. + // Find the first element greater than the pivot. while l < r && !is_less(pivot, v.get_unchecked(l)) { l += 1; }