diff --git a/crates/wasi-common/src/path.rs b/crates/wasi-common/src/path.rs index f5399a7b7e..fdce7b481a 100644 --- a/crates/wasi-common/src/path.rs +++ b/crates/wasi-common/src/path.rs @@ -3,7 +3,7 @@ use crate::handle::{Handle, HandleRights}; use crate::wasi::{types, Errno, Result}; use std::path::{Component, Path}; use std::str; -use wiggle::{GuestBorrows, GuestPtr}; +use wiggle::GuestPtr; pub(crate) use crate::sys::path::{from_host, open_rights}; @@ -14,19 +14,15 @@ pub(crate) fn get( entry: &Entry, required_rights: &HandleRights, dirflags: types::Lookupflags, - path: &GuestPtr<'_, str>, + path_ptr: &GuestPtr<'_, str>, needs_final_component: bool, ) -> Result<(Box, String)> { const MAX_SYMLINK_EXPANSIONS: usize = 128; // Extract path as &str from guest's memory. - let path = unsafe { - let mut bc = GuestBorrows::new(); - let raw = path.as_raw(&mut bc)?; - &*raw - }; + let path = path_ptr.as_str()?; - log::trace!(" | (path_ptr,path_len)='{}'", path); + log::trace!(" | (path_ptr,path_len)='{}'", &*path); if path.contains('\0') { // if contains NUL, return Ilseq diff --git a/crates/wasi-common/src/snapshots/wasi_snapshot_preview1.rs b/crates/wasi-common/src/snapshots/wasi_snapshot_preview1.rs index 31528beb39..49ae8270e0 100644 --- a/crates/wasi-common/src/snapshots/wasi_snapshot_preview1.rs +++ b/crates/wasi-common/src/snapshots/wasi_snapshot_preview1.rs @@ -8,7 +8,7 @@ use crate::{path, poll}; use log::{debug, error, trace}; use std::convert::TryInto; use std::io::{self, SeekFrom}; -use wiggle::{GuestBorrows, GuestPtr}; +use wiggle::{GuestPtr, GuestSlice}; impl<'a> WasiSnapshotPreview1 for WasiCtx { fn args_get<'b>( @@ -199,18 +199,11 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { iovs: &types::IovecArray<'_>, offset: types::Filesize, ) -> Result { - let mut buf = Vec::new(); - let mut bc = GuestBorrows::new(); - bc.borrow_slice(iovs)?; + let mut guest_slices: Vec> = Vec::new(); for iov_ptr in iovs.iter() { let iov_ptr = iov_ptr?; let iov: types::Iovec = iov_ptr.read()?; - let slice = unsafe { - let buf = iov.buf.as_array(iov.buf_len); - let raw = buf.as_raw(&mut bc)?; - &mut *raw - }; - buf.push(io::IoSliceMut::new(slice)); + guest_slices.push(iov.buf.as_array(iov.buf_len).as_slice()?); } let required_rights = @@ -219,10 +212,18 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { if offset > i64::max_value() as u64 { return Err(Errno::Io); } - let host_nread = entry - .as_handle(&required_rights)? - .preadv(&mut buf, offset)? - .try_into()?; + + let host_nread = { + let mut buf = guest_slices + .iter_mut() + .map(|s| io::IoSliceMut::new(&mut *s)) + .collect::>>(); + entry + .as_handle(&required_rights)? + .preadv(&mut buf, offset)? + .try_into()? + }; + drop(guest_slices); Ok(host_nread) } @@ -275,18 +276,11 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { ciovs: &types::CiovecArray<'_>, offset: types::Filesize, ) -> Result { - let mut buf = Vec::new(); - let mut bc = GuestBorrows::new(); - bc.borrow_slice(ciovs)?; + let mut guest_slices = Vec::new(); for ciov_ptr in ciovs.iter() { let ciov_ptr = ciov_ptr?; let ciov: types::Ciovec = ciov_ptr.read()?; - let slice = unsafe { - let buf = ciov.buf.as_array(ciov.buf_len); - let raw = buf.as_raw(&mut bc)?; - &*raw - }; - buf.push(io::IoSlice::new(slice)); + guest_slices.push(ciov.buf.as_array(ciov.buf_len).as_slice()?); } let required_rights = @@ -297,34 +291,37 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { return Err(Errno::Io); } - let host_nwritten = entry - .as_handle(&required_rights)? - .pwritev(&buf, offset)? - .try_into()?; + let host_nwritten = { + let buf: Vec = + guest_slices.iter().map(|s| io::IoSlice::new(&*s)).collect(); + entry + .as_handle(&required_rights)? + .pwritev(&buf, offset)? + .try_into()? + }; Ok(host_nwritten) } fn fd_read(&self, fd: types::Fd, iovs: &types::IovecArray<'_>) -> Result { - let mut bc = GuestBorrows::new(); - let mut slices = Vec::new(); - bc.borrow_slice(&iovs)?; + let mut guest_slices = Vec::new(); for iov_ptr in iovs.iter() { let iov_ptr = iov_ptr?; let iov: types::Iovec = iov_ptr.read()?; - let slice = unsafe { - let buf = iov.buf.as_array(iov.buf_len); - let raw = buf.as_raw(&mut bc)?; - &mut *raw - }; - slices.push(io::IoSliceMut::new(slice)); + guest_slices.push(iov.buf.as_array(iov.buf_len).as_slice()?); } let required_rights = HandleRights::from_base(types::Rights::FD_READ); let entry = self.get_entry(fd)?; - let host_nread = entry - .as_handle(&required_rights)? - .read_vectored(&mut slices)? - .try_into()?; + let host_nread = { + let mut slices: Vec = guest_slices + .iter_mut() + .map(|s| io::IoSliceMut::new(&mut *s)) + .collect(); + entry + .as_handle(&required_rights)? + .read_vectored(&mut slices)? + .try_into()? + }; Ok(host_nread) } @@ -423,25 +420,22 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { } fn fd_write(&self, fd: types::Fd, ciovs: &types::CiovecArray<'_>) -> Result { - let mut bc = GuestBorrows::new(); - let mut slices = Vec::new(); - bc.borrow_slice(&ciovs)?; + let mut guest_slices = Vec::new(); for ciov_ptr in ciovs.iter() { let ciov_ptr = ciov_ptr?; let ciov: types::Ciovec = ciov_ptr.read()?; - let slice = unsafe { - let buf = ciov.buf.as_array(ciov.buf_len); - let raw = buf.as_raw(&mut bc)?; - &*raw - }; - slices.push(io::IoSlice::new(slice)); + guest_slices.push(ciov.buf.as_array(ciov.buf_len).as_slice()?); } let required_rights = HandleRights::from_base(types::Rights::FD_WRITE); let entry = self.get_entry(fd)?; - let host_nwritten = entry - .as_handle(&required_rights)? - .write_vectored(&slices)? - .try_into()?; + let host_nwritten = { + let slices: Vec = + guest_slices.iter().map(|s| io::IoSlice::new(&*s)).collect(); + entry + .as_handle(&required_rights)? + .write_vectored(&slices)? + .try_into()? + }; Ok(host_nwritten) } @@ -596,13 +590,8 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { path, false, )?; - let slice = unsafe { - let mut bc = GuestBorrows::new(); - let buf = buf.as_array(buf_len); - let raw = buf.as_raw(&mut bc)?; - &mut *raw - }; - let host_bufused = dirfd.readlink(&path, slice)?.try_into()?; + let mut slice = buf.as_array(buf_len).as_slice()?; + let host_bufused = dirfd.readlink(&path, &mut *slice)?.try_into()?; Ok(host_bufused) } @@ -662,12 +651,8 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { new_path, true, )?; - let old_path = unsafe { - let mut bc = GuestBorrows::new(); - let raw = old_path.as_raw(&mut bc)?; - &*raw - }; - trace!(" | old_path='{}'", old_path); + let old_path = old_path.as_str()?; + trace!(" | old_path='{}'", &*old_path); new_fd.symlink(&old_path, &new_path) } @@ -696,9 +681,7 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { } let mut subscriptions = Vec::new(); - let mut bc = GuestBorrows::new(); let subs = in_.as_array(nsubscriptions); - bc.borrow_slice(&subs)?; for sub_ptr in subs.iter() { let sub_ptr = sub_ptr?; let sub: types::Subscription = sub_ptr.read()?; @@ -793,7 +776,6 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { let nevents = events.len().try_into()?; let out_events = out.as_array(nevents); - bc.borrow_slice(&out_events)?; for (event, event_ptr) in events.into_iter().zip(out_events.iter()) { let event_ptr = event_ptr?; event_ptr.write(event)?; @@ -820,13 +802,8 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { } fn random_get(&self, buf: &GuestPtr, buf_len: types::Size) -> Result<()> { - let slice = unsafe { - let mut bc = GuestBorrows::new(); - let buf = buf.as_array(buf_len); - let raw = buf.as_raw(&mut bc)?; - &mut *raw - }; - getrandom::getrandom(slice).map_err(|err| { + let mut slice = buf.as_array(buf_len).as_slice()?; + getrandom::getrandom(&mut *slice).map_err(|err| { error!("getrandom failure: {:?}", err); Errno::Io }) diff --git a/crates/wasi-common/src/wasi.rs b/crates/wasi-common/src/wasi.rs index 53dbd5476b..f35055ffa8 100644 --- a/crates/wasi-common/src/wasi.rs +++ b/crates/wasi-common/src/wasi.rs @@ -40,6 +40,7 @@ impl From for Errno { InFunc { .. } => Self::Inval, InDataField { .. } => Self::Inval, SliceLengthsDiffer { .. } => Self::Fault, + BorrowCheckerOutOfHandles { .. } => Self::Fault, } } } diff --git a/crates/wasi-common/wig/src/wasi.rs b/crates/wasi-common/wig/src/wasi.rs index 39e263c7a6..579e6f3ca1 100644 --- a/crates/wasi-common/wig/src/wasi.rs +++ b/crates/wasi-common/wig/src/wasi.rs @@ -467,7 +467,13 @@ pub fn define_struct_for_wiggle(args: TokenStream) -> TokenStream { #handle_early_error } }; - let mem: WasiMemory = mem.into(); + // Wiggle does not expose any methods for + // functions to re-enter the WebAssembly module, + // or expose the memory via non-wiggle mechanisms. + // Therefore, creating a new BorrowChecker at the + // root of each function invocation is correct. + let bc = wiggle::BorrowChecker::new(); + let mem = WasiMemory { mem, bc }; wasi_common::wasi::#module_id::#name_ident( &mut my_cx.borrow_mut(), &mem, @@ -484,17 +490,17 @@ pub fn define_struct_for_wiggle(args: TokenStream) -> TokenStream { /// Lightweight `wasmtime::Memory` wrapper so that we can /// implement `wiggle::GuestMemory` trait on it which is /// now required to interface with `wasi-common`. - struct WasiMemory(wasmtime::Memory); - - impl From for WasiMemory { - fn from(mem: wasmtime::Memory) -> Self { - Self(mem) - } + struct WasiMemory { + mem: wasmtime::Memory, + bc: wiggle::BorrowChecker, } unsafe impl wiggle::GuestMemory for WasiMemory { fn base(&self) -> (*mut u8, u32) { - (self.0.data_ptr(), self.0.data_size() as _) + (self.mem.data_ptr(), self.mem.data_size() as _) + } + fn borrow_checker(&self) -> &wiggle::BorrowChecker { + &self.bc } } diff --git a/crates/wiggle/generate/src/funcs.rs b/crates/wiggle/generate/src/funcs.rs index 6bb5beed40..690ced5512 100644 --- a/crates/wiggle/generate/src/funcs.rs +++ b/crates/wiggle/generate/src/funcs.rs @@ -24,7 +24,8 @@ pub fn define_func( }); let abi_args = quote!( - ctx: &#ctx_type, memory: &dyn #rt::GuestMemory, + ctx: &#ctx_type, + memory: &dyn #rt::GuestMemory, #(#params),* ); let abi_ret = if let Some(ret) = &coretype.ret { diff --git a/crates/wiggle/src/borrow.rs b/crates/wiggle/src/borrow.rs index 8d5d81c01a..83d96c3448 100644 --- a/crates/wiggle/src/borrow.rs +++ b/crates/wiggle/src/borrow.rs @@ -1,64 +1,108 @@ +use crate::error::GuestError; use crate::region::Region; -use crate::{GuestError, GuestPtr, GuestType}; +use std::cell::RefCell; +use std::collections::HashMap; -#[derive(Debug)] -pub struct GuestBorrows { - borrows: Vec, +#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)] +pub struct BorrowHandle(usize); + +pub struct BorrowChecker { + bc: RefCell, } -impl GuestBorrows { - pub fn new() -> Self { - Self { - borrows: Vec::new(), +impl BorrowChecker { + /// A `BorrowChecker` manages run-time validation of borrows from a `GuestMemory`. It keeps + /// track of regions of guest memory which are possible to alias with Rust references (via the + /// `GuestSlice` and `GuestStr` structs, which implement `std::ops::Deref` and + /// `std::ops::DerefMut`. It also enforces that `GuestPtr::read` and `GuestPtr::write` do not + /// access memory with an outstanding borrow. + /// The safety of this mechanism depends on creating exactly one `BorrowChecker` per + /// WebAssembly memory. There must be no other reads or writes of WebAssembly the memory by + /// either Rust or WebAssembly code while there are any outstanding borrows, as given by + /// `BorrowChecker::has_outstanding_borrows()`. + pub unsafe fn new() -> Self { + BorrowChecker { + bc: RefCell::new(InnerBorrowChecker::new()), } } + /// Indicates whether any outstanding borrows are known to the `BorrowChecker`. This function + /// must be `false` in order for it to be safe to recursively call into a WebAssembly module, + /// or to manipulate the WebAssembly memory by any other means. + pub fn has_outstanding_borrows(&self) -> bool { + self.bc.borrow().has_outstanding_borrows() + } + + pub(crate) fn borrow(&self, r: Region) -> Result { + self.bc.borrow_mut().borrow(r) + } + pub(crate) fn unborrow(&self, h: BorrowHandle) { + self.bc.borrow_mut().unborrow(h) + } + pub(crate) fn is_borrowed(&self, r: Region) -> bool { + self.bc.borrow().is_borrowed(r) + } +} + +#[derive(Debug)] +/// This is a pretty naive way to account for borrows. This datastructure +/// could be made a lot more efficient with some effort. +struct InnerBorrowChecker { + /// Map from handle to region borrowed. A HashMap is probably not ideal + /// for this but it works. It would be more efficient if we could + /// check `is_borrowed` without an O(n) iteration, by organizing borrows + /// by an ordering of Region. + borrows: HashMap, + /// Handle to give out for the next borrow. This is the bare minimum of + /// bookkeeping of free handles, and in a pathological case we could run + /// out, hence [`GuestError::BorrowCheckerOutOfHandles`] + next_handle: BorrowHandle, +} + +impl InnerBorrowChecker { + fn new() -> Self { + InnerBorrowChecker { + borrows: HashMap::new(), + next_handle: BorrowHandle(0), + } + } + + fn has_outstanding_borrows(&self) -> bool { + !self.borrows.is_empty() + } fn is_borrowed(&self, r: Region) -> bool { - !self.borrows.iter().all(|b| !b.overlaps(r)) + !self.borrows.values().all(|b| !b.overlaps(r)) } - pub(crate) fn borrow(&mut self, r: Region) -> Result<(), GuestError> { - if self.is_borrowed(r) { - Err(GuestError::PtrBorrowed(r)) - } else { - self.borrows.push(r); - Ok(()) + fn new_handle(&mut self) -> Result { + // Reset handles to 0 if all handles have been returned. + if self.borrows.is_empty() { + self.next_handle = BorrowHandle(0); } + let h = self.next_handle; + // Get the next handle. Since we don't recycle handles until all of + // them have been returned, there is a pathological case where a user + // may make a Very Large (usize::MAX) number of valid borrows and + // unborrows while always keeping at least one borrow outstanding, and + // we will run out of borrow handles. + self.next_handle = BorrowHandle( + h.0.checked_add(1) + .ok_or_else(|| GuestError::BorrowCheckerOutOfHandles)?, + ); + Ok(h) } - /// Borrow the region of memory pointed to by a `GuestPtr`. This is required for safety if - /// you are dereferencing `GuestPtr`s while holding a reference to a slice via - /// `GuestPtr::as_raw`. - pub fn borrow_pointee<'a, T>(&mut self, p: &GuestPtr<'a, T>) -> Result<(), GuestError> - where - T: GuestType<'a>, - { - self.borrow(Region { - start: p.offset(), - len: T::guest_size(), - }) + fn borrow(&mut self, r: Region) -> Result { + if self.is_borrowed(r) { + return Err(GuestError::PtrBorrowed(r)); + } + let h = self.new_handle()?; + self.borrows.insert(h, r); + Ok(h) } - /// Borrow the slice of memory pointed to by a `GuestPtr<[T]>`. This is required for safety if - /// you are dereferencing the `GuestPtr`s while holding a reference to another slice via - /// `GuestPtr::as_raw`. Not required if using `GuestPtr::as_raw` on this pointer. - pub fn borrow_slice<'a, T>(&mut self, p: &GuestPtr<'a, [T]>) -> Result<(), GuestError> - where - T: GuestType<'a>, - { - let (start, elems) = p.offset(); - let len = T::guest_size() - .checked_mul(elems) - .ok_or_else(|| GuestError::PtrOverflow)?; - self.borrow(Region { start, len }) - } - - /// Borrow the slice of memory pointed to by a `GuestPtr`. This is required for safety if - /// you are dereferencing the `GuestPtr`s while holding a reference to another slice via - /// `GuestPtr::as_raw`. Not required if using `GuestPtr::as_raw` on this pointer. - pub fn borrow_str(&mut self, p: &GuestPtr) -> Result<(), GuestError> { - let (start, len) = p.offset(); - self.borrow(Region { start, len }) + fn unborrow(&mut self, h: BorrowHandle) { + let _ = self.borrows.remove(&h); } } @@ -67,14 +111,14 @@ mod test { use super::*; #[test] fn nonoverlapping() { - let mut bs = GuestBorrows::new(); + let mut bs = InnerBorrowChecker::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 mut bs = InnerBorrowChecker::new(); let r1 = Region::new(10, 10); let r2 = Region::new(0, 10); assert!(!r1.overlaps(r2)); @@ -84,35 +128,35 @@ mod test { #[test] fn overlapping() { - let mut bs = GuestBorrows::new(); + let mut bs = InnerBorrowChecker::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 mut bs = InnerBorrowChecker::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 mut bs = InnerBorrowChecker::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 mut bs = InnerBorrowChecker::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 mut bs = InnerBorrowChecker::new(); let r1 = Region::new(2, 5); let r2 = Region::new(10, 5); let r3 = Region::new(15, 5); @@ -123,4 +167,30 @@ mod test { bs.borrow(r3).expect("can borrow r3"); assert!(bs.borrow(r4).is_err(), "cant borrow r4"); } + + #[test] + fn unborrowing() { + let mut bs = InnerBorrowChecker::new(); + let r1 = Region::new(0, 10); + let r2 = Region::new(10, 10); + assert!(!r1.overlaps(r2)); + assert_eq!(bs.has_outstanding_borrows(), false, "start with no borrows"); + let h1 = bs.borrow(r1).expect("can borrow r1"); + assert_eq!(bs.has_outstanding_borrows(), true, "h1 is outstanding"); + let h2 = bs.borrow(r2).expect("can borrow r2"); + + assert!(bs.borrow(r2).is_err(), "can't borrow r2 twice"); + bs.unborrow(h2); + assert_eq!( + bs.has_outstanding_borrows(), + true, + "h1 is still outstanding" + ); + bs.unborrow(h1); + assert_eq!(bs.has_outstanding_borrows(), false, "no remaining borrows"); + + let _h3 = bs + .borrow(r2) + .expect("can borrow r2 again now that its been unborrowed"); + } } diff --git a/crates/wiggle/src/error.rs b/crates/wiggle/src/error.rs index a7bb43fa72..5cfab66675 100644 --- a/crates/wiggle/src/error.rs +++ b/crates/wiggle/src/error.rs @@ -15,6 +15,8 @@ pub enum GuestError { PtrNotAligned(Region, u32), #[error("Pointer already borrowed: {0:?}")] PtrBorrowed(Region), + #[error("Borrow checker out of handles")] + BorrowCheckerOutOfHandles, #[error("Slice length mismatch")] SliceLengthsDiffer, #[error("In func {funcname}:{location}:")] diff --git a/crates/wiggle/src/guest_type.rs b/crates/wiggle/src/guest_type.rs index 51e003c7f6..4359d3745f 100644 --- a/crates/wiggle/src/guest_type.rs +++ b/crates/wiggle/src/guest_type.rs @@ -1,4 +1,4 @@ -use crate::{GuestError, GuestPtr}; +use crate::{region::Region, GuestError, GuestPtr}; use std::mem; /// A trait for types which are used to report errors. Each type used in the @@ -74,24 +74,39 @@ macro_rules! primitives { // size of our type as well as properly aligned. Consequently we // should be able to safely ready the pointer just after we // validated it, returning it along here. + let offset = ptr.offset(); + let size = Self::guest_size(); let host_ptr = ptr.mem().validate_size_align( - ptr.offset(), + offset, Self::guest_align(), - Self::guest_size(), + size, )?; + let region = Region { + start: offset, + len: size, + }; + if ptr.borrow_checker().is_borrowed(region) { + return Err(GuestError::PtrBorrowed(region)); + } Ok(unsafe { *host_ptr.cast::() }) } #[inline] fn write(ptr: &GuestPtr<'_, Self>, val: Self) -> Result<(), GuestError> { + let offset = ptr.offset(); + let size = Self::guest_size(); let host_ptr = ptr.mem().validate_size_align( - ptr.offset(), + offset, Self::guest_align(), - Self::guest_size(), + size, )?; - // Similar to above `as_raw` will do a lot of validation, and - // then afterwards we can safely write our value into the - // memory location. + let region = Region { + start: offset, + len: size, + }; + if ptr.borrow_checker().is_borrowed(region) { + return Err(GuestError::PtrBorrowed(region)); + } unsafe { *host_ptr.cast::() = val; } diff --git a/crates/wiggle/src/lib.rs b/crates/wiggle/src/lib.rs index 2f7d6c7773..000080ad86 100644 --- a/crates/wiggle/src/lib.rs +++ b/crates/wiggle/src/lib.rs @@ -16,7 +16,8 @@ mod error; mod guest_type; mod region; -pub use borrow::GuestBorrows; +pub use borrow::BorrowChecker; +use borrow::BorrowHandle; pub use error::GuestError; pub use guest_type::{GuestErrorType, GuestType, GuestTypeTransparent}; pub use region::Region; @@ -52,37 +53,24 @@ pub use region::Region; /// must be "somehow nonzero in length" to allow users of `GuestMemory` and /// `GuestPtr` to safely read and write interior data. /// -/// # Using Raw Pointers /// -/// Methods like [`GuestMemory::base`] or [`GuestPtr::as_raw`] will return raw -/// pointers to use. Returning raw pointers is significant because it shows -/// there are hazards with using the returned pointers, and they can't blanket -/// be used in a safe fashion. It is possible to use these pointers safely, but -/// any usage needs to uphold a few guarantees. +/// # Using References /// -/// * Whenever a `*mut T` is accessed or modified, it must be guaranteed that -/// since the pointer was originally obtained the guest memory wasn't -/// relocated in any way. This means you can't call back into the guest, call -/// other arbitrary functions which might call into the guest, etc. The -/// problem here is that the guest could execute instructions like -/// `memory.grow` which would invalidate the raw pointer. If, however, after -/// you acquire `*mut T` you only execute your own code and it doesn't touch -/// the guest, then `*mut T` is still guaranteed to point to valid code. +/// See the safety guarantees of [`BorrowChecker`], which asserts that exactly +/// one `BorrowChecker` may be constructed for each WebAssembly memory. /// -/// * Furthermore, Rust's aliasing rules must still be upheld. For example you -/// can't have two `&mut T` types that point to the area or overlap in any -/// way. This in particular becomes an issue when you're dealing with multiple -/// `GuestPtr` types. If you want to simultaneously work with them then you -/// need to dynamically validate that you're either working with them all in a -/// shared fashion (e.g. as if they were `&T`) or you must verify that they do -/// not overlap to work with them as `&mut T`. +/// The [`GuestMemory::as_slice`] or [`GuestPtr::as_str`] will return smart +/// pointers [`GuestSlice`] and [`GuestStr`]. These types, which implement +/// [`std::ops::Deref`] and [`std::ops::DerefMut`], provide mutable references +/// into the memory region given by a `GuestMemory`. /// -/// Note that safely using the raw pointers is relatively difficult. This crate -/// strives to provide utilities to safely work with guest pointers so long as -/// the previous guarantees are all upheld. If advanced operations are done with -/// guest pointers it's recommended to be extremely cautious and thoroughly -/// consider possible ramifications with respect to this API before codifying -/// implementation details. +/// These smart pointers are dynamically borrow-checked by the `BorrowChecker` +/// given by [`GuestMemory::borrow_checker()`]. While a `GuestSlice` +/// or a `GuestStr` are live, the [`BorrowChecker::has_outstanding_borrows()`] +/// method will always return `true`. If you need to re-enter the guest or +/// otherwise read or write to the contents of a WebAssembly memory, all +/// `GuestSlice`s and `GuestStr`s for the memory must be dropped, at which +/// point `BorrowChecker::has_outstanding_borrows()` will return `false`. pub unsafe trait GuestMemory { /// Returns the base allocation of this guest memory, located in host /// memory. @@ -96,6 +84,11 @@ pub unsafe trait GuestMemory { /// [`GuestMemory`] documentation. fn base(&self) -> (*mut u8, u32); + /// Gives a reference to the [`BorrowChecker`] used to keep track of each + /// outstanding borrow of the memory region. [`BorrowChecker::new`] safety + /// rules require that exactly one checker exist for each memory region. + fn borrow_checker(&self) -> &BorrowChecker; + /// Validates a guest-relative pointer given various attributes, and returns /// the corresponding host pointer. /// @@ -165,30 +158,45 @@ unsafe impl<'a, T: ?Sized + GuestMemory> GuestMemory for &'a T { fn base(&self) -> (*mut u8, u32) { T::base(self) } + fn borrow_checker(&self) -> &BorrowChecker { + T::borrow_checker(self) + } } unsafe impl<'a, T: ?Sized + GuestMemory> GuestMemory for &'a mut T { fn base(&self) -> (*mut u8, u32) { T::base(self) } + fn borrow_checker(&self) -> &BorrowChecker { + T::borrow_checker(self) + } } unsafe impl GuestMemory for Box { fn base(&self) -> (*mut u8, u32) { T::base(self) } + fn borrow_checker(&self) -> &BorrowChecker { + T::borrow_checker(self) + } } unsafe impl GuestMemory for Rc { fn base(&self) -> (*mut u8, u32) { T::base(self) } + fn borrow_checker(&self) -> &BorrowChecker { + T::borrow_checker(self) + } } unsafe impl GuestMemory for Arc { fn base(&self) -> (*mut u8, u32) { T::base(self) } + fn borrow_checker(&self) -> &BorrowChecker { + T::borrow_checker(self) + } } /// A *guest* pointer into host memory. @@ -207,8 +215,12 @@ unsafe impl GuestMemory for Arc { /// Note that the type parameter does not need to implement the `Sized` trait, /// so you can implement types such as this: /// -/// * `GuestPtr<'_, str>` - a pointer to a guest string -/// * `GuestPtr<'_, [T]>` - a pointer to a guest array +/// * `GuestPtr<'_, str>` - a pointer to a guest string. Has the method +/// [`GuestPtr::as_str`], which gives a dynamically borrow-checked +/// `GuestStr<'_>`, which `DerefMut`s to a `&mut str`. +/// * `GuestPtr<'_, [T]>` - a pointer to a guest array. Has the method +/// [`GuestPtr::as_slice`], which gives a dynamically borrow-checked +/// `GuestSlice<'_, T>`, which `DerefMut`s to a `&mut [T]`. /// /// Unsized types such as this may have extra methods and won't have methods /// like [`GuestPtr::read`] or [`GuestPtr::write`]. @@ -247,7 +259,7 @@ impl<'a, T: ?Sized + Pointee> GuestPtr<'a, T> { /// Note that for sized types like `u32`, `GuestPtr`, etc, the `pointer` /// vlue is a `u32` offset into guest memory. For slices and strings, /// `pointer` is a `(u32, u32)` offset/length pair. - pub fn new(mem: &'a (dyn GuestMemory + 'a), pointer: T::Pointer) -> GuestPtr<'_, T> { + pub fn new(mem: &'a (dyn GuestMemory + 'a), pointer: T::Pointer) -> GuestPtr<'a, T> { GuestPtr { mem, pointer, @@ -268,6 +280,11 @@ impl<'a, T: ?Sized + Pointee> GuestPtr<'a, T> { self.mem } + /// Returns the borrow checker that this pointer uses + pub fn borrow_checker(&self) -> &'a BorrowChecker { + self.mem.borrow_checker() + } + /// Casts this `GuestPtr` type to a different type. /// /// This is a safe method which is useful for simply reinterpreting the type @@ -386,24 +403,16 @@ 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. + /// Attempts to create a [`GuestSlice<'_, T>`] from this pointer, performing + /// bounds checks and type validation. The `GuestSlice` is a smart pointer + /// that can be used as a `&[T]` or a `&mut [T]` via the `Deref` and `DerefMut` + /// traits. The region of memory backing the slice will be marked as borrowed + /// by the [`BorrowChecker`] until the `GuestSlice` is dropped. /// - /// 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> + /// This function will return a `GuestSlice` into host memory if all checks + /// succeed (valid utf-8, valid pointers, memory is not borrowed, etc). If + /// any checks fail then `GuestError` will be returned. + pub fn as_slice(&self) -> Result, GuestError> where T: GuestTypeTransparent<'a>, { @@ -415,7 +424,7 @@ impl<'a, T> GuestPtr<'a, [T]> { self.mem .validate_size_align(self.pointer.0, T::guest_align(), len)? as *mut T; - bc.borrow(Region { + let borrow = self.mem.borrow_checker().borrow(Region { start: self.pointer.0, len, })?; @@ -426,12 +435,14 @@ impl<'a, T> GuestPtr<'a, [T]> { 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]) - } + // SAFETY: iff there are no overlapping borrows it is valid to construct a &mut [T] + let ptr = unsafe { slice::from_raw_parts_mut(ptr, self.pointer.1 as usize) }; + + Ok(GuestSlice { + ptr, + bc: self.mem.borrow_checker(), + borrow, + }) } /// Copies the data pointed to by `slice` into this guest region. @@ -451,16 +462,14 @@ impl<'a, T> GuestPtr<'a, [T]> { T: GuestTypeTransparent<'a> + Copy, { // bounds check ... - let raw = self.as_raw(&mut GuestBorrows::new())?; - unsafe { - // ... length check ... - if (*raw).len() != slice.len() { - return Err(GuestError::SliceLengthsDiffer); - } - // ... and copy! - (*raw).copy_from_slice(slice); - Ok(()) + let mut self_slice = self.as_slice()?; + // ... length check ... + if self_slice.len() != slice.len() { + return Err(GuestError::SliceLengthsDiffer); } + // ... and copy! + self_slice.copy_from_slice(slice); + Ok(()) } /// Returns a `GuestPtr` pointing to the base of the array for the interior @@ -488,41 +497,36 @@ impl<'a> GuestPtr<'a, str> { GuestPtr::new(self.mem, self.pointer) } - /// 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. + /// Attempts to create a [`GuestStr<'_>`] from this pointer, performing + /// bounds checks and utf-8 checks. The resulting `GuestStr` can be used + /// as a `&str` or `&mut str` via the `Deref` and `DerefMut` traits. The + /// region of memory backing the `str` will be marked as borrowed by the + /// [`BorrowChecker`] until the `GuestStr` is dropped. /// - /// This function will return a raw pointer into host memory if all checks + /// This function will return `GuestStr` 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 str` 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 str, GuestError> { + pub fn as_str(&self) -> Result, GuestError> { let ptr = self .mem .validate_size_align(self.pointer.0, 1, self.pointer.1)?; - bc.borrow(Region { + let borrow = self.mem.borrow_checker().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) { - Ok(s) => Ok(s), - Err(e) => Err(GuestError::InvalidUtf8(e)), - } + // SAFETY: iff there are no overlapping borrows it is ok to construct + // a &mut str. + let ptr = unsafe { slice::from_raw_parts_mut(ptr, self.pointer.1 as usize) }; + // Validate that contents are utf-8: + match str::from_utf8_mut(ptr) { + Ok(ptr) => Ok(GuestStr { + ptr, + bc: self.mem.borrow_checker(), + borrow, + }), + Err(e) => Err(GuestError::InvalidUtf8(e)), } } } @@ -541,6 +545,62 @@ impl fmt::Debug for GuestPtr<'_, T> { } } +/// A smart pointer to a mutable slice in guest memory. +/// Usable as a `&'a [T]` via [`std::ops::Deref`] and as a `&'a mut [T]` via +/// [`std::ops::DerefMut`]. +pub struct GuestSlice<'a, T> { + ptr: &'a mut [T], + bc: &'a BorrowChecker, + borrow: BorrowHandle, +} + +impl<'a, T> std::ops::Deref for GuestSlice<'a, T> { + type Target = [T]; + fn deref(&self) -> &Self::Target { + self.ptr + } +} + +impl<'a, T> std::ops::DerefMut for GuestSlice<'a, T> { + fn deref_mut(&mut self) -> &mut Self::Target { + self.ptr + } +} + +impl<'a, T> Drop for GuestSlice<'a, T> { + fn drop(&mut self) { + self.bc.unborrow(self.borrow) + } +} + +/// A smart pointer to a mutable `str` in guest memory. +/// Usable as a `&'a str` via [`std::ops::Deref`] and as a `&'a mut str` via +/// [`std::ops::DerefMut`]. +pub struct GuestStr<'a> { + ptr: &'a mut str, + bc: &'a BorrowChecker, + borrow: BorrowHandle, +} + +impl<'a> std::ops::Deref for GuestStr<'a> { + type Target = str; + fn deref(&self) -> &Self::Target { + self.ptr + } +} + +impl<'a> std::ops::DerefMut for GuestStr<'a> { + fn deref_mut(&mut self) -> &mut Self::Target { + self.ptr + } +} + +impl<'a> Drop for GuestStr<'a> { + fn drop(&mut self) { + self.bc.unborrow(self.borrow) + } +} + mod private { pub trait Sealed {} impl Sealed for T {} diff --git a/crates/wiggle/test-helpers/src/lib.rs b/crates/wiggle/test-helpers/src/lib.rs index 42719672b2..40d468a435 100644 --- a/crates/wiggle/test-helpers/src/lib.rs +++ b/crates/wiggle/test-helpers/src/lib.rs @@ -1,7 +1,7 @@ use proptest::prelude::*; use std::cell::UnsafeCell; use std::marker; -use wiggle::GuestMemory; +use wiggle::{BorrowChecker, GuestMemory}; #[derive(Debug, Clone)] pub struct MemAreas(Vec); @@ -43,13 +43,21 @@ impl Into> for MemAreas { } #[repr(align(4096))] +struct HostBuffer { + cell: UnsafeCell<[u8; 4096]>, +} + pub struct HostMemory { - buffer: UnsafeCell<[u8; 4096]>, + buffer: HostBuffer, + bc: BorrowChecker, } impl HostMemory { pub fn new() -> Self { HostMemory { - buffer: UnsafeCell::new([0; 4096]), + buffer: HostBuffer { + cell: UnsafeCell::new([0; 4096]), + }, + bc: unsafe { BorrowChecker::new() }, } } @@ -107,10 +115,13 @@ impl HostMemory { unsafe impl GuestMemory for HostMemory { fn base(&self) -> (*mut u8, u32) { unsafe { - let ptr = self.buffer.get(); + let ptr = self.buffer.cell.get(); ((*ptr).as_mut_ptr(), (*ptr).len() as u32) } } + fn borrow_checker(&self) -> &BorrowChecker { + &self.bc + } } #[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord)] diff --git a/crates/wiggle/tests/arrays.rs b/crates/wiggle/tests/arrays.rs index 52d6c4b345..9142e9f01e 100644 --- a/crates/wiggle/tests/arrays.rs +++ b/crates/wiggle/tests/arrays.rs @@ -75,8 +75,8 @@ impl ReduceExcusesExcercise { } pub fn test(&self) { - let mut ctx = WasiCtx::new(); - let mut host_memory = HostMemory::new(); + let ctx = WasiCtx::new(); + let host_memory = HostMemory::new(); // Populate memory with pointers to generated Excuse values for (&excuse, ptr) in self.excuse_values.iter().zip(self.excuse_ptr_locs.iter()) { @@ -98,8 +98,8 @@ impl ReduceExcusesExcercise { } let res = arrays::reduce_excuses( - &mut ctx, - &mut host_memory, + &ctx, + &host_memory, self.array_ptr_loc.ptr as i32, self.excuse_ptr_locs.len() as i32, self.return_ptr_loc.ptr as i32, diff --git a/crates/wiggle/tests/strings.rs b/crates/wiggle/tests/strings.rs index f14920ca75..3546fd1276 100644 --- a/crates/wiggle/tests/strings.rs +++ b/crates/wiggle/tests/strings.rs @@ -1,5 +1,5 @@ use proptest::prelude::*; -use wiggle::{GuestBorrows, GuestMemory, GuestPtr}; +use wiggle::{GuestMemory, GuestPtr}; use wiggle_test::{impl_errno, HostMemory, MemArea, MemAreas, WasiCtx}; wiggle::from_witx!({ @@ -11,12 +11,9 @@ impl_errno!(types::Errno, types::GuestErrorConversion); impl<'a> strings::Strings for WasiCtx<'a> { fn hello_string(&self, a_string: &GuestPtr) -> Result { - let mut bc = GuestBorrows::new(); - let s = a_string.as_raw(&mut bc).expect("should be valid string"); - unsafe { - println!("a_string='{}'", &*s); - Ok((*s).len() as u32) - } + let s = a_string.as_str().expect("should be valid string"); + println!("a_string='{}'", &*s); + Ok(s.len() as u32) } fn multi_string( @@ -25,18 +22,15 @@ impl<'a> strings::Strings for WasiCtx<'a> { b: &GuestPtr, c: &GuestPtr, ) -> Result { - let mut bc = GuestBorrows::new(); - let sa = a.as_raw(&mut bc).expect("A should be valid string"); - let sb = b.as_raw(&mut bc).expect("B should be valid string"); - let sc = c.as_raw(&mut bc).expect("C should be valid string"); - unsafe { - let total_len = (&*sa).len() + (&*sb).len() + (&*sc).len(); - println!( - "len={}, a='{}', b='{}', c='{}'", - total_len, &*sa, &*sb, &*sc - ); - Ok(total_len as u32) - } + let sa = a.as_str().expect("A should be valid string"); + let sb = b.as_str().expect("B should be valid string"); + let sc = c.as_str().expect("C should be valid string"); + let total_len = sa.len() + sb.len() + sc.len(); + println!( + "len={}, a='{}', b='{}', c='{}'", + total_len, &*sa, &*sb, &*sc + ); + Ok(total_len as u32) } } diff --git a/crates/wiggle/tests/wasi.rs b/crates/wiggle/tests/wasi.rs index 5985d4b16b..823d5f84da 100644 --- a/crates/wiggle/tests/wasi.rs +++ b/crates/wiggle/tests/wasi.rs @@ -1,4 +1,4 @@ -use wiggle::{GuestBorrows, GuestError, GuestErrorType, GuestPtr}; +use wiggle::{GuestError, GuestErrorType, GuestPtr, GuestSlice}; use wiggle_test::WasiCtx; // This test file exists to make sure that the entire `wasi.witx` file can be @@ -141,22 +141,25 @@ impl<'a> crate::wasi_snapshot_preview1::WasiSnapshotPreview1 for WasiCtx<'a> { // that we can use the wiggle API to create the datastructures we want // for efficient implementation of this function elsewhere. - let mut bc = GuestBorrows::new(); - let mut slices: Vec<&'_ mut [u8]> = Vec::new(); - // Mark the iov elements as borrowed, to ensure that they does not - // overlap with any of the as_raw regions. - bc.borrow_slice(&iovs).expect("borrow iovec array"); + let mut slices: Vec> = Vec::new(); for iov_ptr in iovs.iter() { let iov_ptr = iov_ptr.expect("iovec element pointer is valid"); + // Borrow checker will make sure the pointee of this read() doesn't overlap with any + // existing borrows: let iov: types::Iovec = iov_ptr.read().expect("read iovec element"); let base: GuestPtr = iov.buf; let len: u32 = iov.buf_len; let buf: GuestPtr<[u8]> = base.as_array(len); - let slice = buf.as_raw(&mut bc).expect("borrow slice from iovec"); - slices.push(unsafe { &mut *slice }); + // GuestSlice will remain borrowed until dropped: + let slice = buf.as_slice().expect("borrow slice from iovec"); + slices.push(slice); } - println!("iovec slices: {:?}", slices); + println!("iovec slices: ["); + for slice in slices { + println!(" {:?},", &*slice); + } + println!("]"); unimplemented!("fd_pread") }