Change the return type of SharedMemory::data (#5240)

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<u8>]` 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

@@ -169,12 +169,11 @@ impl DiffInstance for WasmtimeInstance {
 
  fn get_memory(&mut self, name: &str, shared: bool) -> Option<Vec<u8>> {
  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)

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<u8>` 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<u8>] {
  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())
  }
  }
 

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, ())?;