Merge #280

280: Fix undefined behavior identified by Miri r=japaric a=jgallagher

Hi! We ran into an exception triggered by new undefined behavior checks inserted into the nightly compiler (https://github.com/rust-lang/rust/pull/92686/files#diff-54110dcedc5a4d976321aa5d2a6767ac0744a3ef1363b75ffc62faf81cf14c30R230-L229). Running `heapless`'s test suite under Miri didn't flag anything at first, but it did once we added `MIRIFLAGS="-Zmiri-tag-raw-pointers"`. All three of the fixes in this PR were identified via

```
MIRIFLAGS="-Zmiri-tag-raw-pointers -Zmiri-ignore-leaks" cargo +nightly miri test -- --skip pool::
```

and the fixes came from copying the implementations from the equivalent methods in `std`. Note that I skipped the `pool::` tests; there is at least one miri failure in them, but it wasn't immediately obvious how to fix it so I skipped it for now. It's probably worth adding the flag above to the CI miri run, but I didn't do that either (since it would immediately cause failures given I didn't fix the problem in `pool`).

The specific output for `pool` is

```
test pool::singleton::tests::sanity ... error: Undefined Behavior: trying to reborrow <untagged> for SharedReadWrite permission at alloc36[0x1], but that tag does not exist in the borrow stack for this location
   --> /home/john/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/non_null.rs:380:18
    |
380 |         unsafe { &*self.as_ptr() }
    |                  ^^^^^^^^^^^^^^^
    |                  |
    |                  trying to reborrow <untagged> for SharedReadWrite permission at alloc36[0x1], but that tag does not exist in the borrow stack for this location
    |                  this error occurs as part of a reborrow at alloc36[0x1..0x9]
    |
    = help: this indicates a potential bug in the program: it performed an invalid operation, but the rules it violated are still experimental
    = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information

    = note: inside `std::ptr::NonNull::<pool::stack::Node<u8>>::as_ref` at /home/john/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/non_null.rs:380:18
note: inside `pool::stack::Stack::<u8>::push` at src/pool/cas.rs:43:17
   --> src/pool/cas.rs:43:17
    |
43  | /                 new_head
44  | |                     .as_raw()
45  | |                     .as_ref()
    | |_____________________________^
note: inside `pool::Pool::<u8>::grow` at src/pool/mod.rs:390:25
   --> src/pool/mod.rs:390:25
    |
390 |                         self.stack.push(p);
    |                         ^^^^^^^^^^^^^^^^^^
note: inside `<pool::singleton::tests::sanity::A as pool::singleton::Pool>::grow` at src/pool/singleton.rs:78:9
   --> src/pool/singleton.rs:78:9
    |
78  |         Self::ptr().grow(memory)
    |         ^^^^^^^^^^^^^^^^^^^^^^^^
note: inside `pool::singleton::tests::sanity` at src/pool/singleton.rs:362:9
   --> src/pool/singleton.rs:362:9
    |
362 |         A::grow(unsafe { &mut MEMORY });
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside closure at src/pool/singleton.rs:353:5
   --> src/pool/singleton.rs:353:5
    |
352 |       #[test]
    |       ------- in this procedural macro expansion
353 | /     fn sanity() {
354 | |         const SZ: usize = 2 * mem::size_of::<Node<u8>>() - 1;
355 | |         static mut MEMORY: [u8; SZ] = [0; SZ];
356 | |
...   |
373 | |         assert_eq!(*A::alloc().unwrap().init(1), 1);
374 | |     }
    | |_____^
    = note: this error originates in the attribute macro `test` (in Nightly builds, run with -Z macro-backtrace for more info)
```

Co-authored-by: John Gallagher <john@oxidecomputer.com>

CHANGELOG.md

@@ -10,6 +10,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 ### Fixed
 
 * Fixed `pool` example in docstring.
+* Fixed undefined behavior in `Vec::truncate()`, `Vec::swap_remove_unchecked()`,
+  and `Hole::move_to()` (internal to the binary heap implementation).
 
 ### Added
 

src/binary_heap.rs

@@ -428,8 +428,9 @@ impl<'a, T> Hole<'a, T> {
     unsafe fn move_to(&mut self, index: usize) {
         debug_assert!(index != self.pos);
         debug_assert!(index < self.data.len());
-        let index_ptr: *const _ = self.data.get_unchecked(index);
+        let ptr = self.data.as_mut_ptr();
-        let hole_ptr = self.data.get_unchecked_mut(self.pos);
+        let index_ptr: *const _ = ptr.add(index);
+        let hole_ptr = ptr.add(self.pos);
         ptr::copy_nonoverlapping(index_ptr, hole_ptr, 1);
         self.pos = index;
     }

src/vec.rs

@@ -263,13 +263,24 @@ impl<T, const N: usize> Vec<T, N> {
 
     /// Shortens the vector, keeping the first `len` elements and dropping the rest.
     pub fn truncate(&mut self, len: usize) {
-        // drop any extra elements
+        // This is safe because:
-        while len < self.len {
+        //
-            // decrement len before the drop_in_place(), so a panic on Drop
+        // * the slice passed to `drop_in_place` is valid; the `len > self.len`
-            // doesn't re-drop the just-failed value.
+        //   case avoids creating an invalid slice, and
-            self.len -= 1;
+        // * the `len` of the vector is shrunk before calling `drop_in_place`,
-            let len = self.len;
+        //   such that no value will be dropped twice in case `drop_in_place`
-            unsafe { ptr::drop_in_place(self.as_mut_slice().get_unchecked_mut(len)) };
+        //   were to panic once (if it panics twice, the program aborts).
+        unsafe {
+            // Note: It's intentional that this is `>` and not `>=`.
+            //       Changing it to `>=` has negative performance
+            //       implications in some cases. See rust-lang/rust#78884 for more.
+            if len > self.len {
+                return;
+            }
+            let remaining_len = self.len - len;
+            let s = ptr::slice_from_raw_parts_mut(self.as_mut_ptr().add(len), remaining_len);
+            self.len = len;
+            ptr::drop_in_place(s);
         }
     }
 
@@ -471,11 +482,11 @@ impl<T, const N: usize> Vec<T, N> {
     pub unsafe fn swap_remove_unchecked(&mut self, index: usize) -> T {
         let length = self.len();
         debug_assert!(index < length);
-        ptr::swap(
+        let value = ptr::read(self.as_ptr().add(index));
-            self.as_mut_slice().get_unchecked_mut(index),
+        let base_ptr = self.as_mut_ptr();
-            self.as_mut_slice().get_unchecked_mut(length - 1),
+        ptr::copy(base_ptr.add(length - 1), base_ptr.add(index), 1);
-        );
+        self.len -= 1;
-        self.pop_unchecked()
+        value
     }
 
     /// Returns true if the vec is full