From ac8f8478510d8c913bd17dd377629a24927f20bb Mon Sep 17 00:00:00 2001
From: bstrie <865233+bstrie@users.noreply.github.com>
Date: Tue, 19 Jul 2022 12:40:59 -0400
Subject: [PATCH] fix(sallyport): fix UB by avoiding implicit reference from
 indexing with slice

This potential source of UB was discovered while upgrading the Rust toolchain,
which upgrades us to a new version of Miri with stricter rules around raw pointers.
Specifically, an expression like `addr_of_mut!((*(ptr))[offset..])` is deliberately
attempting to operate only on raw pointers while avoiding any intermediate references,
since references have invariants that raw pointers do not.
However, there is in fact an implicit reference here that is created as a result of the
indexing operation. This is both surprising and not surprising, for interesting reasons.

First, it should not be surprising because indexing is governed by the Index traits,
whose methods function return references, so their presence here is natural.

On the other hand, it is surprising because Rust already special cases `(*ptr)[foo]` when `ptr`
is a raw slice and `foo` is not a range to avoid the Index traits entirely, which allows it to
avoid emitting an intermediate reference.

The ideal solution here is for Rust to be smart enough to not introduce the intermediate
reference here at all, which is tracked at https://github.com/rust-lang/rust/issues/73987 .

In addition, while investigating this issue I brought it up to the Unsafe Code Guidelines team,
who saw fit to file https://github.com/rust-lang/rust/issues/99437 as a more specific example
of the potential perils of the current behavior.
---
 .../sallyport/src/guest/alloc/phase_alloc.rs  | 31 +++++++++++++------
 crates/sallyport/src/lib.rs                   |  1 +
 2 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/crates/sallyport/src/guest/alloc/phase_alloc.rs b/crates/sallyport/src/guest/alloc/phase_alloc.rs
index 634149e1da..b4305a307a 100644
--- a/crates/sallyport/src/guest/alloc/phase_alloc.rs
+++ b/crates/sallyport/src/guest/alloc/phase_alloc.rs
@@ -7,7 +7,6 @@ use crate::Result;
 use core::alloc::Layout;
 use core::marker::PhantomData;
 use core::mem::{align_of, size_of};
-use core::ptr::addr_of_mut;
 use core::ptr::NonNull;
 
 pub(crate) mod phase {
@@ -76,29 +75,41 @@ impl<'a> Alloc<'a, phase::Init> {
 }
 
 impl<'a> Alloc<'a, phase::Stage> {
-    /// Allocates a memory region of `layout.size()` bytes with padding required to ensure alignment
-    /// and returns a tuple of non-null pointer and byte offset of start of that aligned region on success.
+    /// Allocates a memory region of `layout.size()` bytes with padding required to ensure
+    /// alignment and returns a tuple of non-null pointer and byte offset of start of that aligned
+    /// region on success.
     #[inline]
     fn allocate_layout(&mut self, layout: Layout) -> Result<(NonNull<[u8]>, usize)> {
         let free = self.ptr.len();
         let pad_size = self.ptr.cast::<u8>().as_ptr().align_offset(layout.align());
         let layout_size = layout.size();
+
         if free < pad_size.checked_add(layout_size).ok_or(EOVERFLOW)? {
             return Err(ENOMEM);
         }
 
-        let ptr = unsafe { addr_of_mut!((*(self.ptr.as_ptr()))[pad_size..]) };
+        let raw_slice = self.ptr.as_ptr();
+
+        // SAFETY: we know raw_slice is nonnull, and we know pad_size is in-bounds
+        let padded_raw_slice = unsafe { raw_slice.get_unchecked_mut(pad_size..) };
+
+        // SAFETY: we know padded_raw_slice is nonnull, and we know layout_size is in-bounds
+        let new_region = unsafe { padded_raw_slice.get_unchecked_mut(..layout_size) };
+
+        // SAFETY: we know padded_raw_slice is nonnull, and we know layout_size is in-bounds
+        let remainder = unsafe { padded_raw_slice.get_unchecked_mut(layout_size..) };
+
         let offset = self.offset + pad_size;
+
         *self = Self {
-            ptr: unsafe { NonNull::new_unchecked(addr_of_mut!((*(ptr))[layout_size..])) },
+            // SAFETY: we know remainder is nonnull
+            ptr: unsafe { NonNull::new_unchecked(remainder) },
             offset: offset + layout_size,
-
             phase: PhantomData,
         };
-        Ok((
-            unsafe { NonNull::new_unchecked(addr_of_mut!((*(ptr))[..layout_size])) },
-            offset,
-        ))
+
+        // SAFETY: we know new_region is nonnull
+        Ok((unsafe { NonNull::new_unchecked(new_region) }, offset))
     }
 
     #[inline]
diff --git a/crates/sallyport/src/lib.rs b/crates/sallyport/src/lib.rs
index 2eb1994b9c..e412230a0a 100644
--- a/crates/sallyport/src/lib.rs
+++ b/crates/sallyport/src/lib.rs
@@ -11,6 +11,7 @@
 #![feature(core_ffi_c)]
 #![feature(c_size_t)]
 #![feature(nonnull_slice_from_raw_parts)]
+#![feature(slice_ptr_get)]
 #![feature(slice_ptr_len)]
 
 pub mod elf;