diff --git a/lib/api/src/externals/table.rs b/lib/api/src/externals/table.rs index a327f88f7..872d6c4da 100644 --- a/lib/api/src/externals/table.rs +++ b/lib/api/src/externals/table.rs @@ -4,6 +4,7 @@ use crate::store::Store; use crate::types::{Val, ValFuncRef}; use crate::RuntimeError; use crate::TableType; +use std::sync::Arc; use wasmer_runtime::{Export, ExportTable, Table as RuntimeTable, VMCallerCheckedAnyfunc}; /// The `Table` struct is an array-like structure representing a WebAssembly Table, @@ -39,17 +40,18 @@ impl Table { .create_table(table_plan) .map_err(RuntimeError::new)?; - let definition = table.vmtable(); - for i in 0..definition.current_elements { + let num_elements = table.size(); + for i in 0..num_elements { set_table_item(table.as_ref(), i, item.clone())?; } + let definition = table.vmtable(); Ok(Table { store: store.clone(), owned_by_store: true, exported: ExportTable { from: table, - definition: Box::leak(Box::new(definition)), + definition, }, }) } diff --git a/lib/api/src/instance.rs b/lib/api/src/instance.rs index e814d5c94..44c12ef01 100644 --- a/lib/api/src/instance.rs +++ b/lib/api/src/instance.rs @@ -19,6 +19,19 @@ pub struct Instance { /// The exports for an instance. pub exports: Exports, } +/* +#[cfg(test)] +mod send_test { + use super::*; + + fn is_send() -> bool { + true + } + #[test] + fn instance_is_send() { + assert!(is_send::()); + } +}*/ impl Instance { /// Creates a new `Instance` from a WebAssembly [`Module`] and a diff --git a/lib/api/src/memory.rs b/lib/api/src/memory.rs index 6fa2862e8..1e3c5c952 100644 --- a/lib/api/src/memory.rs +++ b/lib/api/src/memory.rs @@ -1,13 +1,14 @@ use crate::{Bytes, Pages}; use more_asserts::{assert_ge, assert_le}; -use std::cell::RefCell; +use std::borrow::{Borrow, BorrowMut}; +use std::sync::{Arc, Mutex}; use wasmer_runtime::{Memory, MemoryError, MemoryPlan, MemoryStyle, Mmap, VMMemoryDefinition}; /// A linear memory instance. #[derive(Debug)] pub struct LinearMemory { // The underlying allocation. - mmap: RefCell, + mmap: Arc>, // The optional maximum size in wasm pages of this linear memory. maximum: Option, @@ -81,7 +82,7 @@ impl LinearMemory { }; Ok(Self { - mmap: mmap.into(), + mmap: Arc::new(Mutex::new(mmap)), maximum: plan.memory.maximum, offset_guard_size: offset_guard_bytes, needs_signal_handlers, @@ -98,7 +99,8 @@ impl Memory for LinearMemory { /// Returns the number of allocated wasm pages. fn size(&self) -> Pages { - self.mmap.borrow().size + let mmap_guard = self.mmap.lock().unwrap(); + mmap_guard.borrow().size } /// Grow memory by the specified amount of wasm pages. @@ -106,8 +108,9 @@ impl Memory for LinearMemory { /// Returns `None` if memory can't be grown by the specified amount /// of wasm pages. fn grow(&self, delta: Pages) -> Result { + let mut mmap_guard = self.mmap.lock().unwrap(); + let mmap = mmap_guard.borrow_mut(); // Optimization of memory.grow 0 calls. - let mut mmap = self.mmap.borrow_mut(); if delta.0 == 0 { return Ok(mmap.size); } @@ -178,7 +181,8 @@ impl Memory for LinearMemory { /// Return a `VMMemoryDefinition` for exposing the memory to compiled wasm code. fn vmmemory(&self) -> VMMemoryDefinition { - let mut mmap = self.mmap.borrow_mut(); + let mut mmap_guard = self.mmap.lock().unwrap(); + let mmap = mmap_guard.borrow_mut(); VMMemoryDefinition { base: mmap.alloc.as_mut_ptr(), current_length: mmap.size.bytes().0, diff --git a/lib/api/src/table.rs b/lib/api/src/table.rs index e984be57a..40d533ee4 100644 --- a/lib/api/src/table.rs +++ b/lib/api/src/table.rs @@ -1,6 +1,9 @@ use crate::ValType; -use std::cell::RefCell; +use std::borrow::{Borrow, BorrowMut}; +use std::cell::UnsafeCell; use std::convert::{TryFrom, TryInto}; +use std::ptr::NonNull; +use std::sync::Mutex; use wasmer_runtime::{ Table, TablePlan, TableStyle, Trap, TrapCode, VMCallerCheckedAnyfunc, VMTableDefinition, }; @@ -8,11 +11,18 @@ use wasmer_runtime::{ /// A table instance. #[derive(Debug)] pub struct LinearTable { - vec: RefCell>, + // TODO: we can remove the mutex by using atomic swaps and preallocating the max table size + vec: Mutex>, maximum: Option, plan: TablePlan, + vm_table_definition: Box>, } +/// This is correct because there is no thread-specific data tied to this type. +unsafe impl Send for LinearTable {} +/// This is correct because all internal mutability is protected by a mutex. +unsafe impl Sync for LinearTable {} + impl LinearTable { /// Create a new table instance with specified minimum and maximum number of elements. pub fn new(plan: &TablePlan) -> Result { @@ -28,16 +38,19 @@ impl LinearTable { )); } } + let table_minimum = usize::try_from(plan.table.minimum) + .map_err(|_| "Table minimum is bigger than usize".to_string())?; + let mut vec = vec![VMCallerCheckedAnyfunc::default(); table_minimum]; + let base = vec.as_mut_ptr(); match plan.style { TableStyle::CallerChecksSignature => Ok(Self { - vec: RefCell::new(vec![ - VMCallerCheckedAnyfunc::default(); - usize::try_from(plan.table.minimum).map_err(|_| { - "Table minimum is bigger than usize".to_string() - })? - ]), + vec: Mutex::new(vec), maximum: plan.table.maximum, plan: plan.clone(), + vm_table_definition: Box::new(UnsafeCell::new(VMTableDefinition { + base: base as _, + current_elements: table_minimum as _, + })), }), } } @@ -51,7 +64,8 @@ impl Table for LinearTable { /// Returns the number of allocated elements. fn size(&self) -> u32 { - self.vec.borrow().len().try_into().unwrap() + let vec_guard = self.vec.lock().unwrap(); + vec_guard.borrow().len().try_into().unwrap() } /// Grow table by the specified amount of elements. @@ -59,15 +73,23 @@ impl Table for LinearTable { /// Returns `None` if table can't be grown by the specified amount /// of elements, otherwise returns the previous size of the table. fn grow(&self, delta: u32) -> Option { + let mut vec_guard = self.vec.lock().unwrap(); + let vec = vec_guard.borrow_mut(); let size = self.size(); let new_len = size.checked_add(delta)?; if self.maximum.map_or(false, |max| new_len > max) { return None; } - self.vec.borrow_mut().resize( + vec.resize( usize::try_from(new_len).unwrap(), VMCallerCheckedAnyfunc::default(), ); + // update table definition + unsafe { + let td = &mut *self.vm_table_definition.get(); + td.current_elements = new_len; + td.base = vec.as_mut_ptr() as _; + } Some(size) } @@ -75,7 +97,8 @@ impl Table for LinearTable { /// /// Returns `None` if the index is out of bounds. fn get(&self, index: u32) -> Option { - self.vec.borrow().get(index as usize).cloned() + let vec_guard = self.vec.lock().unwrap(); + vec_guard.borrow().get(index as usize).cloned() } /// Set reference to the specified element. @@ -84,7 +107,9 @@ impl Table for LinearTable { /// /// Returns an error if the index is out of bounds. fn set(&self, index: u32, func: VMCallerCheckedAnyfunc) -> Result<(), Trap> { - match self.vec.borrow_mut().get_mut(index as usize) { + let mut vec_guard = self.vec.lock().unwrap(); + let vec = vec_guard.borrow_mut(); + match vec.get_mut(index as usize) { Some(slot) => { *slot = func; Ok(()) @@ -94,11 +119,10 @@ impl Table for LinearTable { } /// Return a `VMTableDefinition` for exposing the table to compiled wasm code. - fn vmtable(&self) -> VMTableDefinition { - let mut vec = self.vec.borrow_mut(); - VMTableDefinition { - base: vec.as_mut_ptr() as *mut u8, - current_elements: vec.len().try_into().unwrap(), - } + fn vmtable(&self) -> NonNull { + let _vec_guard = self.vec.lock().unwrap(); + let ptr = self.vm_table_definition.as_ref() as *const UnsafeCell + as *const VMTableDefinition as *mut VMTableDefinition; + unsafe { NonNull::new_unchecked(ptr) } } } diff --git a/lib/runtime/src/export.rs b/lib/runtime/src/export.rs index 29a21d35b..33559b596 100644 --- a/lib/runtime/src/export.rs +++ b/lib/runtime/src/export.rs @@ -7,6 +7,7 @@ use crate::vmcontext::{ VMContext, VMFunctionBody, VMFunctionKind, VMGlobalDefinition, VMMemoryDefinition, VMTableDefinition, }; +use std::ptr::NonNull; use std::sync::Arc; use wasm_common::{FunctionType, GlobalType}; @@ -49,7 +50,7 @@ impl From for Export { #[derive(Debug, Clone)] pub struct ExportTable { /// The address of the table descriptor. - pub definition: *mut VMTableDefinition, + pub definition: NonNull, /// Pointer to the containing `Table`. pub from: Arc, } diff --git a/lib/runtime/src/instance.rs b/lib/runtime/src/instance.rs index c01dd0daa..8ae26e126 100644 --- a/lib/runtime/src/instance.rs +++ b/lib/runtime/src/instance.rs @@ -23,6 +23,7 @@ use std::any::Any; use std::cell::{Cell, RefCell}; use std::collections::HashMap; use std::convert::TryFrom; +use std::ptr::NonNull; use std::sync::Arc; use std::{mem, ptr, slice}; use wasm_common::entity::{packed_option::ReservedValue, BoxedSlice, EntityRef, PrimaryMap}; @@ -178,20 +179,20 @@ impl Instance { /// Return the indexed `VMTableDefinition`. #[allow(dead_code)] fn table(&self, index: LocalTableIndex) -> VMTableDefinition { - unsafe { *self.table_ptr(index) } + unsafe { self.table_ptr(index).as_ref().clone() } } /// Updates the value for a defined table to `VMTableDefinition`. - fn set_table(&self, index: LocalTableIndex, table: VMTableDefinition) { + fn set_table(&self, index: LocalTableIndex, table: &VMTableDefinition) { unsafe { - *self.table_ptr(index) = table; + *self.table_ptr(index).as_ptr() = table.clone(); } } /// Return the indexed `VMTableDefinition`. - fn table_ptr(&self, index: LocalTableIndex) -> *mut VMTableDefinition { + fn table_ptr(&self, index: LocalTableIndex) -> NonNull { let index = usize::try_from(index.as_u32()).unwrap(); - unsafe { self.tables_ptr().add(index) } + NonNull::new(unsafe { self.tables_ptr().add(index) }).unwrap() } /// Return a pointer to the `VMTableDefinition`s. @@ -502,7 +503,9 @@ impl Instance { .grow(delta); // Keep current the VMContext pointers used by compiled wasm code. - self.set_table(table_index, self.tables[table_index].vmtable()); + let table_ptr = self.tables[table_index].vmtable(); + let vmtable = unsafe { table_ptr.as_ref() }; + self.set_table(table_index, vmtable); result } @@ -631,7 +634,8 @@ impl Instance { // https://webassembly.github.io/reference-types/core/exec/instructions.html#exec-memory-copy let memory = self.memory(memory_index); - memory.memory_copy(dst, src, len) + // The following memory copy is not synchronized and is not atomic: + unsafe { memory.memory_copy(dst, src, len) } } /// Perform a `memory.copy` on an imported memory. @@ -644,7 +648,8 @@ impl Instance { ) -> Result<(), Trap> { let import = self.imported_memory(memory_index); let memory = unsafe { &*import.definition }; - memory.memory_copy(dst, src, len) + // The following memory copy is not synchronized and is not atomic: + unsafe { memory.memory_copy(dst, src, len) } } /// Perform the `memory.fill` operation on a locally defined memory. @@ -660,7 +665,8 @@ impl Instance { len: u32, ) -> Result<(), Trap> { let memory = self.memory(memory_index); - memory.memory_fill(dst, val, len) + // The following memory fill is not synchronized and is not atomic: + unsafe { memory.memory_fill(dst, val, len) } } /// Perform the `memory.fill` operation on an imported memory. @@ -677,7 +683,8 @@ impl Instance { ) -> Result<(), Trap> { let import = self.imported_memory(memory_index); let memory = unsafe { &*import.definition }; - memory.memory_fill(dst, val, len) + // The following memory fill is not synchronized and is not atomic: + unsafe { memory.memory_fill(dst, val, len) } } /// Performs the `memory.init` operation. @@ -785,9 +792,15 @@ impl InstanceHandle { vmshared_signatures: BoxedSlice, host_state: Box, ) -> Result { + // TODO: investigate `vmctx_tables` and `vmctx_memories`: both of these + // appear to be dropped in this function which may cause memory problems + // depending on the ownership of the types in the `PrimaryMap`. let vmctx_tables = finished_tables .values() - .map(|t| t.vmtable()) + .map(|t| { + let vmtable_ptr = t.vmtable(); + vmtable_ptr.as_ref().clone() + }) .collect::>() .into_boxed_slice(); diff --git a/lib/runtime/src/memory.rs b/lib/runtime/src/memory.rs index fcc43a029..0dab7f4d1 100644 --- a/lib/runtime/src/memory.rs +++ b/lib/runtime/src/memory.rs @@ -62,7 +62,7 @@ pub struct MemoryPlan { } /// Trait for implementing Wasm Memory used by Wasmer. -pub trait Memory: fmt::Debug { +pub trait Memory: fmt::Debug + Send + Sync { /// Returns the memory plan for this memory. fn plan(&self) -> &MemoryPlan; @@ -72,6 +72,8 @@ pub trait Memory: fmt::Debug { /// Grow memory by the specified amount of wasm pages. fn grow(&self, delta: Pages) -> Result; - /// Return a `VMMemoryDefinition` for exposing the memory to compiled wasm code. + /// Return a [`VMMemoryDefinition`] for exposing the memory to compiled wasm code. + /// + /// The pointer returned in [`VMMemoryDefinition`] must be valid for the lifetime of this memory. fn vmmemory(&self) -> VMMemoryDefinition; } diff --git a/lib/runtime/src/table.rs b/lib/runtime/src/table.rs index 920b24476..7066a6962 100644 --- a/lib/runtime/src/table.rs +++ b/lib/runtime/src/table.rs @@ -9,6 +9,7 @@ use crate::trap::{Trap, TrapCode}; use crate::vmcontext::{VMCallerCheckedAnyfunc, VMTableDefinition}; use serde::{Deserialize, Serialize}; use std::fmt; +use std::ptr::NonNull; use wasm_common::{FunctionIndex, GlobalIndex, TableIndex, TableType}; /// A WebAssembly table initializer. @@ -42,7 +43,7 @@ pub struct TablePlan { } /// Trait for implementing the interface of a Wasm table. -pub trait Table: fmt::Debug { +pub trait Table: fmt::Debug + Send + Sync { /// Returns the table plan for this Table. fn plan(&self) -> &TablePlan; @@ -68,7 +69,7 @@ pub trait Table: fmt::Debug { fn set(&self, index: u32, func: VMCallerCheckedAnyfunc) -> Result<(), Trap>; /// Return a `VMTableDefinition` for exposing the table to compiled wasm code. - fn vmtable(&self) -> VMTableDefinition; + fn vmtable(&self) -> NonNull; /// Copy `len` elements from `src_table[src_index..]` into `dst_table[dst_index..]`. /// diff --git a/lib/runtime/src/vmcontext.rs b/lib/runtime/src/vmcontext.rs index cafe31838..d6e6eda43 100644 --- a/lib/runtime/src/vmcontext.rs +++ b/lib/runtime/src/vmcontext.rs @@ -10,8 +10,9 @@ use crate::table::Table; use crate::trap::{Trap, TrapCode}; use std::any::Any; use std::convert::TryFrom; +use std::ptr::{self, NonNull}; use std::sync::Arc; -use std::{ptr, u32}; +use std::u32; /// An imported function. #[derive(Debug, Copy, Clone)] @@ -140,7 +141,7 @@ pub enum VMFunctionKind { #[repr(C)] pub struct VMTableImport { /// A pointer to the imported table description. - pub definition: *mut VMTableDefinition, + pub definition: NonNull, /// A pointer to the `Table` that owns the table description. pub from: Arc, @@ -247,21 +248,36 @@ mod test_vmglobal_import { #[derive(Debug, Copy, Clone)] #[repr(C)] pub struct VMMemoryDefinition { - /// The start address. + /// The start address which is always valid, even if the memory grows. pub base: *mut u8, /// The current logical size of this linear memory in bytes. pub current_length: usize, } +/// # Safety +/// This data is safe to share between threads because it's plain data that +/// is the user's responsibility to synchronize. +unsafe impl Send for VMMemoryDefinition {} +/// # Safety +/// This data is safe to share between threads because it's plain data that +/// is the user's responsibility to synchronize. And it's `Copy` so there's +/// really no difference between passing it by reference or by value as far as +/// correctness in a multi-threaded context is concerned. +unsafe impl Sync for VMMemoryDefinition {} + impl VMMemoryDefinition { - /// Do a `memory.copy` for the memory. + /// Do an unsynchronized, non-atomic `memory.copy` for the memory. /// /// # Errors /// /// Returns a `Trap` error when the source or destination ranges are out of /// bounds. - pub(crate) fn memory_copy(&self, dst: u32, src: u32, len: u32) -> Result<(), Trap> { + /// + /// # Safety + /// The memory is not copied atomically and is not synchronized: it's the + /// caller's responsibility to synchronize. + pub(crate) unsafe fn memory_copy(&self, dst: u32, src: u32, len: u32) -> Result<(), Trap> { // https://webassembly.github.io/reference-types/core/exec/instructions.html#exec-memory-copy if src .checked_add(len) @@ -278,21 +294,24 @@ impl VMMemoryDefinition { // Bounds and casts are checked above, by this point we know that // everything is safe. - unsafe { - let dst = self.base.add(dst); - let src = self.base.add(src); - ptr::copy(src, dst, len as usize); - } + let dst = self.base.add(dst); + let src = self.base.add(src); + ptr::copy(src, dst, len as usize); Ok(()) } - /// Perform the `memory.fill` operation for the memory. + /// Perform the `memory.fill` operation for the memory in an unsynchronized, + /// non-atomic way. /// /// # Errors /// /// Returns a `Trap` error if the memory range is out of bounds. - pub(crate) fn memory_fill(&self, dst: u32, val: u32, len: u32) -> Result<(), Trap> { + /// + /// # Safety + /// The memory is not filled atomically and is not synchronized: it's the + /// caller's responsibility to synchronize. + pub(crate) unsafe fn memory_fill(&self, dst: u32, val: u32, len: u32) -> Result<(), Trap> { if dst .checked_add(len) .map_or(true, |m| m as usize > self.current_length) @@ -305,10 +324,8 @@ impl VMMemoryDefinition { // Bounds and casts are checked above, by this point we know that // everything is safe. - unsafe { - let dst = self.base.offset(dst); - ptr::write_bytes(dst, val, len as usize); - } + let dst = self.base.offset(dst); + ptr::write_bytes(dst, val, len as usize); Ok(()) } @@ -348,7 +365,7 @@ mod test_vmmemory_definition { /// The fields compiled code needs to access to utilize a WebAssembly table /// defined within the instance. -#[derive(Debug, Copy, Clone)] +#[derive(Debug, Clone)] #[repr(C)] pub struct VMTableDefinition { /// Pointer to the table data.