From a4319d9915c5025643adb0c5420c65fdf38fe0ca Mon Sep 17 00:00:00 2001 From: Andrew Brown Date: Thu, 10 Nov 2022 07:46:20 -0800 Subject: [PATCH 1/2] wiggle: adapt Wiggle strings for shared use This is an extension of #5229 for the `&str` and `&mut str` types. As documented there, we are attempting to maintain Rust guarantees for slices that Wiggle hands out in the presence of WebAssembly shared memory, in which case multiple threads could be modifying the underlying data of the slice. This change changes the API of `GuestPtr` to return an `Option` which is `None` when attempting to view the WebAssembly data as a string and the underlying WebAssembly memory is shared. This reuses the `UnsafeGuestSlice` structure from #5229 to do so and appropriately marks the region as borrowed in Wiggle's manual borrow checker. Each original call site in this project's WASI implementations is fixed up to `expect` that a non-shared memory is used. (Note that I can find no uses of `GuestStrMut` in the WASI implementations). --- crates/wasi-common/src/snapshots/preview_1.rs | 25 ++-- .../wiggle_interfaces/asymmetric_common.rs | 10 +- .../src/wiggle_interfaces/common.rs | 6 +- .../src/wiggle_interfaces/signatures.rs | 2 +- .../src/wiggle_interfaces/symmetric.rs | 14 +- crates/wiggle/src/lib.rs | 131 +++++++++++------- crates/wiggle/tests/strings.rs | 20 ++- 7 files changed, 124 insertions(+), 84 deletions(-) diff --git a/crates/wasi-common/src/snapshots/preview_1.rs b/crates/wasi-common/src/snapshots/preview_1.rs index 670780673593..cdc4df406fc9 100644 --- a/crates/wasi-common/src/snapshots/preview_1.rs +++ b/crates/wasi-common/src/snapshots/preview_1.rs @@ -803,7 +803,7 @@ impl wasi_snapshot_preview1::WasiSnapshotPreview1 for WasiCtx { self.table() .get_dir(u32::from(dirfd))? .get_cap(DirCaps::CREATE_DIRECTORY)? - .create_dir(path.as_str()?.deref()) + .create_dir(path.as_str()?.expect("cannot use with shared memories; see https://github.com/bytecodealliance/wasmtime/issues/5235 (TODO)").deref()) .await } @@ -818,7 +818,7 @@ impl wasi_snapshot_preview1::WasiSnapshotPreview1 for WasiCtx { .get_dir(u32::from(dirfd))? .get_cap(DirCaps::PATH_FILESTAT_GET)? .get_path_filestat( - path.as_str()?.deref(), + path.as_str()?.expect("cannot use with shared memories; see https://github.com/bytecodealliance/wasmtime/issues/5235 (TODO)").deref(), flags.contains(types::Lookupflags::SYMLINK_FOLLOW), ) .await?; @@ -845,7 +845,7 @@ impl wasi_snapshot_preview1::WasiSnapshotPreview1 for WasiCtx { .get_dir(u32::from(dirfd))? .get_cap(DirCaps::PATH_FILESTAT_SET_TIMES)? .set_times( - path.as_str()?.deref(), + path.as_str()?.expect("cannot use with shared memories; see https://github.com/bytecodealliance/wasmtime/issues/5235 (TODO)").deref(), atim, mtim, flags.contains(types::Lookupflags::SYMLINK_FOLLOW), @@ -876,9 +876,9 @@ impl wasi_snapshot_preview1::WasiSnapshotPreview1 for WasiCtx { src_dir .hard_link( - src_path.as_str()?.deref(), + src_path.as_str()?.expect("cannot use with shared memories; see https://github.com/bytecodealliance/wasmtime/issues/5235 (TODO)").deref(), target_dir.deref(), - target_path.as_str()?.deref(), + target_path.as_str()?.expect("cannot use with shared memories; see https://github.com/bytecodealliance/wasmtime/issues/5235 (TODO)").deref(), ) .await } @@ -904,7 +904,7 @@ impl wasi_snapshot_preview1::WasiSnapshotPreview1 for WasiCtx { let oflags = OFlags::from(&oflags); let fdflags = FdFlags::from(fdflags); - let path = path.as_str()?; + let path = path.as_str()?.expect("cannot use with shared memories; see https://github.com/bytecodealliance/wasmtime/issues/5235 (TODO)"); if oflags.contains(OFlags::DIRECTORY) { if oflags.contains(OFlags::CREATE) || oflags.contains(OFlags::EXCLUSIVE) @@ -953,7 +953,7 @@ impl wasi_snapshot_preview1::WasiSnapshotPreview1 for WasiCtx { .table() .get_dir(u32::from(dirfd))? .get_cap(DirCaps::READLINK)? - .read_link(path.as_str()?.deref()) + .read_link(path.as_str()?.expect("cannot use with shared memories; see https://github.com/bytecodealliance/wasmtime/issues/5235 (TODO)").deref()) .await? .into_os_string() .into_string() @@ -979,7 +979,7 @@ impl wasi_snapshot_preview1::WasiSnapshotPreview1 for WasiCtx { self.table() .get_dir(u32::from(dirfd))? .get_cap(DirCaps::REMOVE_DIRECTORY)? - .remove_dir(path.as_str()?.deref()) + .remove_dir(path.as_str()?.expect("cannot use with shared memories; see https://github.com/bytecodealliance/wasmtime/issues/5235 (TODO)").deref()) .await } @@ -999,9 +999,9 @@ impl wasi_snapshot_preview1::WasiSnapshotPreview1 for WasiCtx { .get_cap(DirCaps::RENAME_TARGET)?; src_dir .rename( - src_path.as_str()?.deref(), + src_path.as_str()?.expect("cannot use with shared memories; see https://github.com/bytecodealliance/wasmtime/issues/5235 (TODO)").deref(), dest_dir.deref(), - dest_path.as_str()?.deref(), + dest_path.as_str()?.expect("cannot use with shared memories; see https://github.com/bytecodealliance/wasmtime/issues/5235 (TODO)").deref(), ) .await } @@ -1015,7 +1015,7 @@ impl wasi_snapshot_preview1::WasiSnapshotPreview1 for WasiCtx { self.table() .get_dir(u32::from(dirfd))? .get_cap(DirCaps::SYMLINK)? - .symlink(src_path.as_str()?.deref(), dest_path.as_str()?.deref()) + .symlink(src_path.as_str()?.expect("cannot use with shared memories; see https://github.com/bytecodealliance/wasmtime/issues/5235 (TODO)").deref(), dest_path.as_str()?.expect("cannot use with shared memories; see https://github.com/bytecodealliance/wasmtime/issues/5235 (TODO)").deref()) .await } @@ -1027,7 +1027,8 @@ impl wasi_snapshot_preview1::WasiSnapshotPreview1 for WasiCtx { self.table() .get_dir(u32::from(dirfd))? .get_cap(DirCaps::UNLINK_FILE)? - .unlink_file(path.as_str()?.deref()) + .unlink_file(path.as_str()? + .expect("cannot use with shared memories; see https://github.com/bytecodealliance/wasmtime/issues/5235 (TODO)").deref()) .await } diff --git a/crates/wasi-crypto/src/wiggle_interfaces/asymmetric_common.rs b/crates/wasi-crypto/src/wiggle_interfaces/asymmetric_common.rs index dd346257de18..8588b0f03b2b 100644 --- a/crates/wasi-crypto/src/wiggle_interfaces/asymmetric_common.rs +++ b/crates/wasi-crypto/src/wiggle_interfaces/asymmetric_common.rs @@ -17,7 +17,7 @@ impl super::wasi_ephemeral_crypto_asymmetric_common::WasiEphemeralCryptoAsymmetr alg_str: &wiggle::GuestPtr<'_, str>, options_handle: &guest_types::OptOptions, ) -> Result { - let alg_str = &*alg_str.as_str()?; + let alg_str = &*alg_str.as_str()?.expect("cannot use with shared memories; see https://github.com/bytecodealliance/wasmtime/issues/5235 (TODO)"); let options_handle = match *options_handle { guest_types::OptOptions::Some(options_handle) => Some(options_handle), guest_types::OptOptions::None => None, @@ -89,7 +89,7 @@ impl super::wasi_ephemeral_crypto_asymmetric_common::WasiEphemeralCryptoAsymmetr alg_str: &wiggle::GuestPtr<'_, str>, options_handle: &guest_types::OptOptions, ) -> Result { - let alg_str = &*alg_str.as_str()?; + let alg_str = &*alg_str.as_str()?.expect("cannot use with shared memories; see https://github.com/bytecodealliance/wasmtime/issues/5235 (TODO)"); let options_handle = match *options_handle { guest_types::OptOptions::Some(options_handle) => Some(options_handle), guest_types::OptOptions::None => None, @@ -107,7 +107,7 @@ impl super::wasi_ephemeral_crypto_asymmetric_common::WasiEphemeralCryptoAsymmetr encoded_len: guest_types::Size, encoding: guest_types::KeypairEncoding, ) -> Result { - let alg_str = &*alg_str.as_str()?; + let alg_str = &*alg_str.as_str()?.expect("cannot use with shared memories; see https://github.com/bytecodealliance/wasmtime/issues/5235 (TODO)"); let encoded = &*encoded_ptr .as_array(encoded_len) .as_slice()? @@ -167,7 +167,7 @@ impl super::wasi_ephemeral_crypto_asymmetric_common::WasiEphemeralCryptoAsymmetr encoded_len: guest_types::Size, encoding: guest_types::PublickeyEncoding, ) -> Result { - let alg_str = &*alg_str.as_str()?; + let alg_str = &*alg_str.as_str()?.expect("cannot use with shared memories; see https://github.com/bytecodealliance/wasmtime/issues/5235 (TODO)"); let encoded = &*encoded_ptr .as_array(encoded_len) .as_slice()? @@ -218,7 +218,7 @@ impl super::wasi_ephemeral_crypto_asymmetric_common::WasiEphemeralCryptoAsymmetr encoded_len: guest_types::Size, encoding: guest_types::SecretkeyEncoding, ) -> Result { - let alg_str = &*alg_str.as_str()?; + let alg_str = &*alg_str.as_str()?.expect("cannot use with shared memories; see https://github.com/bytecodealliance/wasmtime/issues/5235 (TODO)"); let encoded = &*encoded_ptr .as_array(encoded_len) .as_slice()? diff --git a/crates/wasi-crypto/src/wiggle_interfaces/common.rs b/crates/wasi-crypto/src/wiggle_interfaces/common.rs index 1652717581c3..c3c73b5fe793 100644 --- a/crates/wasi-crypto/src/wiggle_interfaces/common.rs +++ b/crates/wasi-crypto/src/wiggle_interfaces/common.rs @@ -27,7 +27,7 @@ impl super::wasi_ephemeral_crypto_common::WasiEphemeralCryptoCommon for WasiCryp value_ptr: &wiggle::GuestPtr<'_, u8>, value_len: guest_types::Size, ) -> Result<(), guest_types::CryptoErrno> { - let name_str: &str = &*name_str.as_str()?; + let name_str: &str = &*name_str.as_str()?.expect("cannot use with shared memories; see https://github.com/bytecodealliance/wasmtime/issues/5235 (TODO)"); let value: &[u8] = { &*value_ptr .as_array(value_len) @@ -44,7 +44,7 @@ impl super::wasi_ephemeral_crypto_common::WasiEphemeralCryptoCommon for WasiCryp buffer_ptr: &wiggle::GuestPtr<'_, u8>, buffer_len: guest_types::Size, ) -> Result<(), guest_types::CryptoErrno> { - let name_str: &str = &*name_str.as_str()?; + let name_str: &str = &*name_str.as_str()?.expect("cannot use with shared memories; see https://github.com/bytecodealliance/wasmtime/issues/5235 (TODO)"); let buffer: &'static mut [u8] = unsafe { std::mem::transmute( &mut *buffer_ptr @@ -62,7 +62,7 @@ impl super::wasi_ephemeral_crypto_common::WasiEphemeralCryptoCommon for WasiCryp name_str: &wiggle::GuestPtr<'_, str>, value: u64, ) -> Result<(), guest_types::CryptoErrno> { - let name_str: &str = &*name_str.as_str()?; + let name_str: &str = &*name_str.as_str()?.expect("cannot use with shared memories; see https://github.com/bytecodealliance/wasmtime/issues/5235 (TODO)"); Ok((&*self).options_set_u64(options_handle.into(), name_str, value)?) } diff --git a/crates/wasi-crypto/src/wiggle_interfaces/signatures.rs b/crates/wasi-crypto/src/wiggle_interfaces/signatures.rs index 653446070f9c..5ffc1b729a8c 100644 --- a/crates/wasi-crypto/src/wiggle_interfaces/signatures.rs +++ b/crates/wasi-crypto/src/wiggle_interfaces/signatures.rs @@ -22,7 +22,7 @@ impl super::wasi_ephemeral_crypto_signatures::WasiEphemeralCryptoSignatures for encoded_len: guest_types::Size, encoding: guest_types::SignatureEncoding, ) -> Result { - let alg_str = &*alg_str.as_str()?; + let alg_str = &*alg_str.as_str()?.expect("cannot use with shared memories; see https://github.com/bytecodealliance/wasmtime/issues/5235 (TODO)"); let encoded = &*encoded_ptr .as_array(encoded_len) .as_slice()? diff --git a/crates/wasi-crypto/src/wiggle_interfaces/symmetric.rs b/crates/wasi-crypto/src/wiggle_interfaces/symmetric.rs index 6a65dfaa117b..9ad6a5661110 100644 --- a/crates/wasi-crypto/src/wiggle_interfaces/symmetric.rs +++ b/crates/wasi-crypto/src/wiggle_interfaces/symmetric.rs @@ -12,7 +12,7 @@ impl super::wasi_ephemeral_crypto_symmetric::WasiEphemeralCryptoSymmetric for Wa alg_str: &wiggle::GuestPtr<'_, str>, options_handle: &guest_types::OptOptions, ) -> Result { - let alg_str = &*alg_str.as_str()?; + let alg_str = &*alg_str.as_str()?.expect("cannot use with shared memories; see https://github.com/bytecodealliance/wasmtime/issues/5235 (TODO)"); let options_handle = match *options_handle { guest_types::OptOptions::Some(options_handle) => Some(options_handle), guest_types::OptOptions::None => None, @@ -86,7 +86,7 @@ impl super::wasi_ephemeral_crypto_symmetric::WasiEphemeralCryptoSymmetric for Wa alg_str: &wiggle::GuestPtr<'_, str>, options_handle: &guest_types::OptOptions, ) -> Result { - let alg_str = &*alg_str.as_str()?; + let alg_str = &*alg_str.as_str()?.expect("cannot use with shared memories; see https://github.com/bytecodealliance/wasmtime/issues/5235 (TODO)"); let options_handle = match *options_handle { guest_types::OptOptions::Some(options_handle) => Some(options_handle), guest_types::OptOptions::None => None, @@ -102,7 +102,7 @@ impl super::wasi_ephemeral_crypto_symmetric::WasiEphemeralCryptoSymmetric for Wa raw_ptr: &wiggle::GuestPtr<'_, u8>, raw_len: guest_types::Size, ) -> Result { - let alg_str = &*alg_str.as_str()?; + let alg_str = &*alg_str.as_str()?.expect("cannot use with shared memories; see https://github.com/bytecodealliance/wasmtime/issues/5235 (TODO)"); let raw = &*raw_ptr .as_array(raw_len) .as_slice()? @@ -153,7 +153,7 @@ impl super::wasi_ephemeral_crypto_symmetric::WasiEphemeralCryptoSymmetric for Wa key_handle: &guest_types::OptSymmetricKey, options_handle: &guest_types::OptOptions, ) -> Result { - let alg_str = &*alg_str.as_str()?; + let alg_str = &*alg_str.as_str()?.expect("cannot use with shared memories; see https://github.com/bytecodealliance/wasmtime/issues/5235 (TODO)"); let key_handle = match *key_handle { guest_types::OptSymmetricKey::Some(key_handle) => Some(key_handle), guest_types::OptSymmetricKey::None => None, @@ -178,7 +178,7 @@ impl super::wasi_ephemeral_crypto_symmetric::WasiEphemeralCryptoSymmetric for Wa value_ptr: &wiggle::GuestPtr<'_, u8>, value_max_len: guest_types::Size, ) -> Result { - let name_str: &str = &*name_str.as_str()?; + let name_str: &str = &*name_str.as_str()?.expect("cannot use with shared memories; see https://github.com/bytecodealliance/wasmtime/issues/5235 (TODO)"); let value = &mut *value_ptr .as_array(value_max_len) .as_slice_mut()? @@ -193,7 +193,7 @@ impl super::wasi_ephemeral_crypto_symmetric::WasiEphemeralCryptoSymmetric for Wa symmetric_state_handle: guest_types::SymmetricState, name_str: &wiggle::GuestPtr<'_, str>, ) -> Result { - let name_str: &str = &*name_str.as_str()?; + let name_str: &str = &*name_str.as_str()?.expect("cannot use with shared memories; see https://github.com/bytecodealliance/wasmtime/issues/5235 (TODO)"); Ok((&*self).options_get_u64(symmetric_state_handle.into(), name_str)?) } @@ -244,7 +244,7 @@ impl super::wasi_ephemeral_crypto_symmetric::WasiEphemeralCryptoSymmetric for Wa symmetric_state_handle: guest_types::SymmetricState, alg_str: &wiggle::GuestPtr<'_, str>, ) -> Result { - let alg_str = &*alg_str.as_str()?; + let alg_str = &*alg_str.as_str()?.expect("cannot use with shared memories; see https://github.com/bytecodealliance/wasmtime/issues/5235 (TODO)"); Ok((&*self) .symmetric_state_squeeze_key(symmetric_state_handle.into(), alg_str)? .into()) diff --git a/crates/wiggle/src/lib.rs b/crates/wiggle/src/lib.rs index a03bc6e20ff1..b4a4338065b3 100644 --- a/crates/wiggle/src/lib.rs +++ b/crates/wiggle/src/lib.rs @@ -724,42 +724,23 @@ impl<'a> GuestPtr<'a, str> { GuestPtr::new(self.mem, self.pointer) } - /// Returns a pointer for the underlying slice of bytes that this - /// pointer points to. - pub fn as_byte_ptr(&self) -> GuestPtr<'a, [u8]> { - GuestPtr::new(self.mem, self.pointer) - } - /// Attempts to create a [`GuestStr<'_>`] from this pointer, performing - /// bounds checks and utf-8 checks. The resulting `GuestStr` can be used - /// as a `&str` via the `Deref` trait. The region of memory backing the - /// `str` will be marked as shareably borrowed by the [`GuestMemory`] - /// until the `GuestStr` is dropped. + /// bounds checks and utf-8 checks. The resulting `GuestStr` can be used as + /// a `&str` via the `Deref` trait. The region of memory backing the `str` + /// will be marked as shareably borrowed by the [`GuestMemory`] until the + /// `GuestStr` is dropped. /// /// 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. - pub fn as_str(&self) -> Result, GuestError> { - let ptr = self - .mem - .validate_size_align(self.pointer.0, 1, self.pointer.1)?; - - let borrow = self.mem.shared_borrow(Region { - start: self.pointer.0, - len: self.pointer.1, - })?; - - // SAFETY: iff there are no overlapping borrows it is ok to construct - // a &mut str. - let ptr = unsafe { slice::from_raw_parts(ptr, self.pointer.1 as usize) }; - // Validate that contents are utf-8: - match str::from_utf8(ptr) { - Ok(ptr) => Ok(GuestStr { - ptr, - mem: self.mem, - borrow, - }), - Err(e) => Err(GuestError::InvalidUtf8(e)), + /// + /// Additionally, because it is `unsafe` to have a `GuestStr` of shared + /// memory, this function will return `None` in this case. + pub fn as_str(&self) -> Result>, GuestError> { + match self.as_bytes().as_unsafe_slice_mut()?.shared_str() { + UnsafeBorrowResult::Ok(s) => Ok(Some(s)), + UnsafeBorrowResult::Shared(_) => Ok(None), + UnsafeBorrowResult::Err(e) => Err(e), } } @@ -772,27 +753,14 @@ impl<'a> GuestPtr<'a, str> { /// This function will return `GuestStrMut` into host memory if all checks /// succeed (valid utf-8, valid pointers, etc). If any checks fail then /// `GuestError` will be returned. - pub fn as_str_mut(&self) -> Result, GuestError> { - let ptr = self - .mem - .validate_size_align(self.pointer.0, 1, self.pointer.1)?; - - let borrow = self.mem.mut_borrow(Region { - start: self.pointer.0, - len: self.pointer.1, - })?; - - // 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(GuestStrMut { - ptr, - mem: self.mem, - borrow, - }), - Err(e) => Err(GuestError::InvalidUtf8(e)), + /// + /// Additionally, because it is `unsafe` to have a `GuestStrMut` of shared + /// memory, this function will return `None` in this case. + pub fn as_str_mut(&self) -> Result>, GuestError> { + match self.as_bytes().as_unsafe_slice_mut()?.mut_str() { + UnsafeBorrowResult::Ok(s) => Ok(Some(s)), + UnsafeBorrowResult::Shared(_) => Ok(None), + UnsafeBorrowResult::Err(e) => Err(e), } } } @@ -952,6 +920,65 @@ impl<'a, T> UnsafeGuestSlice<'a, T> { } } +impl<'a> UnsafeGuestSlice<'a, u8> { + /// Transform an `unsafe` guest slice to a [`GuestStr`]. + /// + /// # Safety + /// + /// This function is safe if and only if: + /// - the memory is not shared (it will return `None` in this case) and + /// - there are no overlapping mutable borrows for this region. + fn shared_str(self) -> UnsafeBorrowResult, Self> { + if self.mem.is_shared_memory() { + UnsafeBorrowResult::Shared(self) + } else { + match self.mem.shared_borrow(self.region) { + Ok(borrow) => { + let ptr = unsafe { slice::from_raw_parts(self.ptr, self.len) }; + match str::from_utf8(ptr) { + Ok(ptr) => UnsafeBorrowResult::Ok(GuestStr { + ptr, + mem: self.mem, + borrow, + }), + Err(e) => UnsafeBorrowResult::Err(GuestError::InvalidUtf8(e)), + } + } + Err(e) => UnsafeBorrowResult::Err(e), + } + } + } + + /// Transform an `unsafe` guest slice to a [`GuestStrMut`]. + /// + /// # Safety + /// + /// This function is safe if and only if: + /// - the memory is not shared (it will return `None` in this case) and + /// - there are no overlapping borrows of any kind (shared or mutable) for + /// this region. + fn mut_str(self) -> UnsafeBorrowResult, Self> { + if self.mem.is_shared_memory() { + UnsafeBorrowResult::Shared(self) + } else { + match self.mem.shared_borrow(self.region) { + Ok(borrow) => { + let ptr = unsafe { slice::from_raw_parts_mut(self.ptr, self.len) }; + match str::from_utf8_mut(ptr) { + Ok(ptr) => UnsafeBorrowResult::Ok(GuestStrMut { + ptr, + mem: self.mem, + borrow, + }), + Err(e) => UnsafeBorrowResult::Err(GuestError::InvalidUtf8(e)), + } + } + Err(e) => UnsafeBorrowResult::Err(e), + } + } + } +} + /// A three-way result type for expressing that borrowing from an /// [`UnsafeGuestSlice`] could fail in multiple ways. Retaining the /// [`UnsafeGuestSlice`] in the `Shared` case allows us to reuse it. diff --git a/crates/wiggle/tests/strings.rs b/crates/wiggle/tests/strings.rs index c419942dafb2..a3ccdcab945a 100644 --- a/crates/wiggle/tests/strings.rs +++ b/crates/wiggle/tests/strings.rs @@ -10,7 +10,10 @@ impl_errno!(types::Errno); impl<'a> strings::Strings for WasiCtx<'a> { fn hello_string(&mut self, a_string: &GuestPtr) -> Result { - let s = a_string.as_str().expect("should be valid string"); + let s = a_string + .as_str() + .expect("should be valid string") + .expect("expected non-shared memory"); println!("a_string='{}'", &*s); Ok(s.len() as u32) } @@ -21,9 +24,18 @@ impl<'a> strings::Strings for WasiCtx<'a> { b: &GuestPtr, c: &GuestPtr, ) -> Result { - 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 sa = a + .as_str() + .expect("A should be valid string") + .expect("expected non-shared memory"); + let sb = b + .as_str() + .expect("B should be valid string") + .expect("expected non-shared memory"); + let sc = c + .as_str() + .expect("C should be valid string") + .expect("expected non-shared memory"); let total_len = sa.len() + sb.len() + sc.len(); println!( "len={}, a='{}', b='{}', c='{}'", From 9d4a48c0399344edd4f06bb57de6ca2bd985f419 Mon Sep 17 00:00:00 2001 From: Andrew Brown Date: Mon, 14 Nov 2022 13:38:49 -0800 Subject: [PATCH 2/2] wiggle: make `GuestStr*` containers wrappers of `GuestSlice*` This change makes it possible to reuse the underlying logic in `UnsafeGuestSlice` and the `GuestSlice*` implementations to continue to expose the `GuestStr` and `GuestStrMut` types. These types now are simple wrappers of their `GuestSlice*` variant. The UTF-8 validation that distinguished `GuestStr*` now lives in the `TryFrom` implementations for each type. --- crates/wiggle/src/lib.rs | 119 +++++++++++---------------------------- 1 file changed, 33 insertions(+), 86 deletions(-) diff --git a/crates/wiggle/src/lib.rs b/crates/wiggle/src/lib.rs index b4a4338065b3..c58b7c0eca4f 100644 --- a/crates/wiggle/src/lib.rs +++ b/crates/wiggle/src/lib.rs @@ -737,8 +737,8 @@ impl<'a> GuestPtr<'a, str> { /// Additionally, because it is `unsafe` to have a `GuestStr` of shared /// memory, this function will return `None` in this case. pub fn as_str(&self) -> Result>, GuestError> { - match self.as_bytes().as_unsafe_slice_mut()?.shared_str() { - UnsafeBorrowResult::Ok(s) => Ok(Some(s)), + match self.as_bytes().as_unsafe_slice_mut()?.shared_borrow() { + UnsafeBorrowResult::Ok(s) => Ok(Some(s.try_into()?)), UnsafeBorrowResult::Shared(_) => Ok(None), UnsafeBorrowResult::Err(e) => Err(e), } @@ -757,8 +757,8 @@ impl<'a> GuestPtr<'a, str> { /// Additionally, because it is `unsafe` to have a `GuestStrMut` of shared /// memory, this function will return `None` in this case. pub fn as_str_mut(&self) -> Result>, GuestError> { - match self.as_bytes().as_unsafe_slice_mut()?.mut_str() { - UnsafeBorrowResult::Ok(s) => Ok(Some(s)), + match self.as_bytes().as_unsafe_slice_mut()?.mut_borrow() { + UnsafeBorrowResult::Ok(s) => Ok(Some(s.try_into()?)), UnsafeBorrowResult::Shared(_) => Ok(None), UnsafeBorrowResult::Err(e) => Err(e), } @@ -920,65 +920,6 @@ impl<'a, T> UnsafeGuestSlice<'a, T> { } } -impl<'a> UnsafeGuestSlice<'a, u8> { - /// Transform an `unsafe` guest slice to a [`GuestStr`]. - /// - /// # Safety - /// - /// This function is safe if and only if: - /// - the memory is not shared (it will return `None` in this case) and - /// - there are no overlapping mutable borrows for this region. - fn shared_str(self) -> UnsafeBorrowResult, Self> { - if self.mem.is_shared_memory() { - UnsafeBorrowResult::Shared(self) - } else { - match self.mem.shared_borrow(self.region) { - Ok(borrow) => { - let ptr = unsafe { slice::from_raw_parts(self.ptr, self.len) }; - match str::from_utf8(ptr) { - Ok(ptr) => UnsafeBorrowResult::Ok(GuestStr { - ptr, - mem: self.mem, - borrow, - }), - Err(e) => UnsafeBorrowResult::Err(GuestError::InvalidUtf8(e)), - } - } - Err(e) => UnsafeBorrowResult::Err(e), - } - } - } - - /// Transform an `unsafe` guest slice to a [`GuestStrMut`]. - /// - /// # Safety - /// - /// This function is safe if and only if: - /// - the memory is not shared (it will return `None` in this case) and - /// - there are no overlapping borrows of any kind (shared or mutable) for - /// this region. - fn mut_str(self) -> UnsafeBorrowResult, Self> { - if self.mem.is_shared_memory() { - UnsafeBorrowResult::Shared(self) - } else { - match self.mem.shared_borrow(self.region) { - Ok(borrow) => { - let ptr = unsafe { slice::from_raw_parts_mut(self.ptr, self.len) }; - match str::from_utf8_mut(ptr) { - Ok(ptr) => UnsafeBorrowResult::Ok(GuestStrMut { - ptr, - mem: self.mem, - borrow, - }), - Err(e) => UnsafeBorrowResult::Err(GuestError::InvalidUtf8(e)), - } - } - Err(e) => UnsafeBorrowResult::Err(e), - } - } - } -} - /// A three-way result type for expressing that borrowing from an /// [`UnsafeGuestSlice`] could fail in multiple ways. Retaining the /// [`UnsafeGuestSlice`] in the `Shared` case allows us to reuse it. @@ -1001,50 +942,56 @@ impl From for UnsafeBorrowResult { /// A smart pointer to an shareable `str` in guest memory. /// Usable as a `&'a str` via [`std::ops::Deref`]. -pub struct GuestStr<'a> { - ptr: &'a str, - mem: &'a dyn GuestMemory, - borrow: BorrowHandle, +pub struct GuestStr<'a>(GuestSlice<'a, u8>); + +impl<'a> std::convert::TryFrom> for GuestStr<'a> { + type Error = GuestError; + fn try_from(slice: GuestSlice<'a, u8>) -> Result { + match str::from_utf8(&slice) { + Ok(_) => Ok(Self(slice)), + Err(e) => Err(GuestError::InvalidUtf8(e)), + } + } } impl<'a> std::ops::Deref for GuestStr<'a> { type Target = str; fn deref(&self) -> &Self::Target { - self.ptr - } -} - -impl<'a> Drop for GuestStr<'a> { - fn drop(&mut self) { - self.mem.shared_unborrow(self.borrow) + // SAFETY: every slice in a `GuestStr` has already been checked for + // UTF-8 validity during construction (i.e., `TryFrom`). + unsafe { str::from_utf8_unchecked(&self.0) } } } /// 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 GuestStrMut<'a> { - ptr: &'a mut str, - mem: &'a dyn GuestMemory, - borrow: BorrowHandle, +pub struct GuestStrMut<'a>(GuestSliceMut<'a, u8>); + +impl<'a> std::convert::TryFrom> for GuestStrMut<'a> { + type Error = GuestError; + fn try_from(slice: GuestSliceMut<'a, u8>) -> Result { + match str::from_utf8(&slice) { + Ok(_) => Ok(Self(slice)), + Err(e) => Err(GuestError::InvalidUtf8(e)), + } + } } impl<'a> std::ops::Deref for GuestStrMut<'a> { type Target = str; fn deref(&self) -> &Self::Target { - self.ptr + // SAFETY: every slice in a `GuestStrMut` has already been checked for + // UTF-8 validity during construction (i.e., `TryFrom`). + unsafe { str::from_utf8_unchecked(&self.0) } } } impl<'a> std::ops::DerefMut for GuestStrMut<'a> { fn deref_mut(&mut self) -> &mut Self::Target { - self.ptr - } -} - -impl<'a> Drop for GuestStrMut<'a> { - fn drop(&mut self) { - self.mem.mut_unborrow(self.borrow) + // SAFETY: every slice in a `GuestStrMut` has already been checked for + // UTF-8 validity during construction (i.e., `TryFrom`). + unsafe { str::from_utf8_unchecked_mut(&mut self.0) } } }