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.
This commit is contained in:
Alex Crichton
2022-11-10 11:51:10 -06:00
committed by GitHub
parent 5b6d5e78de
commit 2be457c295
3 changed files with 34 additions and 17 deletions

View File

@@ -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)

View File

@@ -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.
///
/// 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] {
/// # 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.
///
/// 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())
}
}