Improve thread safety of core types

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();
+        let num_elements = table.size();
-        for i in 0..definition.current_elements {
+        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,
             },
         })
     }

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<T: Send>() -> bool {
+        true
+    }
+    #[test]
+    fn instance_is_send() {
+        assert!(is_send::<Instance>());
+    }
+}*/
 
 impl Instance {
     /// Creates a new `Instance` from a WebAssembly [`Module`] and a

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<WasmMmap>,
+    mmap: Arc<Mutex<WasmMmap>>,
 
     // The optional maximum size in wasm pages of this linear memory.
     maximum: Option<Pages>,
@@ -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<Pages, MemoryError> {
+        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,

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<Vec<VMCallerCheckedAnyfunc>>,
+    // TODO: we can remove the mutex by using atomic swaps and preallocating the max table size
+    vec: Mutex<Vec<VMCallerCheckedAnyfunc>>,
     maximum: Option<u32>,
     plan: TablePlan,
+    vm_table_definition: Box<UnsafeCell<VMTableDefinition>>,
 }
 
+/// 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<Self, String> {
@@ -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![
+                vec: Mutex::new(vec),
-                    VMCallerCheckedAnyfunc::default();
-                    usize::try_from(plan.table.minimum).map_err(|_| {
-                        "Table minimum is bigger than usize".to_string()
-                    })?
-                ]),
                 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<u32> {
+        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<VMCallerCheckedAnyfunc> {
-        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 {
+    fn vmtable(&self) -> NonNull<VMTableDefinition> {
-        let mut vec = self.vec.borrow_mut();
+        let _vec_guard = self.vec.lock().unwrap();
-        VMTableDefinition {
+        let ptr = self.vm_table_definition.as_ref() as *const UnsafeCell<VMTableDefinition>
-            base: vec.as_mut_ptr() as *mut u8,
+            as *const VMTableDefinition as *mut VMTableDefinition;
-            current_elements: vec.len().try_into().unwrap(),
+        unsafe { NonNull::new_unchecked(ptr) }
-        }
     }
 }

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<ExportFunction> for Export {
 #[derive(Debug, Clone)]
 pub struct ExportTable {
     /// The address of the table descriptor.
-    pub definition: *mut VMTableDefinition,
+    pub definition: NonNull<VMTableDefinition>,
     /// Pointer to the containing `Table`.
     pub from: Arc<dyn Table>,
 }

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<VMTableDefinition> {
         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<SignatureIndex, VMSharedSignatureIndex>,
         host_state: Box<dyn Any>,
     ) -> Result<Self, Trap> {
+        // 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::<PrimaryMap<LocalTableIndex, _>>()
             .into_boxed_slice();
 

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<Pages, MemoryError>;
 
-    /// 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;
 }

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<VMTableDefinition>;
 
     /// Copy `len` elements from `src_table[src_index..]` into `dst_table[dst_index..]`.
     ///

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<VMTableDefinition>,
 
     /// A pointer to the `Table` that owns the table description.
     pub from: Arc<dyn Table>,
@@ -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 dst = self.base.add(dst);
+        let src = self.base.add(src);
-            let src = self.base.add(src);
+        ptr::copy(src, dst, len as usize);
-            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);
-            let dst = self.base.offset(dst);
+        ptr::write_bytes(dst, val, len as usize);
-            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.