From bafd260cbb2869717480c6809888e27c4fb99854 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Wed, 9 Nov 2022 16:52:29 -0800 Subject: [PATCH] Change the return type of `SharedMemory::data` This commit is an attempt at improving the safety of using the return value of the `SharedMemory::data` method. Previously this returned `*mut [u8]` which, while correct, is unwieldy and unsafe to work with. The new return value of `&[UnsafeCell]` has a few advantages: * The lifetime of the returned data is now connected to the `SharedMemory` itself, removing the possibility for a class of errors of accidentally using the prior `*mut [u8]` beyond its original lifetime. * It's not possibly to safely access `.len()` as opposed to requiring an `unsafe` dereference before. * The data internally within the slice is now what retains the `unsafe` bits, namely indicating that accessing any memory inside of the contents returned is `unsafe` but addressing it is safe. I was inspired by the `wiggle`-based discussion on #5229 and felt it appropriate to apply a similar change here. --- crates/fuzzing/src/oracles/diff_wasmtime.rs | 7 ++--- crates/wasmtime/src/memory.rs | 31 ++++++++++++++------- tests/all/threads.rs | 13 +++++++-- 3 files changed, 34 insertions(+), 17 deletions(-) diff --git a/crates/fuzzing/src/oracles/diff_wasmtime.rs b/crates/fuzzing/src/oracles/diff_wasmtime.rs index f2a31aee1fd1..bdf28fbd5847 100644 --- a/crates/fuzzing/src/oracles/diff_wasmtime.rs +++ b/crates/fuzzing/src/oracles/diff_wasmtime.rs @@ -169,12 +169,11 @@ impl DiffInstance for WasmtimeInstance { fn get_memory(&mut self, name: &str, shared: bool) -> Option> { Some(if shared { - let data = self + let memory = self .instance .get_shared_memory(&mut self.store, name) - .unwrap() - .data(); - unsafe { (*data).to_vec() } + .unwrap(); + memory.data().iter().map(|i| unsafe { *i.get() }).collect() } else { self.instance .get_memory(&mut self.store, name) diff --git a/crates/wasmtime/src/memory.rs b/crates/wasmtime/src/memory.rs index 677400d1ee6d..4d5cc788b26d 100644 --- a/crates/wasmtime/src/memory.rs +++ b/crates/wasmtime/src/memory.rs @@ -2,6 +2,7 @@ use crate::store::{StoreData, StoreOpaque, Stored}; use crate::trampoline::generate_memory_export; use crate::{AsContext, AsContextMut, Engine, MemoryType, StoreContext, StoreContextMut}; use anyhow::{bail, Result}; +use std::cell::UnsafeCell; use std::convert::TryFrom; use std::slice; use wasmtime_environ::MemoryPlan; @@ -699,6 +700,7 @@ pub unsafe trait MemoryCreator: Send + Sync { /// ``` #[derive(Clone)] pub struct SharedMemory(wasmtime_runtime::SharedMemory, Engine); + impl SharedMemory { /// Construct a [`SharedMemory`] by providing both the `minimum` and /// `maximum` number of 64K-sized pages. This call allocates the necessary @@ -737,19 +739,28 @@ impl SharedMemory { /// Return access to the available portion of the shared memory. /// - /// Because the memory is shared, it is possible that this memory is being - /// modified in other threads--in other words, the data can change at any - /// time. Users of this function must manage synchronization and locking to - /// this region of memory themselves. + /// The slice returned represents the region of accessible memory at the + /// time that this function was called. The contents of the returned slice + /// will reflect concurrent modifications happening on other threads. + /// + /// # Safety + /// + /// The returned slice is valid for the entire duration of the lifetime of + /// this instance of [`SharedMemory`]. The base pointer of a shared memory + /// does not change. This [`SharedMemory`] may grow further after this + /// function has been called, but the slice returned will not grow. /// - /// Not only can the data change, but the length of this region can change - /// as well. Other threads can call `memory.grow` operations that will - /// extend the region length but--importantly--this will not be reflected in - /// the size of region returned by this function. - pub fn data(&self) -> *mut [u8] { + /// Concurrent modifications may be happening to the data returned on other + /// threads. The `UnsafeCell` represents that safe access to the + /// contents of the slice is not possible through normal loads and stores. + /// + /// The memory returned must be accessed safely through the `Atomic*` types + /// in the [`std::sync::atomic`] module. Casting to those types must + /// currently be done unsafely. + pub fn data(&self) -> &[UnsafeCell] { unsafe { let definition = &*self.0.vmmemory_ptr(); - slice::from_raw_parts_mut(definition.base, definition.current_length()) + slice::from_raw_parts_mut(definition.base.cast(), definition.current_length()) } } diff --git a/tests/all/threads.rs b/tests/all/threads.rs index 30e22e32e914..4d05a8fd8029 100644 --- a/tests/all/threads.rs +++ b/tests/all/threads.rs @@ -65,15 +65,22 @@ fn test_sharing_of_shared_memory() -> Result<()> { let shared_memory = SharedMemory::new(&engine, MemoryType::shared(1, 5))?; let instance1 = Instance::new(&mut store, &module, &[shared_memory.clone().into()])?; let instance2 = Instance::new(&mut store, &module, &[shared_memory.clone().into()])?; + let data = shared_memory.data(); // Modify the memory in one place. unsafe { - (*(shared_memory.data()))[0] = 42; + *data[0].get() = 42; } // Verify that the memory is the same in all shared locations. - let shared_memory_first_word = - i32::from_le_bytes(unsafe { (*shared_memory.data())[0..4].try_into()? }); + let shared_memory_first_word = i32::from_le_bytes(unsafe { + [ + *data[0].get(), + *data[1].get(), + *data[2].get(), + *data[3].get(), + ] + }); let instance1_first_word = instance1 .get_typed_func::<(), i32, _>(&mut store, "first_word")? .call(&mut store, ())?;