Check safety of as_raw with a simplified borrow checker (#37)

* wiggle-runtime: add as_raw method for [T]

* add trivial borrow checker back in

* integrate runtime borrow checker with as_raw methods

* handle pointer arith overflow correctly in as_raw, create PtrOverflow error

* runtime: add validation back to GuestType

* generate: impl validate for enums, flags, handles, ints

* oops! make validate its own method on trait GuestTypeTransparent

* fix transparent impls for enum, flag, handle, int

* some structs are transparent. fix tests.

* tests: define byte_slice_strat and friends

* wiggle-tests: i believe my allocator is working now

* some type juggling around memset for ease of use

* make GuestTypeTransparent an unsafe trait

* delete redundant validation of pointer align

* fix doc

* wiggle_test: aha, you cant use sets to track memory areas

* add multi-string test

which exercises the runtime borrow checker against
HostMemory::byte_slice_strat

* oops left debug panic in

* remove redundant (& incorrect, since unchecked) length calc

* redesign validate again, and actually hook to as_raw

* makr all validate impls as inline

this should hopefully allow as_raw's check loop to be unrolled to a
no-op in most cases!

* code review fixes

crates/runtime/src/borrow.rs

@@ -0,0 +1,91 @@
+use crate::region::Region;
+use crate::GuestError;
+
+#[derive(Debug)]
+pub struct GuestBorrows {
+    borrows: Vec<Region>,
+}
+
+impl GuestBorrows {
+    pub fn new() -> Self {
+        Self {
+            borrows: Vec::new(),
+        }
+    }
+
+    fn is_borrowed(&self, r: Region) -> bool {
+        !self.borrows.iter().all(|b| !b.overlaps(r))
+    }
+
+    pub fn borrow(&mut self, r: Region) -> Result<(), GuestError> {
+        if self.is_borrowed(r) {
+            Err(GuestError::PtrBorrowed(r))
+        } else {
+            self.borrows.push(r);
+            Ok(())
+        }
+    }
+}
+
+#[cfg(test)]
+mod test {
+    use super::*;
+    #[test]
+    fn nonoverlapping() {
+        let mut bs = GuestBorrows::new();
+        let r1 = Region::new(0, 10);
+        let r2 = Region::new(10, 10);
+        assert!(!r1.overlaps(r2));
+        bs.borrow(r1).expect("can borrow r1");
+        bs.borrow(r2).expect("can borrow r2");
+
+        let mut bs = GuestBorrows::new();
+        let r1 = Region::new(10, 10);
+        let r2 = Region::new(0, 10);
+        assert!(!r1.overlaps(r2));
+        bs.borrow(r1).expect("can borrow r1");
+        bs.borrow(r2).expect("can borrow r2");
+    }
+
+    #[test]
+    fn overlapping() {
+        let mut bs = GuestBorrows::new();
+        let r1 = Region::new(0, 10);
+        let r2 = Region::new(9, 10);
+        assert!(r1.overlaps(r2));
+        bs.borrow(r1).expect("can borrow r1");
+        assert!(bs.borrow(r2).is_err(), "cant borrow r2");
+
+        let mut bs = GuestBorrows::new();
+        let r1 = Region::new(0, 10);
+        let r2 = Region::new(2, 5);
+        assert!(r1.overlaps(r2));
+        bs.borrow(r1).expect("can borrow r1");
+        assert!(bs.borrow(r2).is_err(), "cant borrow r2");
+
+        let mut bs = GuestBorrows::new();
+        let r1 = Region::new(9, 10);
+        let r2 = Region::new(0, 10);
+        assert!(r1.overlaps(r2));
+        bs.borrow(r1).expect("can borrow r1");
+        assert!(bs.borrow(r2).is_err(), "cant borrow r2");
+
+        let mut bs = GuestBorrows::new();
+        let r1 = Region::new(2, 5);
+        let r2 = Region::new(0, 10);
+        assert!(r1.overlaps(r2));
+        bs.borrow(r1).expect("can borrow r1");
+        assert!(bs.borrow(r2).is_err(), "cant borrow r2");
+
+        let mut bs = GuestBorrows::new();
+        let r1 = Region::new(2, 5);
+        let r2 = Region::new(10, 5);
+        let r3 = Region::new(15, 5);
+        let r4 = Region::new(0, 10);
+        assert!(r1.overlaps(r4));
+        bs.borrow(r1).expect("can borrow r1");
+        bs.borrow(r2).expect("can borrow r2");
+        bs.borrow(r3).expect("can borrow r3");
+        assert!(bs.borrow(r4).is_err(), "cant borrow r4");
+    }
+}

crates/runtime/src/error.rs

@@ -7,6 +7,8 @@ pub enum GuestError {
     InvalidFlagValue(&'static str),
     #[error("Invalid enum value {0}")]
     InvalidEnumValue(&'static str),
+    #[error("Pointer overflow")]
+    PtrOverflow,
     #[error("Pointer out of bounds: {0:?}")]
     PtrOutOfBounds(Region),
     #[error("Pointer not aligned to {1}: {0:?}")]

crates/runtime/src/guest_type.rs

@@ -40,6 +40,22 @@ pub trait GuestType<'a>: Sized {
     fn write(ptr: &GuestPtr<'_, Self>, val: Self) -> Result<(), GuestError>;
 }
 
+/// A trait for `GuestType`s that have the same representation in guest memory
+/// as in Rust. These types can be used with the `GuestPtr::as_raw` method to
+/// view as a slice.
+///
+/// Unsafe trait because a correct GuestTypeTransparent implemengation ensures that the
+/// GuestPtr::as_raw methods are safe. This trait should only ever be implemented
+/// by wiggle_generate-produced code.
+pub unsafe trait GuestTypeTransparent<'a>: GuestType<'a> {
+    /// Checks that the memory at `ptr` is a valid representation of `Self`.
+    ///
+    /// Assumes that memory safety checks have already been performed: `ptr`
+    /// has been checked to be aligned correctly and reside in memory using
+    /// `GuestMemory::validate_size_align`
+    fn validate(ptr: *mut Self) -> Result<(), GuestError>;
+}
+
 macro_rules! primitives {
     ($($i:ident)*) => ($(
         impl<'a> GuestType<'a> for $i {
@@ -78,6 +94,15 @@ macro_rules! primitives {
                 Ok(())
             }
         }
+
+        unsafe impl<'a> GuestTypeTransparent<'a> for $i {
+            #[inline]
+            fn validate(_ptr: *mut $i) -> Result<(), GuestError> {
+                // All bit patterns are safe, nothing to do here
+                Ok(())
+            }
+        }
+
     )*)
 }
 

crates/runtime/src/lib.rs

@@ -6,11 +6,14 @@ use std::slice;
 use std::str;
 use std::sync::Arc;
 
+mod borrow;
 mod error;
 mod guest_type;
 mod region;
+
+pub use borrow::GuestBorrows;
 pub use error::GuestError;
-pub use guest_type::{GuestErrorType, GuestType};
+pub use guest_type::{GuestErrorType, GuestType, GuestTypeTransparent};
 pub use region::Region;
 
 /// A trait which abstracts how to get at the region of host memory taht
@@ -119,12 +122,12 @@ pub unsafe trait GuestMemory {
         // Figure out our pointer to the start of memory
         let start = match (base_ptr as usize).checked_add(offset as usize) {
             Some(ptr) => ptr,
-            None => return Err(GuestError::PtrOutOfBounds(region)),
+            None => return Err(GuestError::PtrOverflow),
         };
         // and use that to figure out the end pointer
         let end = match start.checked_add(len as usize) {
             Some(ptr) => ptr,
-            None => return Err(GuestError::PtrOutOfBounds(region)),
+            None => return Err(GuestError::PtrOverflow),
         };
         // and then verify that our end doesn't reach past the end of our memory
         if end > (base_ptr as usize) + (base_len as usize) {
@@ -335,7 +338,7 @@ impl<'a, T: ?Sized + Pointee> GuestPtr<'a, T> {
             .and_then(|o| self.pointer.checked_add(o));
         let offset = match offset {
             Some(o) => o,
-            None => return Err(GuestError::InvalidFlagValue("")),
+            None => return Err(GuestError::PtrOverflow),
         };
         Ok(GuestPtr::new(self.mem, offset))
     }
@@ -369,6 +372,54 @@ impl<'a, T> GuestPtr<'a, [T]> {
         (0..self.len()).map(move |i| base.add(i))
     }
 
+    /// Attempts to read a raw `*mut [T]` pointer from this pointer, performing
+    /// bounds checks and type validation.
+    /// The resulting `*mut [T]` can be used as a `&mut [t]` as long as the
+    /// reference is dropped before any Wasm code is re-entered.
+    ///
+    /// This function will return a raw pointer into host memory if all checks
+    /// succeed (valid utf-8, valid pointers, etc). If any checks fail then
+    /// `GuestError` will be returned.
+    ///
+    /// Note that the `*mut [T]` pointer is still unsafe to use in general, but
+    /// there are specific situations that it is safe to use. For more
+    /// information about using the raw pointer, consult the [`GuestMemory`]
+    /// trait documentation.
+    ///
+    /// For safety against overlapping mutable borrows, the user must use the
+    /// same `GuestBorrows` to create all *mut str or *mut [T] that are alive
+    /// at the same time.
+    pub fn as_raw(&self, bc: &mut GuestBorrows) -> Result<*mut [T], GuestError>
+    where
+        T: GuestTypeTransparent<'a>,
+    {
+        let len = match self.pointer.1.checked_mul(T::guest_size()) {
+            Some(l) => l,
+            None => return Err(GuestError::PtrOverflow),
+        };
+        let ptr =
+            self.mem
+                .validate_size_align(self.pointer.0, T::guest_align(), len)? as *mut T;
+
+        bc.borrow(Region {
+            start: self.pointer.0,
+            len,
+        })?;
+
+        // Validate all elements in slice.
+        // SAFETY: ptr has been validated by self.mem.validate_size_align
+        for offs in 0..self.pointer.1 {
+            T::validate(unsafe { ptr.add(offs as usize) })?;
+        }
+
+        // SAFETY: iff there are no overlapping borrows (all uses of as_raw use this same
+        // GuestBorrows), its valid to construct a *mut [T]
+        unsafe {
+            let s = slice::from_raw_parts_mut(ptr, self.pointer.1 as usize);
+            Ok(s as *mut [T])
+        }
+    }
+
     /// Returns a `GuestPtr` pointing to the base of the array for the interior
     /// type `T`.
     pub fn as_ptr(&self) -> GuestPtr<'a, T> {
@@ -396,6 +447,8 @@ impl<'a> GuestPtr<'a, str> {
 
     /// Attempts to read a raw `*mut str` pointer from this pointer, performing
     /// bounds checks and utf-8 checks.
+    /// The resulting `*mut str` can be used as a `&mut str` as long as the
+    /// reference is dropped before any Wasm code is re-entered.
     ///
     /// This function will return a raw pointer into host memory if all checks
     /// succeed (valid utf-8, valid pointers, etc). If any checks fail then
@@ -405,12 +458,22 @@ impl<'a> GuestPtr<'a, str> {
     /// there are specific situations that it is safe to use. For more
     /// information about using the raw pointer, consult the [`GuestMemory`]
     /// trait documentation.
-    pub fn as_raw(&self) -> Result<*mut str, GuestError> {
+    ///
+    /// For safety against overlapping mutable borrows, the user must use the
+    /// same `GuestBorrows` to create all *mut str or *mut [T] that are alive
+    /// at the same time.
+    pub fn as_raw(&self, bc: &mut GuestBorrows) -> Result<*mut str, GuestError> {
         let ptr = self
             .mem
             .validate_size_align(self.pointer.0, 1, self.pointer.1)?;
 
-        // TODO: doc unsafety here
+        bc.borrow(Region {
+            start: self.pointer.0,
+            len: self.pointer.1,
+        })?;
+
+        // SAFETY: iff there are no overlapping borrows (all uses of as_raw use this same
+        // GuestBorrows), its valid to construct a *mut str
         unsafe {
             let s = slice::from_raw_parts_mut(ptr, self.pointer.1 as usize);
             match str::from_utf8_mut(s) {