From 8368f05b8b12e22a23c9d722a4255b08fe390f11 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Tue, 2 May 2023 15:16:07 -0700 Subject: [PATCH 01/11] Make Wasmtime compatible with Stacked Borrows in MIRI The fact that Wasmtime executes correctly under Tree Borrows but not Stacked Borrows is a bit suspect and given what I've since learned about the aliasing models I wanted to give it a stab to get things working with Stacked Borrows. It turns out that this wasn't all that difficult, but required two underlying changes: * First the implementation of `Instance::vmctx` is now specially crafted in an intentional way to preserve the provenance of the returned pointer. This way all `&Instance` pointers will return a `VMContext` pointer with the same provenance and acquiring the pointer won't accidentally invalidate all prior pointers. * Second the conversion from `VMContext` to `Instance` has been updated to work with provenance and such. Previously the conversion looked like `&mut VMContext -> &mut Instance`, but I think this didn't play well with MIRI because `&mut VMContext` has no provenance over any data since it's zero-sized. Instead now the conversion is from `*mut VMContext` to `&mut Instance` where we know that `*mut VMContext` has provenance over the entire instance allocation. This shuffled a fair bit around to handle the new closure-based API to prevent escaping pointers, but otherwise no major change other than the structure and the types in play. This commit additionally picks up a dependency on the `sptr` crate which is a crate for prototyping strict-provenance APIs in Rust. This is I believe intended to be upstreamed into Rust one day (it's in the standard library as a Nightly-only API right now) but in the meantime this is a stable alternative. --- Cargo.lock | 7 + crates/runtime/Cargo.toml | 1 + crates/runtime/src/component.rs | 49 ++-- crates/runtime/src/debug_builtins.rs | 36 +-- crates/runtime/src/instance.rs | 267 ++++++++++++------ .../runtime/src/instance/allocator/pooling.rs | 11 +- crates/runtime/src/lib.rs | 8 +- crates/runtime/src/libcalls.rs | 267 +++++++++--------- crates/runtime/src/memory.rs | 52 ++-- crates/runtime/src/traphandlers.rs | 4 +- crates/runtime/src/vmcontext.rs | 28 -- crates/wasmtime/src/externals.rs | 9 +- crates/wasmtime/src/func.rs | 17 +- crates/wasmtime/src/instance.rs | 2 +- crates/wasmtime/src/memory.rs | 26 +- crates/wasmtime/src/store.rs | 68 ++--- supply-chain/audits.toml | 10 + 17 files changed, 485 insertions(+), 377 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 0cb248761dea..f7229ca7f9cd 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2965,6 +2965,12 @@ dependencies = [ "der", ] +[[package]] +name = "sptr" +version = "0.3.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3b9b39299b249ad65f3b7e96443bad61c02ca5cd3589f46cb6d610a0fd6c0d6a" + [[package]] name = "stable_deref_trait" version = "1.2.0" @@ -4106,6 +4112,7 @@ dependencies = [ "paste", "rand 0.8.5", "rustix", + "sptr", "wasmtime-asm-macros", "wasmtime-environ", "wasmtime-fiber", diff --git a/crates/runtime/Cargo.toml b/crates/runtime/Cargo.toml index beb2c33a3730..0ff570f4cba4 100644 --- a/crates/runtime/Cargo.toml +++ b/crates/runtime/Cargo.toml @@ -25,6 +25,7 @@ anyhow = { workspace = true } memfd = "0.6.2" paste = "1.0.3" encoding_rs = { version = "0.8.31", optional = true } +sptr = "0.3.2" [target.'cfg(target_os = "macos")'.dependencies] mach = "0.3.2" diff --git a/crates/runtime/src/component.rs b/crates/runtime/src/component.rs index aa167ca6b3e9..d65b0ce3a002 100644 --- a/crates/runtime/src/component.rs +++ b/crates/runtime/src/component.rs @@ -11,6 +11,7 @@ use crate::{ VMNativeCallFunction, VMOpaqueContext, VMSharedSignatureIndex, VMWasmCallFunction, ValRaw, }; use memoffset::offset_of; +use sptr::Strict; use std::alloc::{self, Layout}; use std::marker; use std::mem; @@ -40,36 +41,19 @@ pub struct ComponentInstance { /// Size and offset information for the trailing `VMComponentContext`. offsets: VMComponentOffsets, - /// A self-referential pointer to the `ComponentInstance` itself. - /// - /// The reason for this field's existence is a bit subtle because if you - /// have a pointer to `ComponentInstance` then why have this field which has - /// the same information? The subtlety here is due to the fact that this is - /// here to handle provenance and aliasing issues with optimizations in LLVM - /// and Rustc. Put another way, this is here to make MIRI happy. That's an - /// oversimplification, though, since this is actually required for - /// correctness to communicate the actual intent to LLVM's optimizations and - /// such. - /// - /// This field is chiefly used during `ComponentInstance::vmctx`. If you - /// think of pointers as a pair of address and provenance, this type stores - /// the provenance of the entire allocation. That's technically all we need - /// here, just the provenance, but that can only be done with storage of a - /// pointer hence the storage here. - /// - /// Finally note that there's a small wrapper around this to add `Send` and - /// `Sync` bounds, but serves no other purpose. - self_reference: RawComponentInstance, + /// For more information about this see the documentation on + /// `Instance::vmctx_self_reference`. + vmctx_self_reference: VMComponentContextSelfReference, /// A zero-sized field which represents the end of the struct for the actual /// `VMComponentContext` to be allocated behind. vmctx: VMComponentContext, } -struct RawComponentInstance(NonNull); +struct VMComponentContextSelfReference(NonNull); -unsafe impl Send for RawComponentInstance {} -unsafe impl Sync for RawComponentInstance {} +unsafe impl Send for VMComponentContextSelfReference {} +unsafe impl Sync for VMComponentContextSelfReference {} /// Type signature for host-defined trampolines that are called from /// WebAssembly. @@ -174,7 +158,15 @@ impl ComponentInstance { ptr.as_ptr(), ComponentInstance { offsets, - self_reference: RawComponentInstance(ptr), + vmctx_self_reference: VMComponentContextSelfReference( + NonNull::new( + ptr.as_ptr() + .cast::() + .add(mem::size_of::()) + .cast(), + ) + .unwrap(), + ), vmctx: VMComponentContext { _marker: marker::PhantomPinned, }, @@ -185,13 +177,8 @@ impl ComponentInstance { } fn vmctx(&self) -> *mut VMComponentContext { - let self_ref = self.self_reference.0.as_ptr(); - unsafe { - self_ref - .cast::() - .add(mem::size_of::()) - .cast() - } + let addr = std::ptr::addr_of!(self.vmctx); + Strict::with_addr(self.vmctx_self_reference.0.as_ptr(), Strict::addr(addr)) } unsafe fn vmctx_plus_offset(&self, offset: u32) -> *const T { diff --git a/crates/runtime/src/debug_builtins.rs b/crates/runtime/src/debug_builtins.rs index 8354c1146687..9e7ef295a374 100644 --- a/crates/runtime/src/debug_builtins.rs +++ b/crates/runtime/src/debug_builtins.rs @@ -1,6 +1,6 @@ #![doc(hidden)] -use crate::instance::InstanceHandle; +use crate::instance::Instance; use crate::vmcontext::VMContext; use wasmtime_environ::{EntityRef, MemoryIndex}; @@ -8,14 +8,15 @@ static mut VMCTX_AND_MEMORY: (*mut VMContext, usize) = (std::ptr::null_mut(), 0) #[no_mangle] pub unsafe extern "C" fn resolve_vmctx_memory(ptr: usize) -> *const u8 { - let handle = InstanceHandle::from_vmctx(VMCTX_AND_MEMORY.0); - assert!( - VMCTX_AND_MEMORY.1 < handle.module().memory_plans.len(), - "memory index for debugger is out of bounds" - ); - let index = MemoryIndex::new(VMCTX_AND_MEMORY.1); - let mem = handle.instance().get_memory(index); - mem.base.add(ptr) + Instance::from_vmctx(VMCTX_AND_MEMORY.0, |handle| { + assert!( + VMCTX_AND_MEMORY.1 < handle.module().memory_plans.len(), + "memory index for debugger is out of bounds" + ); + let index = MemoryIndex::new(VMCTX_AND_MEMORY.1); + let mem = handle.get_memory(index); + mem.base.add(ptr) + }) } #[no_mangle] @@ -25,14 +26,15 @@ pub unsafe extern "C" fn resolve_vmctx_memory_ptr(p: *const u32) -> *const u8 { !VMCTX_AND_MEMORY.0.is_null(), "must call `__vmctx->set()` before resolving Wasm pointers" ); - let handle = InstanceHandle::from_vmctx(VMCTX_AND_MEMORY.0); - assert!( - VMCTX_AND_MEMORY.1 < handle.module().memory_plans.len(), - "memory index for debugger is out of bounds" - ); - let index = MemoryIndex::new(VMCTX_AND_MEMORY.1); - let mem = handle.instance().get_memory(index); - mem.base.add(ptr as usize) + Instance::from_vmctx(VMCTX_AND_MEMORY.0, |handle| { + assert!( + VMCTX_AND_MEMORY.1 < handle.module().memory_plans.len(), + "memory index for debugger is out of bounds" + ); + let index = MemoryIndex::new(VMCTX_AND_MEMORY.1); + let mem = handle.get_memory(index); + mem.base.add(ptr as usize) + }) } #[no_mangle] diff --git a/crates/runtime/src/instance.rs b/crates/runtime/src/instance.rs index 1f9dabfae1f8..93e7bd639b6c 100644 --- a/crates/runtime/src/instance.rs +++ b/crates/runtime/src/instance.rs @@ -17,7 +17,7 @@ use crate::{ }; use anyhow::Error; use anyhow::Result; -use memoffset::offset_of; +use sptr::Strict; use std::alloc::{self, Layout}; use std::any::Any; use std::convert::TryFrom; @@ -54,7 +54,7 @@ pub use allocator::*; /// This `Instance` type is used as a ubiquitous representation for WebAssembly /// values, whether or not they were created on the host or through a module. #[repr(C)] // ensure that the vmctx field is last. -pub(crate) struct Instance { +pub struct Instance { /// The runtime info (corresponding to the "compiled module" /// abstraction in higher layers) that is retained and needed for /// lazy initialization. This provides access to the underlying @@ -95,12 +95,63 @@ pub(crate) struct Instance { /// index of the slot in the pooling allocator. index: usize, + /// A pointer to the `vmctx` field at the end of the `Instance`. + /// + /// If you're looking at this a reasonable question would be "why do we need + /// a pointer to ourselves?" because after all the pointer's valid is + /// trivially derivable from any `&Instance` pointer. The rationale for this + /// field's existence is subtle, but it's required for correctness. The + /// short version is "this makes miri happy". + /// + /// The long version of why this field exists is that the rules that MIRI + /// uses to ensure pointers are used correctly have various conditions on + /// them depend on how pointers are used. More specifically if `*mut T` is + /// derived from `&mut T`, then that invalidates all prior pointers drived + /// from the `&mut T`. This means that while we liberally want to re-acquire + /// a `*mut VMContext` throughout the implementation of `Instance` the + /// trivial way, a function `fn vmctx(&mut Instance) -> *mut VMContext` + /// would effectively invalidate all prior `*mut VMContext` pointers + /// acquired. The purpose of this field is to serve as a sort of + /// source-of-truth for where `*mut VMContext` pointers come from. + /// + /// This field is initialized when the `Instance` is created with the + /// original allocation's pointer. That means that the provenance of this + /// pointer contains the entire allocation (both instance and `VMContext`). + /// This provenance bit is then "carried through" where `fn vmctx` will base + /// all returned pointers on this pointer itself. This provides the means of + /// never invalidating this pointer throughout MIRI and additionally being + /// able to still temporarily have `&mut Instance` methods and such. + /// + /// It's important to note, though, that this is not here purely for MIRI. + /// The careful construction of the `fn vmctx` method has ramifications on + /// the LLVM IR generated, for example. A historical CVE on Wasmtime, + /// GHSA-ch89-5g45-qwc7, was caused due to relying on undefined behavior. By + /// deriving VMContext pointers from this pointer it specifically hints to + /// LLVM that trickery is afoot and it properly informs `noalias` and such + /// annotations and analysis. More-or-less this pointer is actually loaded + /// in LLVM IR which helps defeat otherwise present aliasing optimizations, + /// which we want, since writes to this should basically never be optimized + /// out. + /// + /// As a final note it's worth pointing out that the machine code generated + /// for accessing `fn vmctx` is still as one would expect. This member isn't + /// actually ever loaded at runtime (or at least shouldn't be). Perhaps in + /// the future if the memory consumption of this field is a problem we could + /// shrink it slightly, but for now one extra pointer per wasm instance + /// seems not too bad. + vmctx_self_reference: VMContextSelfReference, + /// Additional context used by compiled wasm code. This field is last, and /// represents a dynamically-sized array that extends beyond the nominal /// end of the struct (similar to a flexible array member). vmctx: VMContext, } +struct VMContextSelfReference(NonNull); + +unsafe impl Send for VMContextSelfReference {} +unsafe impl Sync for VMContextSelfReference {} + #[allow(clippy::cast_ptr_alignment)] impl Instance { /// Create an instance at the given memory address. @@ -135,6 +186,9 @@ impl Instance { dropped_elements, dropped_data, host_state: req.host_state, + vmctx_self_reference: VMContextSelfReference( + NonNull::new(ptr.cast::().add(mem::size_of::()).cast()).unwrap(), + ), vmctx: VMContext { _marker: std::marker::PhantomPinned, }, @@ -145,16 +199,46 @@ impl Instance { InstanceHandle { instance: ptr } } + /// Converts the provided `*mut VMContext` to an `Instance` pointer and runs + /// the provided closure with the instance. + /// + /// This method will move the `vmctx` pointer backwards to point to the + /// original `Instance` that precedes it. The closure is provided a + /// temporary version of the `Instance` pointer with a constrained lifetime + /// to the closure to ensure it doesn't accidentally escape. + /// + /// # Unsafety + /// + /// Callers must validate that the `vmctx` pointer is a valid allocation + /// and that it's valid to acquire `&mut Instance` at this time. For example + /// this can't be called twice on the same `VMContext` to get two active + /// pointers to the same `Instance`. + pub unsafe fn from_vmctx(vmctx: *mut VMContext, f: impl FnOnce(&mut Instance) -> R) -> R { + let ptr = vmctx + .cast::() + .offset(-(mem::size_of::() as isize)) + .cast::(); + f(&mut *ptr) + } + /// Helper function to access various locations offset from our `*mut /// VMContext` object. + /// + /// # Safety + /// + /// This method is unsafe because the `offset` must be within bounds of the + /// `VMContext` object trailing this instance. unsafe fn vmctx_plus_offset(&self, offset: u32) -> *const T { - (std::ptr::addr_of!(self.vmctx).cast::()) + self.vmctx() + .cast::() .add(usize::try_from(offset).unwrap()) .cast() } + /// Dual of `vmctx_plus_offset`, but for mutability. unsafe fn vmctx_plus_offset_mut(&mut self, offset: u32) -> *mut T { - (std::ptr::addr_of_mut!(self.vmctx).cast::()) + self.vmctx() + .cast::() .add(usize::try_from(offset).unwrap()) .cast() } @@ -222,8 +306,11 @@ impl Instance { unsafe { &mut *self.get_defined_memory(defined_index) } } else { let import = self.imported_memory(index); - let ctx = unsafe { &mut *import.vmctx }; - unsafe { &mut *ctx.instance_mut().get_defined_memory(import.index) } + unsafe { + let ptr = + Instance::from_vmctx(import.vmctx, |i| i.get_defined_memory(import.index)); + &mut *ptr + } } } @@ -302,7 +389,7 @@ impl Instance { ptr } - pub unsafe fn set_store(&mut self, store: Option<*mut dyn Store>) { + pub(crate) unsafe fn set_store(&mut self, store: Option<*mut dyn Store>) { if let Some(store) = store { *self.vmctx_plus_offset_mut(self.offsets().vmctx_store()) = store; *self.runtime_limits() = (*store).vmruntime_limits(); @@ -329,14 +416,29 @@ impl Instance { /// Return a reference to the vmctx used by compiled wasm code. #[inline] - pub fn vmctx(&self) -> &VMContext { - &self.vmctx - } - - /// Return a raw pointer to the vmctx used by compiled wasm code. - #[inline] - pub fn vmctx_ptr(&self) -> *mut VMContext { - self.vmctx() as *const VMContext as *mut VMContext + pub fn vmctx(&self) -> *mut VMContext { + // The definition of this method is subtle but intentional. The goal + // here is that effectively this should return `&mut self.vmctx`, but + // it's not quite so simple. Some more documentation is available on the + // `vmctx_self_reference` field, but the general idea is that we're + // creating a pointer to return with proper provenance. Provenance is + // still in the works in Rust at the time of this writing but the load + // of the `self.vmctx_self_reference` field is important here as it + // affects how LLVM thinks about aliasing with respect to the returned + // pointer. + // + // The intention of this method is to codegen to machine code as `&mut + // self.vmctx`, however. While it doesn't show up like this in LLVM IR + // (there's an actual load of the field) it does look like that by the + // time the backend runs. (that's magic to me, the backend removing + // loads...) + // + // As a final minor note, strict provenance APIs are not stable on Rust + // today so the `sptr` crate is used. This crate provides the extension + // trait `Strict` but the method names conflict with the nightly methods + // so a different syntax is used to invoke methods here. + let addr = std::ptr::addr_of!(self.vmctx); + Strict::with_addr(self.vmctx_self_reference.0.as_ptr(), Strict::addr(addr)) } fn get_exported_func(&mut self, index: FuncIndex) -> ExportFunction { @@ -348,7 +450,7 @@ impl Instance { fn get_exported_table(&mut self, index: TableIndex) -> ExportTable { let (definition, vmctx) = if let Some(def_index) = self.module().defined_table_index(index) { - (self.table_ptr(def_index), self.vmctx_ptr()) + (self.table_ptr(def_index), self.vmctx()) } else { let import = self.imported_table(index); (import.from, import.vmctx) @@ -363,7 +465,7 @@ impl Instance { fn get_exported_memory(&mut self, index: MemoryIndex) -> ExportMemory { let (definition, vmctx, def_index) = if let Some(def_index) = self.module().defined_memory_index(index) { - (self.memory_ptr(def_index), self.vmctx_ptr(), def_index) + (self.memory_ptr(def_index), self.vmctx(), def_index) } else { let import = self.imported_memory(index); (import.from, import.vmctx, import.index) @@ -402,14 +504,8 @@ impl Instance { &*self.host_state } - /// Return the offset from the vmctx pointer to its containing Instance. - #[inline] - pub(crate) fn vmctx_offset() -> isize { - offset_of!(Self, vmctx) as isize - } - /// Return the table index for the given `VMTableDefinition`. - unsafe fn table_index(&mut self, table: &VMTableDefinition) -> DefinedTableIndex { + pub unsafe fn table_index(&mut self, table: &VMTableDefinition) -> DefinedTableIndex { let index = DefinedTableIndex::new( usize::try_from( (table as *const VMTableDefinition) @@ -431,17 +527,26 @@ impl Instance { index: MemoryIndex, delta: u64, ) -> Result, Error> { - let (idx, instance) = if let Some(idx) = self.module().defined_memory_index(index) { - (idx, self) - } else { - let import = self.imported_memory(index); - unsafe { - let foreign_instance = (*import.vmctx).instance_mut(); - (import.index, foreign_instance) + match self.module().defined_memory_index(index) { + Some(idx) => self.defined_memory_grow(idx, delta), + None => { + let import = self.imported_memory(index); + unsafe { + Instance::from_vmctx(import.vmctx, |i| { + i.defined_memory_grow(import.index, delta) + }) + } } - }; - let store = unsafe { &mut *instance.store() }; - let memory = &mut instance.memories[idx]; + } + } + + fn defined_memory_grow( + &mut self, + idx: DefinedMemoryIndex, + delta: u64, + ) -> Result, Error> { + let store = unsafe { &mut *self.store() }; + let memory = &mut self.memories[idx]; let result = unsafe { memory.grow(delta, Some(store)) }; @@ -449,7 +554,7 @@ impl Instance { // pointer and/or the length changed. if memory.as_shared_memory().is_none() { let vmmemory = memory.vmmemory(); - instance.set_memory(idx, vmmemory); + self.set_memory(idx, vmmemory); } result @@ -470,9 +575,9 @@ impl Instance { delta: u32, init_value: TableElement, ) -> Result, Error> { - let (defined_table_index, instance) = - self.get_defined_table_index_and_instance(table_index); - instance.defined_table_grow(defined_table_index, delta, init_value) + self.with_defined_table_index_and_instance(table_index, |i, instance| { + instance.defined_table_grow(i, delta, init_value) + }) } fn defined_table_grow( @@ -532,7 +637,7 @@ impl Instance { .array_to_wasm_trampoline(def_index) .expect("should have array-to-Wasm trampoline for escaping function"), wasm_call: Some(self.runtime_info.function(def_index)), - vmctx: VMOpaqueContext::from_vmcontext(self.vmctx_ptr()), + vmctx: VMOpaqueContext::from_vmcontext(self.vmctx()), type_index, } } else { @@ -679,7 +784,7 @@ impl Instance { } /// Get a locally-defined memory. - pub(crate) fn get_defined_memory(&mut self, index: DefinedMemoryIndex) -> *mut Memory { + pub fn get_defined_memory(&mut self, index: DefinedMemoryIndex) -> *mut Memory { ptr::addr_of_mut!(self.memories[index]) } @@ -835,12 +940,22 @@ impl Instance { table_index: TableIndex, range: impl Iterator, ) -> *mut Table { - let (idx, instance) = self.get_defined_table_index_and_instance(table_index); - let elt_ty = instance.tables[idx].element_type(); + self.with_defined_table_index_and_instance(table_index, |idx, instance| { + instance.get_defined_table_with_lazy_init(idx, range) + }) + } + + /// TODO + pub fn get_defined_table_with_lazy_init( + &mut self, + idx: DefinedTableIndex, + range: impl Iterator, + ) -> *mut Table { + let elt_ty = self.tables[idx].element_type(); if elt_ty == TableElementType::Func { for i in range { - let value = match instance.tables[idx].get(i) { + let value = match self.tables[idx].get(i) { Some(value) => value, None => { // Out-of-bounds; caller will handle by likely @@ -850,14 +965,15 @@ impl Instance { } }; if value.is_uninit() { - let table_init = match &instance.module().table_initialization { + let module = self.module(); + let table_init = match &self.module().table_initialization { // We unfortunately can't borrow `tables` outside the // loop because we need to call `get_func_ref` (a `&mut` // method) below; so unwrap it dynamically here. TableInitialization::FuncTable { tables, .. } => tables, _ => break, } - .get(table_index); + .get(module.table_index(idx)); // The TableInitialization::FuncTable elements table may // be smaller than the current size of the table: it @@ -870,26 +986,27 @@ impl Instance { let func_index = table_init.and_then(|indices| indices.get(i as usize).cloned()); let func_ref = func_index - .and_then(|func_index| instance.get_func_ref(func_index)) + .and_then(|func_index| self.get_func_ref(func_index)) .unwrap_or(std::ptr::null_mut()); let value = TableElement::FuncRef(func_ref); - instance.tables[idx] + self.tables[idx] .set(i, value) .expect("Table type should match and index should be in-bounds"); } } } - ptr::addr_of_mut!(instance.tables[idx]) + ptr::addr_of_mut!(self.tables[idx]) } /// Get a table by index regardless of whether it is locally-defined or an /// imported, foreign table. pub(crate) fn get_table(&mut self, table_index: TableIndex) -> *mut Table { - let (idx, instance) = self.get_defined_table_index_and_instance(table_index); - ptr::addr_of_mut!(instance.tables[idx]) + self.with_defined_table_index_and_instance(table_index, |idx, instance| { + ptr::addr_of_mut!(instance.tables[idx]) + }) } /// Get a locally-defined table. @@ -897,19 +1014,21 @@ impl Instance { ptr::addr_of_mut!(self.tables[index]) } - pub(crate) fn get_defined_table_index_and_instance( + pub(crate) fn with_defined_table_index_and_instance( &mut self, index: TableIndex, - ) -> (DefinedTableIndex, &mut Instance) { + f: impl FnOnce(DefinedTableIndex, &mut Instance) -> R, + ) -> R { if let Some(defined_table_index) = self.module().defined_table_index(index) { - (defined_table_index, self) + f(defined_table_index, self) } else { let import = self.imported_table(index); unsafe { - let foreign_instance = (*import.vmctx).instance_mut(); - let foreign_table_def = &*import.from; - let foreign_table_index = foreign_instance.table_index(foreign_table_def); - (foreign_table_index, foreign_instance) + Instance::from_vmctx(import.vmctx, |foreign_instance| { + let foreign_table_def = import.from; + let foreign_table_index = foreign_instance.table_index(&*foreign_table_def); + f(foreign_table_index, foreign_instance) + }) } } } @@ -1107,29 +1226,17 @@ fn _assert_send_sync() { } impl InstanceHandle { - /// Create a new `InstanceHandle` pointing at the instance - /// pointed to by the given `VMContext` pointer. - /// - /// # Safety - /// This is unsafe because it doesn't work on just any `VMContext`, it must - /// be a `VMContext` allocated as part of an `Instance`. - #[inline] - pub unsafe fn from_vmctx(vmctx: *mut VMContext) -> Self { - let instance = (&mut *vmctx).instance(); - Self { - instance: instance as *const Instance as *mut Instance, + /// TODO + pub fn null() -> InstanceHandle { + InstanceHandle { + instance: ptr::null_mut(), } } - /// Return a reference to the vmctx used by compiled wasm code. - pub fn vmctx(&self) -> &VMContext { - self.instance().vmctx() - } - /// Return a raw pointer to the vmctx used by compiled wasm code. #[inline] - pub fn vmctx_ptr(&self) -> *mut VMContext { - self.instance().vmctx_ptr() + pub fn vmctx(&self) -> *mut VMContext { + self.instance().vmctx() } /// Return a reference to a module. @@ -1181,16 +1288,6 @@ impl InstanceHandle { self.instance().host_state() } - /// Get a memory defined locally within this module. - pub fn get_defined_memory(&mut self, index: DefinedMemoryIndex) -> *mut Memory { - self.instance_mut().get_defined_memory(index) - } - - /// Return the table index for the given `VMTableDefinition` in this instance. - pub unsafe fn table_index(&mut self, table: &VMTableDefinition) -> DefinedTableIndex { - self.instance_mut().table_index(table) - } - /// Get a table defined locally within this module. pub fn get_defined_table(&mut self, index: DefinedTableIndex) -> *mut Table { self.instance_mut().get_defined_table(index) diff --git a/crates/runtime/src/instance/allocator/pooling.rs b/crates/runtime/src/instance/allocator/pooling.rs index 54e7588e3abd..5d3d57bbea7d 100644 --- a/crates/runtime/src/instance/allocator/pooling.rs +++ b/crates/runtime/src/instance/allocator/pooling.rs @@ -794,12 +794,8 @@ unsafe impl InstanceAllocator for PoolingInstanceAllocator { MemoryStyle::Dynamic { .. } => {} } - let memory = unsafe { - std::slice::from_raw_parts_mut( - self.memories.get_base(index, defined_index), - self.memories.max_accessible, - ) - }; + let base_ptr = self.memories.get_base(index, defined_index); + let base_capacity = self.memories.max_accessible; let mut slot = self.memories.take_memory_image_slot(index, defined_index); let image = req.runtime_info.memory_image(defined_index)?; @@ -822,7 +818,8 @@ unsafe impl InstanceAllocator for PoolingInstanceAllocator { memories.push(Memory::new_static( plan, - memory, + base_ptr, + base_capacity, slot, self.memories.memory_and_guard_size, unsafe { &mut *req.store.get().unwrap() }, diff --git a/crates/runtime/src/lib.rs b/crates/runtime/src/lib.rs index 47b854b2b0ae..6f7c0dc613fa 100644 --- a/crates/runtime/src/lib.rs +++ b/crates/runtime/src/lib.rs @@ -53,13 +53,13 @@ pub use wasmtime_jit_debug::gdb_jit_int::GdbJitImageRegistration; pub use crate::export::*; pub use crate::externref::*; pub use crate::imports::Imports; +#[cfg(feature = "pooling-allocator")] pub use crate::instance::{ - InstanceAllocationRequest, InstanceAllocator, InstanceHandle, OnDemandInstanceAllocator, - StorePtr, + Instance, InstanceLimits, PoolingInstanceAllocator, PoolingInstanceAllocatorConfig, }; -#[cfg(feature = "pooling-allocator")] pub use crate::instance::{ - InstanceLimits, PoolingInstanceAllocator, PoolingInstanceAllocatorConfig, + InstanceAllocationRequest, InstanceAllocator, InstanceHandle, OnDemandInstanceAllocator, + StorePtr, }; pub use crate::memory::{ DefaultMemoryCreator, Memory, RuntimeLinearMemory, RuntimeMemoryCreator, SharedMemory, diff --git a/crates/runtime/src/libcalls.rs b/crates/runtime/src/libcalls.rs index 4aa8133dbef7..631501d95aee 100644 --- a/crates/runtime/src/libcalls.rs +++ b/crates/runtime/src/libcalls.rs @@ -57,7 +57,7 @@ use crate::externref::VMExternRef; use crate::table::{Table, TableElementType}; use crate::vmcontext::{VMContext, VMFuncRef}; -use crate::TrapReason; +use crate::{Instance, TrapReason}; use anyhow::Result; use std::mem; use std::ptr::{self, NonNull}; @@ -114,9 +114,9 @@ pub mod trampolines { vmctx : *mut VMContext, $( $pname : libcall!(@ty $param), )* ) $( -> libcall!(@ty $result))? { - let result = std::panic::catch_unwind(|| { + let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { super::$name(vmctx, $($pname),*) - }); + })); match result { Ok(ret) => LibcallResult::convert(ret), Err(panic) => crate::traphandlers::resume_panic(panic), @@ -186,19 +186,20 @@ unsafe fn memory32_grow( delta: u64, memory_index: u32, ) -> Result<*mut u8, TrapReason> { - let instance = (*vmctx).instance_mut(); - let memory_index = MemoryIndex::from_u32(memory_index); - let result = - match instance - .memory_grow(memory_index, delta) - .map_err(|error| TrapReason::User { - error, - needs_backtrace: true, - })? { - Some(size_in_bytes) => size_in_bytes / (wasmtime_environ::WASM_PAGE_SIZE as usize), - None => usize::max_value(), - }; - Ok(result as *mut _) + Instance::from_vmctx(vmctx, |instance| { + let memory_index = MemoryIndex::from_u32(memory_index); + let result = + match instance + .memory_grow(memory_index, delta) + .map_err(|error| TrapReason::User { + error, + needs_backtrace: true, + })? { + Some(size_in_bytes) => size_in_bytes / (wasmtime_environ::WASM_PAGE_SIZE as usize), + None => usize::max_value(), + }; + Ok(result as *mut _) + }) } // Implementation of `table.grow`. @@ -213,22 +214,23 @@ unsafe fn table_grow( // or is a `VMExternRef` until we look at the table type. init_value: *mut u8, ) -> Result { - let instance = (*vmctx).instance_mut(); let table_index = TableIndex::from_u32(table_index); - let element = match instance.table_element_type(table_index) { - TableElementType::Func => (init_value as *mut VMFuncRef).into(), - TableElementType::Extern => { - let init_value = if init_value.is_null() { - None - } else { - Some(VMExternRef::clone_from_raw(init_value)) - }; - init_value.into() - } - }; - Ok(match instance.table_grow(table_index, delta, element)? { - Some(r) => r, - None => -1_i32 as u32, + Instance::from_vmctx(vmctx, |instance| { + let element = match instance.table_element_type(table_index) { + TableElementType::Func => (init_value as *mut VMFuncRef).into(), + TableElementType::Extern => { + let init_value = if init_value.is_null() { + None + } else { + Some(VMExternRef::clone_from_raw(init_value)) + }; + init_value.into() + } + }; + Ok(match instance.table_grow(table_index, delta, element)? { + Some(r) => r, + None => -1_i32 as u32, + }) }) } @@ -245,23 +247,24 @@ unsafe fn table_fill( val: *mut u8, len: u32, ) -> Result<(), Trap> { - let instance = (*vmctx).instance_mut(); - let table_index = TableIndex::from_u32(table_index); - let table = &mut *instance.get_table(table_index); - match table.element_type() { - TableElementType::Func => { - let val = val as *mut VMFuncRef; - table.fill(dst, val.into(), len) - } - TableElementType::Extern => { - let val = if val.is_null() { - None - } else { - Some(VMExternRef::clone_from_raw(val)) - }; - table.fill(dst, val.into(), len) + Instance::from_vmctx(vmctx, |instance| { + let table_index = TableIndex::from_u32(table_index); + let table = &mut *instance.get_table(table_index); + match table.element_type() { + TableElementType::Func => { + let val = val as *mut VMFuncRef; + table.fill(dst, val.into(), len) + } + TableElementType::Extern => { + let val = if val.is_null() { + None + } else { + Some(VMExternRef::clone_from_raw(val)) + }; + table.fill(dst, val.into(), len) + } } - } + }) } use table_fill as table_fill_func_ref; @@ -278,12 +281,13 @@ unsafe fn table_copy( ) -> Result<(), Trap> { let dst_table_index = TableIndex::from_u32(dst_table_index); let src_table_index = TableIndex::from_u32(src_table_index); - let instance = (*vmctx).instance_mut(); - let dst_table = instance.get_table(dst_table_index); - // Lazy-initialize the whole range in the source table first. - let src_range = src..(src.checked_add(len).unwrap_or(u32::MAX)); - let src_table = instance.get_table_with_lazy_init(src_table_index, src_range); - Table::copy(dst_table, src_table, dst, src, len) + Instance::from_vmctx(vmctx, |instance| { + let dst_table = instance.get_table(dst_table_index); + // Lazy-initialize the whole range in the source table first. + let src_range = src..(src.checked_add(len).unwrap_or(u32::MAX)); + let src_table = instance.get_table_with_lazy_init(src_table_index, src_range); + Table::copy(dst_table, src_table, dst, src, len) + }) } // Implementation of `table.init`. @@ -297,15 +301,15 @@ unsafe fn table_init( ) -> Result<(), Trap> { let table_index = TableIndex::from_u32(table_index); let elem_index = ElemIndex::from_u32(elem_index); - let instance = (*vmctx).instance_mut(); - instance.table_init(table_index, elem_index, dst, src, len) + Instance::from_vmctx(vmctx, |i| { + i.table_init(table_index, elem_index, dst, src, len) + }) } // Implementation of `elem.drop`. unsafe fn elem_drop(vmctx: *mut VMContext, elem_index: u32) { let elem_index = ElemIndex::from_u32(elem_index); - let instance = (*vmctx).instance_mut(); - instance.elem_drop(elem_index); + Instance::from_vmctx(vmctx, |i| i.elem_drop(elem_index)) } // Implementation of `memory.copy` for locally defined memories. @@ -319,8 +323,9 @@ unsafe fn memory_copy( ) -> Result<(), Trap> { let src_index = MemoryIndex::from_u32(src_index); let dst_index = MemoryIndex::from_u32(dst_index); - let instance = (*vmctx).instance_mut(); - instance.memory_copy(dst_index, dst, src_index, src, len) + Instance::from_vmctx(vmctx, |i| { + i.memory_copy(dst_index, dst, src_index, src, len) + }) } // Implementation of `memory.fill` for locally defined memories. @@ -332,8 +337,7 @@ unsafe fn memory_fill( len: u64, ) -> Result<(), Trap> { let memory_index = MemoryIndex::from_u32(memory_index); - let instance = (*vmctx).instance_mut(); - instance.memory_fill(memory_index, dst, val as u8, len) + Instance::from_vmctx(vmctx, |i| i.memory_fill(memory_index, dst, val as u8, len)) } // Implementation of `memory.init`. @@ -347,24 +351,25 @@ unsafe fn memory_init( ) -> Result<(), Trap> { let memory_index = MemoryIndex::from_u32(memory_index); let data_index = DataIndex::from_u32(data_index); - let instance = (*vmctx).instance_mut(); - instance.memory_init(memory_index, data_index, dst, src, len) + Instance::from_vmctx(vmctx, |i| { + i.memory_init(memory_index, data_index, dst, src, len) + }) } // Implementation of `ref.func`. unsafe fn ref_func(vmctx: *mut VMContext, func_index: u32) -> *mut u8 { - let instance = (*vmctx).instance_mut(); - let func_ref = instance - .get_func_ref(FuncIndex::from_u32(func_index)) - .expect("ref_func: `get_func_ref` should always be available for our given func index"); - func_ref as *mut _ + Instance::from_vmctx(vmctx, |instance| { + instance + .get_func_ref(FuncIndex::from_u32(func_index)) + .expect("ref_func: funcref should always be available for given func index") + .cast() + }) } // Implementation of `data.drop`. unsafe fn data_drop(vmctx: *mut VMContext, data_index: u32) { let data_index = DataIndex::from_u32(data_index); - let instance = (*vmctx).instance_mut(); - instance.data_drop(data_index) + Instance::from_vmctx(vmctx, |i| i.data_drop(data_index)) } // Returns a table entry after lazily initializing it. @@ -373,14 +378,15 @@ unsafe fn table_get_lazy_init_func_ref( table_index: u32, index: u32, ) -> *mut u8 { - let instance = (*vmctx).instance_mut(); - let table_index = TableIndex::from_u32(table_index); - let table = instance.get_table_with_lazy_init(table_index, std::iter::once(index)); - let elem = (*table) - .get(index) - .expect("table access already bounds-checked"); - - elem.into_ref_asserting_initialized() as *mut _ + Instance::from_vmctx(vmctx, |instance| { + let table_index = TableIndex::from_u32(table_index); + let table = instance.get_table_with_lazy_init(table_index, std::iter::once(index)); + let elem = (*table) + .get(index) + .expect("table access already bounds-checked"); + + elem.into_ref_asserting_initialized() as *mut u8 + }) } // Drop a `VMExternRef`. @@ -394,38 +400,41 @@ unsafe fn drop_externref(_vmctx: *mut VMContext, externref: *mut u8) { // `VMExternRefActivationsTable`. unsafe fn activations_table_insert_with_gc(vmctx: *mut VMContext, externref: *mut u8) { let externref = VMExternRef::clone_from_raw(externref); - let instance = (*vmctx).instance_mut(); - let limits = *instance.runtime_limits(); - let (activations_table, module_info_lookup) = (*instance.store()).externref_activations_table(); - - // Invariant: all `externref`s on the stack have an entry in the activations - // table. So we need to ensure that this `externref` is in the table - // *before* we GC, even though `insert_with_gc` will ensure that it is in - // the table *after* the GC. This technically results in one more hash table - // look up than is strictly necessary -- which we could avoid by having an - // additional GC method that is aware of these GC-triggering references -- - // but it isn't really a concern because this is already a slow path. - activations_table.insert_without_gc(externref.clone()); - - activations_table.insert_with_gc(limits, externref, module_info_lookup); + Instance::from_vmctx(vmctx, |instance| { + let limits = *instance.runtime_limits(); + let (activations_table, module_info_lookup) = + (*instance.store()).externref_activations_table(); + + // Invariant: all `externref`s on the stack have an entry in the activations + // table. So we need to ensure that this `externref` is in the table + // *before* we GC, even though `insert_with_gc` will ensure that it is in + // the table *after* the GC. This technically results in one more hash table + // look up than is strictly necessary -- which we could avoid by having an + // additional GC method that is aware of these GC-triggering references -- + // but it isn't really a concern because this is already a slow path. + activations_table.insert_without_gc(externref.clone()); + + activations_table.insert_with_gc(limits, externref, module_info_lookup); + }) } // Perform a Wasm `global.get` for `externref` globals. unsafe fn externref_global_get(vmctx: *mut VMContext, index: u32) -> *mut u8 { let index = GlobalIndex::from_u32(index); - let instance = (*vmctx).instance_mut(); - let limits = *instance.runtime_limits(); - let global = instance.defined_or_imported_global_ptr(index); - match (*global).as_externref().clone() { - None => ptr::null_mut(), - Some(externref) => { - let raw = externref.as_raw(); - let (activations_table, module_info_lookup) = - (*instance.store()).externref_activations_table(); - activations_table.insert_with_gc(limits, externref, module_info_lookup); - raw + Instance::from_vmctx(vmctx, |instance| { + let limits = *instance.runtime_limits(); + let global = instance.defined_or_imported_global_ptr(index); + match (*global).as_externref().clone() { + None => ptr::null_mut(), + Some(externref) => { + let raw = externref.as_raw(); + let (activations_table, module_info_lookup) = + (*instance.store()).externref_activations_table(); + activations_table.insert_with_gc(limits, externref, module_info_lookup); + raw + } } - } + }) } // Perform a Wasm `global.set` for `externref` globals. @@ -437,15 +446,16 @@ unsafe fn externref_global_set(vmctx: *mut VMContext, index: u32, externref: *mu }; let index = GlobalIndex::from_u32(index); - let instance = (*vmctx).instance_mut(); - let global = instance.defined_or_imported_global_ptr(index); - - // Swap the new `externref` value into the global before we drop the old - // value. This protects against an `externref` with a `Drop` implementation - // that calls back into Wasm and touches this global again (we want to avoid - // it observing a halfway-deinitialized value). - let old = mem::replace((*global).as_externref_mut(), externref); - drop(old); + Instance::from_vmctx(vmctx, |instance| { + let global = instance.defined_or_imported_global_ptr(index); + + // Swap the new `externref` value into the global before we drop the old + // value. This protects against an `externref` with a `Drop` implementation + // that calls back into Wasm and touches this global again (we want to avoid + // it observing a halfway-deinitialized value). + let old = mem::replace((*global).as_externref_mut(), externref); + drop(old); + }) } // Implementation of `memory.atomic.notify` for locally defined memories. @@ -456,10 +466,11 @@ unsafe fn memory_atomic_notify( count: u32, ) -> Result { let memory = MemoryIndex::from_u32(memory_index); - let instance = (*vmctx).instance_mut(); - instance - .get_runtime_memory(memory) - .atomic_notify(addr_index, count) + Instance::from_vmctx(vmctx, |instance| { + instance + .get_runtime_memory(memory) + .atomic_notify(addr_index, count) + }) } // Implementation of `memory.atomic.wait32` for locally defined memories. @@ -473,10 +484,11 @@ unsafe fn memory_atomic_wait32( // convert timeout to Instant, before any wait happens on locking let timeout = (timeout as i64 >= 0).then(|| Instant::now() + Duration::from_nanos(timeout)); let memory = MemoryIndex::from_u32(memory_index); - let instance = (*vmctx).instance_mut(); - Ok(instance - .get_runtime_memory(memory) - .atomic_wait32(addr_index, expected, timeout)? as u32) + Instance::from_vmctx(vmctx, |instance| { + Ok(instance + .get_runtime_memory(memory) + .atomic_wait32(addr_index, expected, timeout)? as u32) + }) } // Implementation of `memory.atomic.wait64` for locally defined memories. @@ -490,20 +502,21 @@ unsafe fn memory_atomic_wait64( // convert timeout to Instant, before any wait happens on locking let timeout = (timeout as i64 >= 0).then(|| Instant::now() + Duration::from_nanos(timeout)); let memory = MemoryIndex::from_u32(memory_index); - let instance = (*vmctx).instance_mut(); - Ok(instance - .get_runtime_memory(memory) - .atomic_wait64(addr_index, expected, timeout)? as u32) + Instance::from_vmctx(vmctx, |instance| { + Ok(instance + .get_runtime_memory(memory) + .atomic_wait64(addr_index, expected, timeout)? as u32) + }) } // Hook for when an instance runs out of fuel. unsafe fn out_of_gas(vmctx: *mut VMContext) -> Result<()> { - (*(*vmctx).instance().store()).out_of_gas() + Instance::from_vmctx(vmctx, |i| (*i.store()).out_of_gas()) } // Hook for when an instance observes that the epoch has changed. unsafe fn new_epoch(vmctx: *mut VMContext) -> Result { - (*(*vmctx).instance().store()).new_epoch() + Instance::from_vmctx(vmctx, |i| (*i.store()).new_epoch()) } /// This module contains functions which are used for resolving relocations at diff --git a/crates/runtime/src/memory.rs b/crates/runtime/src/memory.rs index 7802bb01a686..7eaea6500319 100644 --- a/crates/runtime/src/memory.rs +++ b/crates/runtime/src/memory.rs @@ -10,6 +10,7 @@ use anyhow::Error; use anyhow::{bail, format_err, Result}; use std::convert::TryFrom; use std::ops::Range; +use std::ptr::NonNull; use std::sync::atomic::{AtomicU32, AtomicU64, Ordering}; use std::sync::{Arc, RwLock}; use std::time::Instant; @@ -363,9 +364,12 @@ impl RuntimeLinearMemory for MmapMemory { /// A "static" memory where the lifetime of the backing memory is managed /// elsewhere. Currently used with the pooling allocator. struct StaticMemory { - /// The memory in the host for this wasm memory. The length of this - /// slice is the maximum size of the memory that can be grown to. - base: &'static mut [u8], + /// The base pointer of this static memory, wrapped up in a send/sync + /// wrapper. + base: RawBase, + + /// The byte capacity of the `base` pointer. + capacity: usize, /// The current size, in bytes, of this memory. size: usize, @@ -379,31 +383,38 @@ struct StaticMemory { memory_image: MemoryImageSlot, } +struct RawBase(NonNull); + +unsafe impl Send for RawBase {} +unsafe impl Sync for RawBase {} + impl StaticMemory { fn new( - base: &'static mut [u8], + base_ptr: *mut u8, + base_capacity: usize, initial_size: usize, maximum_size: Option, memory_image: MemoryImageSlot, memory_and_guard_size: usize, ) -> Result { - if base.len() < initial_size { + if base_capacity < initial_size { bail!( "initial memory size of {} exceeds the pooling allocator's \ configured maximum memory size of {} bytes", initial_size, - base.len(), + base_capacity, ); } // Only use the part of the slice that is necessary. - let base = match maximum_size { - Some(max) if max < base.len() => &mut base[..max], - _ => base, + let base_capacity = match maximum_size { + Some(max) if max < base_capacity => max, + _ => base_capacity, }; Ok(Self { - base, + base: RawBase(NonNull::new(base_ptr).unwrap()), + capacity: base_capacity, size: initial_size, memory_image, memory_and_guard_size, @@ -417,13 +428,13 @@ impl RuntimeLinearMemory for StaticMemory { } fn maximum_byte_size(&self) -> Option { - Some(self.base.len()) + Some(self.capacity) } fn grow_to(&mut self, new_byte_size: usize) -> Result<()> { // Never exceed the static memory size; this check should have been made // prior to arriving here. - assert!(new_byte_size <= self.base.len()); + assert!(new_byte_size <= self.capacity); self.memory_image.set_heap_limit(new_byte_size)?; @@ -434,7 +445,7 @@ impl RuntimeLinearMemory for StaticMemory { fn vmmemory(&mut self) -> VMMemoryDefinition { VMMemoryDefinition { - base: self.base.as_mut_ptr().cast(), + base: self.base.0.as_ptr(), current_length: self.size.into(), } } @@ -448,7 +459,7 @@ impl RuntimeLinearMemory for StaticMemory { } fn wasm_accessible(&self) -> Range { - let base = self.base.as_ptr() as usize; + let base = self.base.0.as_ptr() as usize; let end = base + self.memory_and_guard_size; base..end } @@ -682,14 +693,21 @@ impl Memory { /// Create a new static (immovable) memory instance for the specified plan. pub fn new_static( plan: &MemoryPlan, - base: &'static mut [u8], + base_ptr: *mut u8, + base_capacity: usize, memory_image: MemoryImageSlot, memory_and_guard_size: usize, store: &mut dyn Store, ) -> Result { let (minimum, maximum) = Self::limit_new(plan, Some(store))?; - let pooled_memory = - StaticMemory::new(base, minimum, maximum, memory_image, memory_and_guard_size)?; + let pooled_memory = StaticMemory::new( + base_ptr, + base_capacity, + minimum, + maximum, + memory_image, + memory_and_guard_size, + )?; let allocation = Box::new(pooled_memory); let allocation: Box = if plan.memory.shared { // FIXME: since the pooling allocator owns the memory allocation diff --git a/crates/runtime/src/traphandlers.rs b/crates/runtime/src/traphandlers.rs index 28a0ead6aeee..8dad271cf060 100644 --- a/crates/runtime/src/traphandlers.rs +++ b/crates/runtime/src/traphandlers.rs @@ -3,7 +3,7 @@ mod backtrace; -use crate::{VMContext, VMRuntimeLimits}; +use crate::{Instance, VMContext, VMRuntimeLimits}; use anyhow::Error; use std::any::Any; use std::cell::{Cell, UnsafeCell}; @@ -257,7 +257,7 @@ pub unsafe fn catch_traps<'a, F>( where F: FnMut(*mut VMContext), { - let limits = (*caller).instance_mut().runtime_limits(); + let limits = Instance::from_vmctx(caller, |i| i.runtime_limits()); let result = CallThreadState::new(signal_handler, capture_backtrace, *limits).with(|cx| { wasmtime_setjmp( diff --git a/crates/runtime/src/vmcontext.rs b/crates/runtime/src/vmcontext.rs index 1f10f9f45680..ae2c1f9154f6 100644 --- a/crates/runtime/src/vmcontext.rs +++ b/crates/runtime/src/vmcontext.rs @@ -4,8 +4,6 @@ mod vm_host_func_context; use crate::externref::VMExternRef; -use crate::instance::Instance; -use std::any::Any; use std::cell::UnsafeCell; use std::marker; use std::ptr::NonNull; @@ -966,32 +964,6 @@ impl VMContext { debug_assert_eq!((*opaque).magic, VMCONTEXT_MAGIC); opaque.cast() } - - /// Return a mutable reference to the associated `Instance`. - /// - /// # Safety - /// This is unsafe because it doesn't work on just any `VMContext`, it must - /// be a `VMContext` allocated as part of an `Instance`. - #[allow(clippy::cast_ptr_alignment)] - #[inline] - pub(crate) unsafe fn instance(&self) -> &Instance { - &*((self as *const Self as *mut u8).offset(-Instance::vmctx_offset()) as *const Instance) - } - - #[inline] - pub(crate) unsafe fn instance_mut(&mut self) -> &mut Instance { - &mut *((self as *const Self as *mut u8).offset(-Instance::vmctx_offset()) as *mut Instance) - } - - /// Return a reference to the host state associated with this `Instance`. - /// - /// # Safety - /// This is unsafe because it doesn't work on just any `VMContext`, it must - /// be a `VMContext` allocated as part of an `Instance`. - #[inline] - pub unsafe fn host_state(&self) -> &dyn Any { - self.instance().host_state() - } } /// A "raw" and unsafe representation of a WebAssembly value. diff --git a/crates/wasmtime/src/externals.rs b/crates/wasmtime/src/externals.rs index 264d8fc8422d..a9e1ebc8a94c 100644 --- a/crates/wasmtime/src/externals.rs +++ b/crates/wasmtime/src/externals.rs @@ -7,7 +7,7 @@ use crate::{ use anyhow::{anyhow, bail, Result}; use std::mem; use std::ptr; -use wasmtime_runtime::{self as runtime, InstanceHandle}; +use wasmtime_runtime::{self as runtime}; // Externals @@ -477,9 +477,10 @@ impl Table { ) -> *mut runtime::Table { unsafe { let export = &store[self.0]; - let mut handle = InstanceHandle::from_vmctx(export.vmctx); - let idx = handle.table_index(&*export.definition); - handle.get_defined_table_with_lazy_init(idx, lazy_init_range) + wasmtime_runtime::Instance::from_vmctx(export.vmctx, |handle| { + let idx = handle.table_index(&*export.definition); + handle.get_defined_table_with_lazy_init(idx, lazy_init_range) + }) } } diff --git a/crates/wasmtime/src/func.rs b/crates/wasmtime/src/func.rs index 906b80427514..d01b6c7ced44 100644 --- a/crates/wasmtime/src/func.rs +++ b/crates/wasmtime/src/func.rs @@ -11,8 +11,8 @@ use std::pin::Pin; use std::ptr::{self, NonNull}; use std::sync::Arc; use wasmtime_runtime::{ - ExportFunction, InstanceHandle, StoreBox, VMArrayCallHostFuncContext, VMContext, VMFuncRef, - VMFunctionImport, VMNativeCallHostFuncContext, VMOpaqueContext, VMSharedSignatureIndex, + ExportFunction, StoreBox, VMArrayCallHostFuncContext, VMContext, VMFuncRef, VMFunctionImport, + VMNativeCallHostFuncContext, VMOpaqueContext, VMSharedSignatureIndex, }; /// A WebAssembly function which can be called. @@ -1792,17 +1792,18 @@ pub trait IntoFunc: Send + Sync + 'static { /// recommended to use this type. pub struct Caller<'a, T> { pub(crate) store: StoreContextMut<'a, T>, - caller: &'a InstanceHandle, + caller: &'a wasmtime_runtime::Instance, } impl Caller<'_, T> { unsafe fn with(caller: *mut VMContext, f: impl FnOnce(Caller<'_, T>) -> R) -> R { assert!(!caller.is_null()); - let instance = InstanceHandle::from_vmctx(caller); - let store = StoreContextMut::from_raw(instance.store()); - f(Caller { - store, - caller: &instance, + wasmtime_runtime::Instance::from_vmctx(caller, |instance| { + let store = StoreContextMut::from_raw(instance.store()); + f(Caller { + store, + caller: &instance, + }) }) } diff --git a/crates/wasmtime/src/instance.rs b/crates/wasmtime/src/instance.rs index 4abc1825f30b..9350ffbd8bbc 100644 --- a/crates/wasmtime/src/instance.rs +++ b/crates/wasmtime/src/instance.rs @@ -340,7 +340,7 @@ impl Instance { // trap-handling configuration in `store` as well. let instance = store.0.instance_mut(id); let f = instance.get_exported_func(start); - let caller_vmctx = instance.vmctx_ptr(); + let caller_vmctx = instance.vmctx(); unsafe { super::func::invoke_wasm_and_catch_traps(store, |_default_caller| { let func = mem::transmute::< diff --git a/crates/wasmtime/src/memory.rs b/crates/wasmtime/src/memory.rs index 1d309f995e5c..c473fa143dd1 100644 --- a/crates/wasmtime/src/memory.rs +++ b/crates/wasmtime/src/memory.rs @@ -548,8 +548,9 @@ impl Memory { fn wasmtime_memory(&self, store: &mut StoreOpaque) -> *mut wasmtime_runtime::Memory { unsafe { let export = &store[self.0]; - let mut handle = wasmtime_runtime::InstanceHandle::from_vmctx(export.vmctx); - handle.get_defined_memory(export.index) + wasmtime_runtime::Instance::from_vmctx(export.vmctx, |handle| { + handle.get_defined_memory(export.index) + }) } } @@ -905,16 +906,17 @@ impl SharedMemory { wasmtime_export: wasmtime_runtime::ExportMemory, store: &mut StoreOpaque, ) -> Self { - let mut handle = wasmtime_runtime::InstanceHandle::from_vmctx(wasmtime_export.vmctx); - let memory = handle - .get_defined_memory(wasmtime_export.index) - .as_mut() - .unwrap(); - let shared_memory = memory - .as_shared_memory() - .expect("unable to convert from a shared memory") - .clone(); - Self(shared_memory, store.engine().clone()) + wasmtime_runtime::Instance::from_vmctx(wasmtime_export.vmctx, |handle| { + let memory = handle + .get_defined_memory(wasmtime_export.index) + .as_mut() + .unwrap(); + let shared_memory = memory + .as_shared_memory() + .expect("unable to convert from a shared memory") + .clone(); + Self(shared_memory, store.engine().clone()) + }) } } diff --git a/crates/wasmtime/src/store.rs b/crates/wasmtime/src/store.rs index 40771ed2df76..8ba337e43040 100644 --- a/crates/wasmtime/src/store.rs +++ b/crates/wasmtime/src/store.rs @@ -448,26 +448,6 @@ impl Store { /// tables created to 10,000. This can be overridden with the /// [`Store::limiter`] configuration method. pub fn new(engine: &Engine, data: T) -> Self { - // Wasmtime uses the callee argument to host functions to learn about - // the original pointer to the `Store` itself, allowing it to - // reconstruct a `StoreContextMut`. When we initially call a `Func`, - // however, there's no "callee" to provide. To fix this we allocate a - // single "default callee" for the entire `Store`. This is then used as - // part of `Func::call` to guarantee that the `callee: *mut VMContext` - // is never null. - let default_callee = { - let module = Arc::new(wasmtime_environ::Module::default()); - let shim = BareModuleInfo::empty(module).into_traitobj(); - OnDemandInstanceAllocator::default() - .allocate(InstanceAllocationRequest { - host_state: Box::new(()), - imports: Default::default(), - store: StorePtr::empty(), - runtime_info: &shim, - }) - .expect("failed to allocate default callee") - }; - let mut inner = Box::new(StoreInner { inner: StoreOpaque { _marker: marker::PhantomPinned, @@ -493,7 +473,7 @@ impl Store { }, out_of_gas_behavior: OutOfGas::Trap, store_data: ManuallyDrop::new(StoreData::new()), - default_caller: default_callee, + default_caller: InstanceHandle::null(), hostcall_val_storage: Vec::new(), wasm_val_raw_storage: Vec::new(), rooted_host_funcs: ManuallyDrop::new(Vec::new()), @@ -504,18 +484,38 @@ impl Store { data: ManuallyDrop::new(data), }); - // Once we've actually allocated the store itself we can configure the - // trait object pointer of the default callee. Note the erasure of the - // lifetime here into `'static`, so in general usage of this trait - // object must be strictly bounded to the `Store` itself, and is a - // variant that we have to maintain throughout Wasmtime. - unsafe { - let traitobj = std::mem::transmute::< - *mut (dyn wasmtime_runtime::Store + '_), - *mut (dyn wasmtime_runtime::Store + 'static), - >(&mut *inner); - inner.default_caller.set_store(traitobj); - } + // Wasmtime uses the callee argument to host functions to learn about + // the original pointer to the `Store` itself, allowing it to + // reconstruct a `StoreContextMut`. When we initially call a `Func`, + // however, there's no "callee" to provide. To fix this we allocate a + // single "default callee" for the entire `Store`. This is then used as + // part of `Func::call` to guarantee that the `callee: *mut VMContext` + // is never null. + inner.default_caller = { + let module = Arc::new(wasmtime_environ::Module::default()); + let shim = BareModuleInfo::empty(module).into_traitobj(); + let mut instance = OnDemandInstanceAllocator::default() + .allocate(InstanceAllocationRequest { + host_state: Box::new(()), + imports: Default::default(), + store: StorePtr::empty(), + runtime_info: &shim, + }) + .expect("failed to allocate default callee"); + + // Note the erasure of the lifetime here into `'static`, so in + // general usage of this trait object must be strictly bounded to + // the `Store` itself, and is a variant that we have to maintain + // throughout Wasmtime. + unsafe { + let traitobj = std::mem::transmute::< + *mut (dyn wasmtime_runtime::Store + '_), + *mut (dyn wasmtime_runtime::Store + 'static), + >(&mut *inner); + instance.set_store(traitobj); + } + instance + }; Self { inner: ManuallyDrop::new(inner), @@ -1395,7 +1395,7 @@ impl StoreOpaque { #[inline] pub fn default_caller(&self) -> *mut VMContext { - self.default_caller.vmctx_ptr() + self.default_caller.vmctx() } pub fn traitobj(&self) -> *mut dyn wasmtime_runtime::Store { diff --git a/supply-chain/audits.toml b/supply-chain/audits.toml index bac7f0824949..804a8cb8bd66 100644 --- a/supply-chain/audits.toml +++ b/supply-chain/audits.toml @@ -1190,6 +1190,16 @@ and does what it says on the tin, providing spin-based synchronization primitives. """ +[[audits.sptr]] +who = "Alex Crichton " +criteria = "safe-to-deploy" +version = "0.3.2" +notes = """ +This crate is 90% documentation and does contain a good deal of `unsafe` code, +but it's all doing what it says on the tin: being a stable polyfill for strict +provenance APIs in the standard library while they're on Nightly. +""" + [[audits.system-interface]] who = "Dan Gohman " criteria = "safe-to-deploy" From ecfe3a7ffa5affc1078308659c031d9c00bc3187 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Wed, 3 May 2023 08:08:59 -0700 Subject: [PATCH 02/11] Clean up manual `unsafe impl Send` impls This commit adds a new wrapper type `SendSyncPtr` which automatically impls the `Send` and `Sync` traits based on the `T` type contained. Otherwise it works similarly to `NonNull`. This helps clean up a number of manual annotations of `unsafe impl {Send,Sync} for ...` throughout the runtime. --- crates/runtime/src/component.rs | 24 ++----- crates/runtime/src/externref.rs | 31 ++++----- crates/runtime/src/instance.rs | 41 ++++------- crates/runtime/src/instance/allocator.rs | 6 +- crates/runtime/src/lib.rs | 10 +-- crates/runtime/src/libcalls.rs | 2 +- crates/runtime/src/memory.rs | 15 ++-- crates/runtime/src/mmap/miri.rs | 25 +++---- crates/runtime/src/mmap/unix.rs | 33 ++++----- crates/runtime/src/mmap/windows.rs | 31 +++++---- crates/runtime/src/send_sync_ptr.rs | 69 +++++++++++++++++++ .../src/vmcontext/vm_host_func_context.rs | 10 --- crates/wasmtime/src/func.rs | 55 +++------------ crates/wasmtime/src/store/func_refs.rs | 35 ++-------- 14 files changed, 179 insertions(+), 208 deletions(-) create mode 100644 crates/runtime/src/send_sync_ptr.rs diff --git a/crates/runtime/src/component.rs b/crates/runtime/src/component.rs index d65b0ce3a002..2b02fc5a984c 100644 --- a/crates/runtime/src/component.rs +++ b/crates/runtime/src/component.rs @@ -7,7 +7,7 @@ //! cranelift-compiled adapters, will use this `VMComponentContext` as well. use crate::{ - Store, VMArrayCallFunction, VMFuncRef, VMGlobalDefinition, VMMemoryDefinition, + SendSyncPtr, Store, VMArrayCallFunction, VMFuncRef, VMGlobalDefinition, VMMemoryDefinition, VMNativeCallFunction, VMOpaqueContext, VMSharedSignatureIndex, VMWasmCallFunction, ValRaw, }; use memoffset::offset_of; @@ -43,18 +43,13 @@ pub struct ComponentInstance { /// For more information about this see the documentation on /// `Instance::vmctx_self_reference`. - vmctx_self_reference: VMComponentContextSelfReference, + vmctx_self_reference: SendSyncPtr, /// A zero-sized field which represents the end of the struct for the actual /// `VMComponentContext` to be allocated behind. vmctx: VMComponentContext, } -struct VMComponentContextSelfReference(NonNull); - -unsafe impl Send for VMComponentContextSelfReference {} -unsafe impl Sync for VMComponentContextSelfReference {} - /// Type signature for host-defined trampolines that are called from /// WebAssembly. /// @@ -158,7 +153,7 @@ impl ComponentInstance { ptr.as_ptr(), ComponentInstance { offsets, - vmctx_self_reference: VMComponentContextSelfReference( + vmctx_self_reference: SendSyncPtr::new( NonNull::new( ptr.as_ptr() .cast::() @@ -178,7 +173,7 @@ impl ComponentInstance { fn vmctx(&self) -> *mut VMComponentContext { let addr = std::ptr::addr_of!(self.vmctx); - Strict::with_addr(self.vmctx_self_reference.0.as_ptr(), Strict::addr(addr)) + Strict::with_addr(self.vmctx_self_reference.as_ptr(), Strict::addr(addr)) } unsafe fn vmctx_plus_offset(&self, offset: u32) -> *const T { @@ -507,15 +502,9 @@ impl VMComponentContext { /// This type can be dereferenced to `ComponentInstance` to access the /// underlying methods. pub struct OwnedComponentInstance { - ptr: ptr::NonNull, + ptr: SendSyncPtr, } -// Using `NonNull` turns off auto-derivation of these traits but the owned usage -// here enables these trait impls so long as `ComponentInstance` itself -// implements these traits. -unsafe impl Send for OwnedComponentInstance where ComponentInstance: Send {} -unsafe impl Sync for OwnedComponentInstance where ComponentInstance: Sync {} - impl OwnedComponentInstance { /// Allocates a new `ComponentInstance + VMComponentContext` pair on the /// heap with `malloc` and configures it for the `component` specified. @@ -532,10 +521,11 @@ impl OwnedComponentInstance { // zeroed allocation is done here to try to contain // use-before-initialized issues. let ptr = alloc::alloc_zeroed(layout) as *mut ComponentInstance; - let ptr = ptr::NonNull::new(ptr).unwrap(); + let ptr = NonNull::new(ptr).unwrap(); ComponentInstance::new_at(ptr, layout.size(), offsets, store); + let ptr = SendSyncPtr::new(ptr); OwnedComponentInstance { ptr } } } diff --git a/crates/runtime/src/externref.rs b/crates/runtime/src/externref.rs index b2d2e10c9a6a..9aa67685f92e 100644 --- a/crates/runtime/src/externref.rs +++ b/crates/runtime/src/externref.rs @@ -99,7 +99,7 @@ //! Examination of Deferred Reference Counting and Cycle Detection* by Quinane: //! -use crate::{Backtrace, VMRuntimeLimits}; +use crate::{Backtrace, SendSyncPtr, VMRuntimeLimits}; use std::alloc::Layout; use std::any::Any; use std::cell::UnsafeCell; @@ -164,7 +164,7 @@ use wasmtime_environ::StackMap; /// ``` #[derive(Debug)] #[repr(transparent)] -pub struct VMExternRef(NonNull); +pub struct VMExternRef(SendSyncPtr); impl std::fmt::Pointer for VMExternRef { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { @@ -172,10 +172,6 @@ impl std::fmt::Pointer for VMExternRef { } } -// Data contained is always Send+Sync so these should be safe. -unsafe impl Send for VMExternRef {} -unsafe impl Sync for VMExternRef {} - #[repr(C)] pub(crate) struct VMExternData { // Implicit, dynamically-sized member that always preceded an @@ -195,7 +191,7 @@ pub(crate) struct VMExternData { /// Always points to the implicit, dynamically-sized `value` member that /// precedes this `VMExternData`. - value_ptr: NonNull, + value_ptr: SendSyncPtr, } impl Clone for VMExternRef { @@ -261,7 +257,7 @@ impl VMExternData { } /// Drop the inner value and then free this `VMExternData` heap allocation. - pub(crate) unsafe fn drop_and_dealloc(mut data: NonNull) { + pub(crate) unsafe fn drop_and_dealloc(mut data: SendSyncPtr) { log::trace!("Dropping externref data @ {:p}", data); // Note: we introduce a block scope so that we drop the live @@ -279,13 +275,13 @@ impl VMExternData { }; ptr::drop_in_place(data.value_ptr.as_ptr()); - let alloc_ptr = data.value_ptr.cast::(); + let alloc_ptr = data.value_ptr.as_ptr().cast::(); (alloc_ptr, layout) }; ptr::drop_in_place(data.as_ptr()); - std::alloc::dealloc(alloc_ptr.as_ptr(), layout); + std::alloc::dealloc(alloc_ptr, layout); } #[inline] @@ -336,17 +332,18 @@ impl VMExternRef { let extern_data_ptr = alloc_ptr.cast::().as_ptr().add(footer_offset) as *mut VMExternData; + ptr::write( extern_data_ptr, VMExternData { ref_count: AtomicUsize::new(1), // Cast from `*mut T` to `*mut dyn Any` here. - value_ptr: NonNull::new_unchecked(value_ptr.as_ptr()), + value_ptr: SendSyncPtr::new(NonNull::new_unchecked(value_ptr.as_ptr())), }, ); log::trace!("New externref data @ {:p}", extern_data_ptr); - VMExternRef(NonNull::new_unchecked(extern_data_ptr)) + VMExternRef(NonNull::new_unchecked(extern_data_ptr).into()) } } @@ -361,7 +358,7 @@ impl VMExternRef { /// `clone_from_raw` is called. #[inline] pub fn as_raw(&self) -> *mut u8 { - let ptr = self.0.cast::().as_ptr(); + let ptr = self.0.as_ptr().cast::(); ptr } @@ -374,7 +371,7 @@ impl VMExternRef { /// /// Use `from_raw` to recreate the `VMExternRef`. pub unsafe fn into_raw(self) -> *mut u8 { - let ptr = self.0.cast::().as_ptr(); + let ptr = self.0.as_ptr().cast::(); std::mem::forget(self); ptr } @@ -389,7 +386,7 @@ impl VMExternRef { /// function. pub unsafe fn from_raw(ptr: *mut u8) -> Self { debug_assert!(!ptr.is_null()); - VMExternRef(NonNull::new_unchecked(ptr).cast()) + VMExternRef(NonNull::new_unchecked(ptr).cast().into()) } /// Recreate a `VMExternRef` from a pointer returned from a previous call to @@ -405,7 +402,7 @@ impl VMExternRef { /// so will result in use after free! pub unsafe fn clone_from_raw(ptr: *mut u8) -> Self { debug_assert!(!ptr.is_null()); - let x = VMExternRef(NonNull::new_unchecked(ptr).cast()); + let x = VMExternRef(NonNull::new_unchecked(ptr).cast().into()); x.extern_data().increment_ref_count(); x } @@ -996,7 +993,7 @@ mod tests { let extern_data = VMExternData { ref_count: AtomicUsize::new(0), - value_ptr: NonNull::new(s).unwrap(), + value_ptr: NonNull::new(s).unwrap().into(), }; let extern_data_ptr = &extern_data as *const _; diff --git a/crates/runtime/src/instance.rs b/crates/runtime/src/instance.rs index 93e7bd639b6c..1bab636714be 100644 --- a/crates/runtime/src/instance.rs +++ b/crates/runtime/src/instance.rs @@ -12,8 +12,8 @@ use crate::vmcontext::{ VMTableDefinition, VMTableImport, }; use crate::{ - ExportFunction, ExportGlobal, ExportMemory, ExportTable, Imports, ModuleRuntimeInfo, Store, - VMFunctionBody, VMSharedSignatureIndex, WasmFault, + ExportFunction, ExportGlobal, ExportMemory, ExportTable, Imports, ModuleRuntimeInfo, + SendSyncPtr, Store, VMFunctionBody, VMSharedSignatureIndex, WasmFault, }; use anyhow::Error; use anyhow::Result; @@ -21,7 +21,6 @@ use sptr::Strict; use std::alloc::{self, Layout}; use std::any::Any; use std::convert::TryFrom; -use std::hash::Hash; use std::ops::Range; use std::ptr::NonNull; use std::sync::atomic::AtomicU64; @@ -139,7 +138,7 @@ pub struct Instance { /// the future if the memory consumption of this field is a problem we could /// shrink it slightly, but for now one extra pointer per wasm instance /// seems not too bad. - vmctx_self_reference: VMContextSelfReference, + vmctx_self_reference: SendSyncPtr, /// Additional context used by compiled wasm code. This field is last, and /// represents a dynamically-sized array that extends beyond the nominal @@ -147,11 +146,6 @@ pub struct Instance { vmctx: VMContext, } -struct VMContextSelfReference(NonNull); - -unsafe impl Send for VMContextSelfReference {} -unsafe impl Sync for VMContextSelfReference {} - #[allow(clippy::cast_ptr_alignment)] impl Instance { /// Create an instance at the given memory address. @@ -186,7 +180,7 @@ impl Instance { dropped_elements, dropped_data, host_state: req.host_state, - vmctx_self_reference: VMContextSelfReference( + vmctx_self_reference: SendSyncPtr::new( NonNull::new(ptr.cast::().add(mem::size_of::()).cast()).unwrap(), ), vmctx: VMContext { @@ -196,7 +190,9 @@ impl Instance { ); (*ptr).initialize_vmctx(module, req.runtime_info.offsets(), req.store, req.imports); - InstanceHandle { instance: ptr } + InstanceHandle { + instance: Some(SendSyncPtr::new(NonNull::new(ptr).unwrap())), + } } /// Converts the provided `*mut VMContext` to an `Instance` pointer and runs @@ -438,7 +434,7 @@ impl Instance { // trait `Strict` but the method names conflict with the nightly methods // so a different syntax is used to invoke methods here. let addr = std::ptr::addr_of!(self.vmctx); - Strict::with_addr(self.vmctx_self_reference.0.as_ptr(), Strict::addr(addr)) + Strict::with_addr(self.vmctx_self_reference.as_ptr(), Strict::addr(addr)) } fn get_exported_func(&mut self, index: FuncIndex) -> ExportFunction { @@ -1210,27 +1206,14 @@ impl Drop for Instance { } /// A handle holding an `Instance` of a WebAssembly module. -#[derive(Hash, PartialEq, Eq)] pub struct InstanceHandle { - instance: *mut Instance, -} - -// These are only valid if the `Instance` type is send/sync, hence the -// assertion below. -unsafe impl Send for InstanceHandle {} -unsafe impl Sync for InstanceHandle {} - -fn _assert_send_sync() { - fn _assert() {} - _assert::(); + instance: Option>, } impl InstanceHandle { /// TODO pub fn null() -> InstanceHandle { - InstanceHandle { - instance: ptr::null_mut(), - } + InstanceHandle { instance: None } } /// Return a raw pointer to the vmctx used by compiled wasm code. @@ -1307,11 +1290,11 @@ impl InstanceHandle { /// Return a reference to the contained `Instance`. #[inline] pub(crate) fn instance(&self) -> &Instance { - unsafe { &*(self.instance as *const Instance) } + unsafe { &*self.instance.unwrap().as_ptr() } } pub(crate) fn instance_mut(&mut self) -> &mut Instance { - unsafe { &mut *self.instance } + unsafe { &mut *self.instance.unwrap().as_ptr() } } /// Returns the `Store` pointer that was stored on creation diff --git a/crates/runtime/src/instance/allocator.rs b/crates/runtime/src/instance/allocator.rs index 3c5ab816ecbe..996806fb3ed1 100644 --- a/crates/runtime/src/instance/allocator.rs +++ b/crates/runtime/src/instance/allocator.rs @@ -133,9 +133,9 @@ pub unsafe trait InstanceAllocator { self.deallocate_tables(index, &mut handle.instance_mut().tables); unsafe { let layout = Instance::alloc_layout(handle.instance().offsets()); - ptr::drop_in_place(handle.instance); - alloc::dealloc(handle.instance.cast(), layout); - handle.instance = std::ptr::null_mut(); + let ptr = handle.instance.take().unwrap(); + ptr::drop_in_place(ptr.as_ptr()); + alloc::dealloc(ptr.as_ptr().cast(), layout); } self.deallocate_index(index); } diff --git a/crates/runtime/src/lib.rs b/crates/runtime/src/lib.rs index 6f7c0dc613fa..8ae55575f210 100644 --- a/crates/runtime/src/lib.rs +++ b/crates/runtime/src/lib.rs @@ -40,6 +40,7 @@ mod memory; mod mmap; mod mmap_vec; mod parking_spot; +mod send_sync_ptr; mod store_box; mod table; mod traphandlers; @@ -53,13 +54,13 @@ pub use wasmtime_jit_debug::gdb_jit_int::GdbJitImageRegistration; pub use crate::export::*; pub use crate::externref::*; pub use crate::imports::Imports; -#[cfg(feature = "pooling-allocator")] pub use crate::instance::{ - Instance, InstanceLimits, PoolingInstanceAllocator, PoolingInstanceAllocatorConfig, + Instance, InstanceAllocationRequest, InstanceAllocator, InstanceHandle, + OnDemandInstanceAllocator, StorePtr, }; +#[cfg(feature = "pooling-allocator")] pub use crate::instance::{ - InstanceAllocationRequest, InstanceAllocator, InstanceHandle, OnDemandInstanceAllocator, - StorePtr, + InstanceLimits, PoolingInstanceAllocator, PoolingInstanceAllocatorConfig, }; pub use crate::memory::{ DefaultMemoryCreator, Memory, RuntimeLinearMemory, RuntimeMemoryCreator, SharedMemory, @@ -79,6 +80,7 @@ pub use crate::vmcontext::{ VMRuntimeLimits, VMSharedSignatureIndex, VMTableDefinition, VMTableImport, VMWasmCallFunction, ValRaw, }; +pub use send_sync_ptr::SendSyncPtr; mod module_id; pub use module_id::{CompiledModuleId, CompiledModuleIdAllocator}; diff --git a/crates/runtime/src/libcalls.rs b/crates/runtime/src/libcalls.rs index 631501d95aee..81d14d83358a 100644 --- a/crates/runtime/src/libcalls.rs +++ b/crates/runtime/src/libcalls.rs @@ -392,7 +392,7 @@ unsafe fn table_get_lazy_init_func_ref( // Drop a `VMExternRef`. unsafe fn drop_externref(_vmctx: *mut VMContext, externref: *mut u8) { let externref = externref as *mut crate::externref::VMExternData; - let externref = NonNull::new(externref).unwrap(); + let externref = NonNull::new(externref).unwrap().into(); crate::externref::VMExternData::drop_and_dealloc(externref); } diff --git a/crates/runtime/src/memory.rs b/crates/runtime/src/memory.rs index 7eaea6500319..6927b931ada9 100644 --- a/crates/runtime/src/memory.rs +++ b/crates/runtime/src/memory.rs @@ -5,7 +5,7 @@ use crate::mmap::Mmap; use crate::parking_spot::ParkingSpot; use crate::vmcontext::VMMemoryDefinition; -use crate::{MemoryImage, MemoryImageSlot, Store, WaitResult}; +use crate::{MemoryImage, MemoryImageSlot, SendSyncPtr, Store, WaitResult}; use anyhow::Error; use anyhow::{bail, format_err, Result}; use std::convert::TryFrom; @@ -366,7 +366,7 @@ impl RuntimeLinearMemory for MmapMemory { struct StaticMemory { /// The base pointer of this static memory, wrapped up in a send/sync /// wrapper. - base: RawBase, + base: SendSyncPtr, /// The byte capacity of the `base` pointer. capacity: usize, @@ -383,11 +383,6 @@ struct StaticMemory { memory_image: MemoryImageSlot, } -struct RawBase(NonNull); - -unsafe impl Send for RawBase {} -unsafe impl Sync for RawBase {} - impl StaticMemory { fn new( base_ptr: *mut u8, @@ -413,7 +408,7 @@ impl StaticMemory { }; Ok(Self { - base: RawBase(NonNull::new(base_ptr).unwrap()), + base: SendSyncPtr::new(NonNull::new(base_ptr).unwrap()), capacity: base_capacity, size: initial_size, memory_image, @@ -445,7 +440,7 @@ impl RuntimeLinearMemory for StaticMemory { fn vmmemory(&mut self) -> VMMemoryDefinition { VMMemoryDefinition { - base: self.base.0.as_ptr(), + base: self.base.as_ptr(), current_length: self.size.into(), } } @@ -459,7 +454,7 @@ impl RuntimeLinearMemory for StaticMemory { } fn wasm_accessible(&self) -> Range { - let base = self.base.0.as_ptr() as usize; + let base = self.base.as_ptr() as usize; let end = base + self.memory_and_guard_size; base..end } diff --git a/crates/runtime/src/mmap/miri.rs b/crates/runtime/src/mmap/miri.rs index bb5e9e7511a7..5f4a117b9afe 100644 --- a/crates/runtime/src/mmap/miri.rs +++ b/crates/runtime/src/mmap/miri.rs @@ -5,23 +5,24 @@ //! like becoming executable or becoming readonly or being created from files, //! but it's enough to get various tests running relying on memories and such. +use crate::SendSyncPtr; use anyhow::{bail, Result}; use std::alloc::{self, Layout}; use std::fs::File; use std::ops::Range; use std::path::Path; +use std::ptr::NonNull; #[derive(Debug)] pub struct Mmap { - memory: *mut [u8], + memory: SendSyncPtr<[u8]>, } -unsafe impl Send for Mmap {} -unsafe impl Sync for Mmap {} - impl Mmap { pub fn new_empty() -> Mmap { - Mmap { memory: &mut [] } + Mmap { + memory: SendSyncPtr::from(&mut [][..]), + } } pub fn new(size: usize) -> Result { @@ -37,9 +38,9 @@ impl Mmap { bail!("failed to allocate memory"); } - Ok(Mmap { - memory: std::ptr::slice_from_raw_parts_mut(ptr, size), - }) + let memory = std::ptr::slice_from_raw_parts_mut(ptr.cast(), size); + let memory = SendSyncPtr::new(NonNull::new(memory).unwrap()); + Ok(Mmap { memory }) } pub fn from_file(_path: &Path) -> Result<(Self, File)> { @@ -50,21 +51,21 @@ impl Mmap { // The memory is technically always accessible but this marks it as // initialized for miri-level checking. unsafe { - std::ptr::write_bytes(self.memory.cast::().add(start), 0u8, len); + std::ptr::write_bytes(self.memory.as_ptr().cast::().add(start), 0u8, len); } Ok(()) } pub fn as_ptr(&self) -> *const u8 { - self.memory as *const u8 + self.memory.as_ptr() as *const u8 } pub fn as_mut_ptr(&mut self) -> *mut u8 { - self.memory.cast() + self.memory.as_ptr().cast() } pub fn len(&self) -> usize { - unsafe { (*self.memory).len() } + unsafe { (*self.memory.as_ptr()).len() } } pub unsafe fn make_executable( diff --git a/crates/runtime/src/mmap/unix.rs b/crates/runtime/src/mmap/unix.rs index a3252b35893b..742426e2a119 100644 --- a/crates/runtime/src/mmap/unix.rs +++ b/crates/runtime/src/mmap/unix.rs @@ -1,23 +1,21 @@ +use crate::SendSyncPtr; use anyhow::{anyhow, Context, Result}; use rustix::mm::{mprotect, MprotectFlags}; use std::fs::File; use std::ops::Range; use std::path::Path; -use std::ptr; +use std::ptr::{self, NonNull}; #[derive(Debug)] pub struct Mmap { - memory: *mut [u8], + memory: SendSyncPtr<[u8]>, } -// Mmaps are sendable and threadsafe, and otherwise fix the auto-traits on the -// `*mut [u8]` storage internally. -unsafe impl Send for Mmap {} -unsafe impl Sync for Mmap {} - impl Mmap { pub fn new_empty() -> Mmap { - Mmap { memory: &mut [] } + Mmap { + memory: SendSyncPtr::from(&mut [][..]), + } } pub fn new(size: usize) -> Result { @@ -30,6 +28,7 @@ impl Mmap { )? }; let memory = std::ptr::slice_from_raw_parts_mut(ptr.cast(), size); + let memory = SendSyncPtr::new(NonNull::new(memory).unwrap()); Ok(Mmap { memory }) } @@ -44,6 +43,7 @@ impl Mmap { }; let memory = std::ptr::slice_from_raw_parts_mut(ptr.cast(), size); + let memory = SendSyncPtr::new(NonNull::new(memory).unwrap()); Ok(Mmap { memory }) } @@ -66,12 +66,13 @@ impl Mmap { .context(format!("mmap failed to allocate {:#x} bytes", len))? }; let memory = std::ptr::slice_from_raw_parts_mut(ptr.cast(), len); + let memory = SendSyncPtr::new(NonNull::new(memory).unwrap()); Ok((Mmap { memory }, file)) } pub fn make_accessible(&mut self, start: usize, len: usize) -> Result<()> { - let ptr = self.memory.cast::(); + let ptr = self.memory.as_ptr().cast::(); unsafe { mprotect( ptr.add(start).cast(), @@ -84,15 +85,15 @@ impl Mmap { } pub fn as_ptr(&self) -> *const u8 { - self.memory as *const u8 + self.memory.as_ptr() as *const u8 } pub fn as_mut_ptr(&mut self) -> *mut u8 { - self.memory.cast() + self.memory.as_ptr().cast() } pub fn len(&self) -> usize { - unsafe { (*self.memory).len() } + unsafe { (*self.memory.as_ptr()).len() } } pub unsafe fn make_executable( @@ -100,7 +101,7 @@ impl Mmap { range: Range, enable_branch_protection: bool, ) -> Result<()> { - let base = self.memory.cast::().add(range.start).cast(); + let base = self.memory.as_ptr().cast::().add(range.start).cast(); let len = range.end - range.start; let flags = MprotectFlags::READ | MprotectFlags::EXEC; @@ -124,7 +125,7 @@ impl Mmap { } pub unsafe fn make_readonly(&self, range: Range) -> Result<()> { - let base = self.memory.cast::().add(range.start).cast(); + let base = self.memory.as_ptr().cast::().add(range.start).cast(); let len = range.end - range.start; mprotect(base, len, MprotectFlags::READ)?; @@ -136,8 +137,8 @@ impl Mmap { impl Drop for Mmap { fn drop(&mut self) { unsafe { - let ptr = self.memory.cast(); - let len = (*self.memory).len(); + let ptr = self.memory.as_ptr().cast(); + let len = (*self.memory.as_ptr()).len(); if len == 0 { return; } diff --git a/crates/runtime/src/mmap/windows.rs b/crates/runtime/src/mmap/windows.rs index a3f28b884e94..5ca208b337ac 100644 --- a/crates/runtime/src/mmap/windows.rs +++ b/crates/runtime/src/mmap/windows.rs @@ -1,29 +1,25 @@ +use crate::SendSyncPtr; use anyhow::{anyhow, bail, Context, Result}; use std::fs::{File, OpenOptions}; use std::io; use std::ops::Range; use std::os::windows::prelude::*; use std::path::Path; -use std::ptr; +use std::ptr::{self, NonNull}; use windows_sys::Win32::Foundation::*; use windows_sys::Win32::Storage::FileSystem::*; use windows_sys::Win32::System::Memory::*; #[derive(Debug)] pub struct Mmap { - memory: *mut [u8], + memory: SendSyncPtr<[u8]>, is_file: bool, } -// Mappings are threadsafe and this is fixing up the auto-traits from the usage -// of a raw pointer in the above structure. -unsafe impl Send for Mmap {} -unsafe impl Sync for Mmap {} - impl Mmap { pub fn new_empty() -> Mmap { Mmap { - memory: &mut [], + memory: SendSyncPtr::from(&mut [][..]), is_file: false, } } @@ -41,8 +37,10 @@ impl Mmap { bail!(io::Error::last_os_error()) } + let memory = std::ptr::slice_from_raw_parts_mut(ptr.cast(), size); + let memory = SendSyncPtr::new(NonNull::new(memory).unwrap()); Ok(Self { - memory: ptr::slice_from_raw_parts_mut(ptr.cast(), size), + memory, is_file: false, }) } @@ -52,9 +50,10 @@ impl Mmap { if ptr.is_null() { bail!(io::Error::last_os_error()) } - + let memory = std::ptr::slice_from_raw_parts_mut(ptr.cast(), size); + let memory = SendSyncPtr::new(NonNull::new(memory).unwrap()); Ok(Self { - memory: ptr::slice_from_raw_parts_mut(ptr.cast(), size), + memory, is_file: false, }) } @@ -112,8 +111,10 @@ impl Mmap { return Err(err).context(format!("failed to create map view of {:#x} bytes", len)); } + let memory = std::ptr::slice_from_raw_parts_mut(ptr.cast(), len); + let memory = SendSyncPtr::new(NonNull::new(memory).unwrap()); let mut ret = Self { - memory: ptr::slice_from_raw_parts_mut(ptr.cast(), len), + memory, is_file: true, }; @@ -147,15 +148,15 @@ impl Mmap { } pub fn as_ptr(&self) -> *const u8 { - self.memory as *const u8 + self.memory.as_ptr() as *const u8 } pub fn as_mut_ptr(&mut self) -> *mut u8 { - self.memory.cast() + self.memory.as_ptr().cast() } pub fn len(&self) -> usize { - unsafe { (*self.memory).len() } + unsafe { (*self.memory.as_ptr()).len() } } pub unsafe fn make_executable( diff --git a/crates/runtime/src/send_sync_ptr.rs b/crates/runtime/src/send_sync_ptr.rs new file mode 100644 index 000000000000..a8a129645de5 --- /dev/null +++ b/crates/runtime/src/send_sync_ptr.rs @@ -0,0 +1,69 @@ +use std::fmt; +use std::ptr::NonNull; + +/// A helper type in Wasmtime to store a raw pointer to `T` while automatically +/// inferring the `Send` and `Sync` traits for the container based on the +/// properties of `T`. +#[repr(transparent)] +pub struct SendSyncPtr(NonNull); + +unsafe impl Send for SendSyncPtr {} +unsafe impl Sync for SendSyncPtr {} + +impl SendSyncPtr { + /// Creates a new pointer wrapping the non-nullable pointer provided. + pub fn new(ptr: NonNull) -> SendSyncPtr { + SendSyncPtr(ptr) + } + + /// Returns the underlying raw pointer. + pub fn as_ptr(&self) -> *mut T { + self.0.as_ptr() + } + + /// Unsafely assert that this is a pointer to valid contents and it's also + /// valid to get a shared reference to it at this time. + pub unsafe fn as_ref<'a>(&self) -> &'a T { + self.0.as_ref() + } + + /// Unsafely assert that this is a pointer to valid contents and it's also + /// valid to get a mutable reference to it at this time. + pub unsafe fn as_mut<'a>(&mut self) -> &'a mut T { + self.0.as_mut() + } + + /// Returns the underlying `NonNull` wrapper. + pub fn as_non_null(&self) -> NonNull { + self.0 + } +} + +impl From for SendSyncPtr +where + U: Into>, +{ + fn from(ptr: U) -> SendSyncPtr { + SendSyncPtr::new(ptr.into()) + } +} + +impl fmt::Debug for SendSyncPtr { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + self.as_ptr().fmt(f) + } +} + +impl fmt::Pointer for SendSyncPtr { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + self.as_ptr().fmt(f) + } +} + +impl Clone for SendSyncPtr { + fn clone(&self) -> Self { + *self + } +} + +impl Copy for SendSyncPtr {} diff --git a/crates/runtime/src/vmcontext/vm_host_func_context.rs b/crates/runtime/src/vmcontext/vm_host_func_context.rs index e939dba4d45c..547e3dcacf21 100644 --- a/crates/runtime/src/vmcontext/vm_host_func_context.rs +++ b/crates/runtime/src/vmcontext/vm_host_func_context.rs @@ -21,11 +21,6 @@ pub struct VMArrayCallHostFuncContext { host_state: Box, } -// Declare that this type is send/sync, it's the responsibility of -// `VMHostFuncContext::new` callers to uphold this guarantee. -unsafe impl Send for VMArrayCallHostFuncContext {} -unsafe impl Sync for VMArrayCallHostFuncContext {} - impl VMArrayCallHostFuncContext { /// Create the context for the given host function. /// @@ -96,11 +91,6 @@ fn vmnative_call_host_func_context_offsets() { ); } -// Declare that this type is send/sync, it's the responsibility of -// `VMHostFuncContext::new` callers to uphold this guarantee. -unsafe impl Send for VMNativeCallHostFuncContext {} -unsafe impl Sync for VMNativeCallHostFuncContext {} - impl VMNativeCallHostFuncContext { /// Create the context for the given host function. /// diff --git a/crates/wasmtime/src/func.rs b/crates/wasmtime/src/func.rs index d01b6c7ced44..5f73a9acdb35 100644 --- a/crates/wasmtime/src/func.rs +++ b/crates/wasmtime/src/func.rs @@ -11,8 +11,8 @@ use std::pin::Pin; use std::ptr::{self, NonNull}; use std::sync::Arc; use wasmtime_runtime::{ - ExportFunction, StoreBox, VMArrayCallHostFuncContext, VMContext, VMFuncRef, VMFunctionImport, - VMNativeCallHostFuncContext, VMOpaqueContext, VMSharedSignatureIndex, + ExportFunction, SendSyncPtr, StoreBox, VMArrayCallHostFuncContext, VMContext, VMFuncRef, + VMFunctionImport, VMNativeCallHostFuncContext, VMOpaqueContext, VMSharedSignatureIndex, }; /// A WebAssembly function which can be called. @@ -191,7 +191,7 @@ pub(crate) struct FuncData { // `StoreOpaque::func_refs`, where we can safely patch the field without // worrying about synchronization and we hold a pointer to it here so we can // reuse it rather than re-copy if it is passed to Wasm again. - in_store_func_ref: Option, + in_store_func_ref: Option>, // This is somewhat expensive to load from the `Engine` and in most // optimized use cases (e.g. `TypedFunc`) it's not actually needed or it's @@ -203,34 +203,6 @@ pub(crate) struct FuncData { ty: Option>, } -use in_store_func_ref::InStoreFuncRef; -mod in_store_func_ref { - use super::*; - - #[derive(Copy, Clone)] - pub struct InStoreFuncRef(NonNull); - - impl InStoreFuncRef { - /// Create a new `InStoreFuncRef`. - /// - /// Safety: Callers must ensure that the given `func_ref` is pinned - /// inside a store, and that this resulting `InStoreFuncRef` is only - /// used in conjuction with that store and on its same thread. - pub unsafe fn new(func_ref: NonNull) -> InStoreFuncRef { - InStoreFuncRef(func_ref) - } - - pub fn func_ref(&self) -> NonNull { - self.0 - } - } - - // Safety: The `InStoreFuncRef::new` constructor puts the correctness - // responsibility on its callers. - unsafe impl Send for InStoreFuncRef {} - unsafe impl Sync for InStoreFuncRef {} -} - /// The three ways that a function can be created and referenced from within a /// store. enum FuncKind { @@ -1109,14 +1081,14 @@ impl Func { pub(crate) fn caller_checked_func_ref(&self, store: &mut StoreOpaque) -> NonNull { let func_data = &mut store.store_data_mut()[self.0]; if let Some(in_store) = func_data.in_store_func_ref { - in_store.func_ref() + in_store.as_non_null() } else { let func_ref = func_data.export().func_ref; unsafe { if func_ref.as_ref().wasm_call.is_none() { let func_ref = store.func_refs().push(func_ref.as_ref().clone()); store.store_data_mut()[self.0].in_store_func_ref = - Some(InStoreFuncRef::new(func_ref)); + Some(SendSyncPtr::new(func_ref)); store.fill_func_refs(); func_ref } else { @@ -1149,7 +1121,7 @@ impl Func { // copy in the store, use the patched version. Otherwise, use // the potentially un-patched version. if let Some(func_ref) = func_data.in_store_func_ref { - func_ref.func_ref() + func_ref.as_non_null() } else { func_data.export().func_ref } @@ -2330,7 +2302,7 @@ use self::rooted::*; /// `RootedHostFunc` instead of accidentally safely allowing access to its /// constructor. mod rooted { - use wasmtime_runtime::VMFuncRef; + use wasmtime_runtime::{SendSyncPtr, VMFuncRef}; use super::HostFunc; use std::ptr::NonNull; @@ -2342,15 +2314,10 @@ mod rooted { /// For more documentation see `FuncKind::RootedHost`, `InstancePre`, and /// `HostFunc::to_func_store_rooted`. pub(crate) struct RootedHostFunc { - func: NonNull, - func_ref: Option>, + func: SendSyncPtr, + func_ref: Option>, } - // These are required due to the usage of `NonNull` but should be safe - // because `HostFunc` is itself send/sync. - unsafe impl Send for RootedHostFunc where HostFunc: Send {} - unsafe impl Sync for RootedHostFunc where HostFunc: Sync {} - impl RootedHostFunc { /// Note that this is `unsafe` because this wrapper type allows safe /// access to the pointer given at any time, including outside the @@ -2364,8 +2331,8 @@ mod rooted { func_ref: Option>, ) -> RootedHostFunc { RootedHostFunc { - func: NonNull::from(&**func), - func_ref, + func: NonNull::from(&**func).into(), + func_ref: func_ref.map(|p| p.into()), } } diff --git a/crates/wasmtime/src/store/func_refs.rs b/crates/wasmtime/src/store/func_refs.rs index 814f680c6f7e..16ab2c2226a7 100644 --- a/crates/wasmtime/src/store/func_refs.rs +++ b/crates/wasmtime/src/store/func_refs.rs @@ -1,6 +1,6 @@ use crate::{instance::PrePatchedFuncRef, module::ModuleRegistry}; use std::{ptr::NonNull, sync::Arc}; -use wasmtime_runtime::{VMFuncRef, VMNativeCallHostFuncContext}; +use wasmtime_runtime::{SendSyncPtr, VMFuncRef, VMNativeCallHostFuncContext}; /// An arena of `VMFuncRef`s. /// @@ -15,7 +15,7 @@ pub struct FuncRefs { /// Pointers into `self.bump` for entries that need `wasm_call` field filled /// in. - with_holes: Vec, + with_holes: Vec>, /// Pinned `VMFuncRef`s that had their `wasm_call` field /// pre-patched when constructing an `InstancePre`, and which we need to @@ -40,31 +40,6 @@ mod send_sync_bump { unsafe impl Sync for SendSyncBump {} } -use unpatched_func_ref::UnpatchedFuncRef; -mod unpatched_func_ref { - use super::*; - - pub struct UnpatchedFuncRef(NonNull); - - impl UnpatchedFuncRef { - /// Safety: Callers must ensure that the given `func_ref` and resulting - /// wrapped value are used in a `Send + Sync` compatible way. - pub unsafe fn new(func_ref: &mut VMFuncRef) -> UnpatchedFuncRef { - debug_assert!(func_ref.wasm_call.is_none()); - UnpatchedFuncRef(NonNull::from(func_ref)) - } - - pub fn func_ref(&self) -> NonNull { - self.0 - } - } - - // Safety: It is `UnpatchedFuncRef::new` callers' responsibility to uphold - // this. - unsafe impl Send for UnpatchedFuncRef {} - unsafe impl Sync for UnpatchedFuncRef {} -} - impl FuncRefs { /// Push the given `VMFuncRef` into this arena, returning a /// pinned pointer to it. @@ -80,8 +55,8 @@ impl FuncRefs { let _ = unsafe { VMNativeCallHostFuncContext::from_opaque(func_ref.vmctx) }; let func_ref = self.bump.alloc(func_ref); - let unpatched = UnpatchedFuncRef::new(func_ref); - let ret = unpatched.func_ref(); + let unpatched = SendSyncPtr::from(func_ref); + let ret = unpatched.as_non_null(); self.with_holes.push(unpatched); ret } @@ -90,7 +65,7 @@ impl FuncRefs { pub fn fill(&mut self, modules: &ModuleRegistry) { self.with_holes.retain_mut(|f| { unsafe { - let func_ref = f.func_ref().as_mut(); + let func_ref = f.as_mut(); debug_assert!(func_ref.wasm_call.is_none()); // Debug assert that the vmctx is a `VMNativeCallHostFuncContext` as From 38c568e36c51081be61ecfeb757685ff0e1c2c6f Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Wed, 3 May 2023 08:55:16 -0700 Subject: [PATCH 03/11] Remove pointer-to-integer casts with tables In an effort to enable MIRI's "strict provenance" mode this commit removes the integer-to-pointer casts in the runtime `Table` implementation for Wasmtime. Most of the bits were already there to track all this, so this commit plumbed around the various pointer types and with the help of the `sptr` crate preserves the provenance of all related pointers. --- crates/runtime/src/table.rs | 67 +++++++++++++++++++++---------------- 1 file changed, 38 insertions(+), 29 deletions(-) diff --git a/crates/runtime/src/table.rs b/crates/runtime/src/table.rs index 391c5de0f874..03004aaafa1b 100644 --- a/crates/runtime/src/table.rs +++ b/crates/runtime/src/table.rs @@ -3,11 +3,12 @@ //! `Table` is to WebAssembly tables what `LinearMemory` is to WebAssembly linear memories. use crate::vmcontext::{VMFuncRef, VMTableDefinition}; -use crate::{Store, VMExternRef}; +use crate::{SendSyncPtr, Store, VMExternRef}; use anyhow::{bail, format_err, Error, Result}; +use sptr::Strict; use std::convert::{TryFrom, TryInto}; use std::ops::Range; -use std::ptr; +use std::ptr::{self, NonNull}; use wasmtime_environ::{TablePlan, Trap, WasmType, FUNCREF_INIT_BIT, FUNCREF_MASK}; /// An element going into or coming out of a table. @@ -45,13 +46,17 @@ impl TableElement { /// This is unsafe as it will *not* clone any externref, leaving the reference count unchanged. /// /// This should only be used if the raw pointer is no longer in use. - unsafe fn from_table_value(ty: TableElementType, ptr: usize) -> Self { + unsafe fn from_table_value(ty: TableElementType, ptr: TableValue) -> Self { match (ty, ptr) { - (TableElementType::Func, 0) => Self::UninitFunc, - (TableElementType::Func, ptr) => Self::FuncRef((ptr & FUNCREF_MASK) as _), - (TableElementType::Extern, 0) => Self::ExternRef(None), - (TableElementType::Extern, ptr) => { - Self::ExternRef(Some(VMExternRef::from_raw(ptr as *mut u8))) + (TableElementType::Func, None) => Self::UninitFunc, + (TableElementType::Func, Some(ptr)) => { + let ptr = ptr.as_ptr(); + let masked = Strict::map_addr(ptr, |a| a & FUNCREF_MASK); + Self::FuncRef(masked.cast()) + } + (TableElementType::Extern, None) => Self::ExternRef(None), + (TableElementType::Extern, Some(ptr)) => { + Self::ExternRef(Some(VMExternRef::from_raw(ptr.as_ptr()))) } } } @@ -61,13 +66,13 @@ impl TableElement { /// # Safety /// /// This is unsafe as it will clone any externref, incrementing the reference count. - unsafe fn clone_from_table_value(ty: TableElementType, ptr: usize) -> Self { - match (ty, ptr) { - (TableElementType::Func, 0) => Self::UninitFunc, - (TableElementType::Func, ptr) => Self::FuncRef((ptr & FUNCREF_MASK) as _), - (TableElementType::Extern, 0) => Self::ExternRef(None), - (TableElementType::Extern, ptr) => { - Self::ExternRef(Some(VMExternRef::clone_from_raw(ptr as *mut u8))) + unsafe fn clone_from_table_value(ty: TableElementType, ptr: TableValue) -> Self { + match ty { + // Functions have no ownership, so defer to the prior method. + TableElementType::Func => TableElement::from_table_value(ty, ptr), + + TableElementType::Extern => { + Self::ExternRef(ptr.map(|p| VMExternRef::clone_from_raw(p.as_ptr()))) } } } @@ -81,12 +86,14 @@ impl TableElement { /// This is unsafe as it will consume any underlying externref into a raw pointer without modifying /// the reference count. /// - /// Use `from_raw` to properly drop any table elements stored as raw pointers. - unsafe fn into_table_value(self) -> usize { + unsafe fn into_table_value(self) -> TableValue { match self { - Self::UninitFunc => 0, - Self::FuncRef(e) => (e as usize) | FUNCREF_INIT_BIT, - Self::ExternRef(e) => e.map_or(0, |e| e.into_raw() as usize), + Self::UninitFunc => None, + Self::FuncRef(e) => { + let tagged = Strict::map_addr(e, |e| e | FUNCREF_INIT_BIT); + Some(NonNull::new(tagged.cast()).unwrap().into()) + } + Self::ExternRef(e) => e.map(|e| NonNull::new(e.into_raw()).unwrap().into()), } } @@ -144,7 +151,7 @@ pub enum Table { Static { /// Where data for this table is stored. The length of this list is the /// maximum size of the table. - data: &'static mut [usize], + data: &'static mut [TableValue], /// The current size of the table. size: u32, /// The type of this table. @@ -155,7 +162,7 @@ pub enum Table { Dynamic { /// Dynamically managed storage space for this table. The length of this /// vector is the current size of the table. - elements: Vec, + elements: Vec, /// The type of this table. ty: TableElementType, /// Maximum size that `elements` can grow to. @@ -163,6 +170,8 @@ pub enum Table { }, } +pub type TableValue = Option>; + fn wasm_to_table_type(ty: WasmType) -> Result { match ty { WasmType::FuncRef => Ok(TableElementType::Func), @@ -175,7 +184,7 @@ impl Table { /// Create a new dynamic (movable) table instance for the specified table plan. pub fn new_dynamic(plan: &TablePlan, store: &mut dyn Store) -> Result { Self::limit_new(plan, store)?; - let elements = vec![0; plan.table.minimum as usize]; + let elements = vec![None; plan.table.minimum as usize]; let ty = wasm_to_table_type(plan.table.wasm_ty)?; let maximum = plan.table.maximum; @@ -189,7 +198,7 @@ impl Table { /// Create a new static (immovable) table instance for the specified table plan. pub fn new_static( plan: &TablePlan, - data: &'static mut [usize], + data: &'static mut [TableValue], store: &mut dyn Store, ) -> Result { Self::limit_new(plan, store)?; @@ -360,11 +369,11 @@ impl Table { Table::Static { size, data, .. } => { debug_assert!(data[*size as usize..new_size as usize] .iter() - .all(|x| *x == 0)); + .all(|x| x.is_none())); *size = new_size; } Table::Dynamic { elements, .. } => { - elements.resize(new_size as usize, 0); + elements.resize(new_size as usize, None); } } @@ -465,21 +474,21 @@ impl Table { } } - fn elements(&self) -> &[usize] { + fn elements(&self) -> &[TableValue] { match self { Table::Static { data, size, .. } => &data[..*size as usize], Table::Dynamic { elements, .. } => &elements[..], } } - fn elements_mut(&mut self) -> &mut [usize] { + fn elements_mut(&mut self) -> &mut [TableValue] { match self { Table::Static { data, size, .. } => &mut data[..*size as usize], Table::Dynamic { elements, .. } => &mut elements[..], } } - fn set_raw(ty: TableElementType, elem: &mut usize, val: TableElement) { + fn set_raw(ty: TableElementType, elem: &mut TableValue, val: TableElement) { unsafe { let old = *elem; *elem = val.into_table_value(); From 002aa3ab35077badb5675191d97067d6a031a2b7 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Wed, 3 May 2023 09:03:56 -0700 Subject: [PATCH 04/11] Remove integer-to-pointer casts in CoW management The `MemoryImageSlot` type stored a `base: usize` field mostly because I was too lazy to have a `Send`/`Sync` type as a pointer, so this commit updates it to use `SendSyncPtr` and then plumbs the pointer-ness throughout the implementation. This removes all integer-to-pointer casts and has pointers stores as actual pointers when they're at rest. --- crates/runtime/src/cow.rs | 61 +++++++++++++++++---------------------- 1 file changed, 27 insertions(+), 34 deletions(-) diff --git a/crates/runtime/src/cow.rs b/crates/runtime/src/cow.rs index 20fea79343f1..f3c9b387b51f 100644 --- a/crates/runtime/src/cow.rs +++ b/crates/runtime/src/cow.rs @@ -3,10 +3,11 @@ #![cfg_attr(any(not(unix), miri), allow(unused_imports, unused_variables))] -use crate::MmapVec; +use crate::{MmapVec, SendSyncPtr}; use anyhow::Result; use libc::c_void; use std::fs::File; +use std::ptr::NonNull; use std::sync::Arc; use std::{convert::TryFrom, ops::Range}; use wasmtime_environ::{ @@ -202,18 +203,18 @@ impl MemoryImage { } } - unsafe fn map_at(&self, base: usize) -> Result<()> { + unsafe fn map_at(&self, base: *mut u8) -> Result<()> { cfg_if::cfg_if! { if #[cfg(all(unix, not(miri)))] { let ptr = rustix::mm::mmap( - (base + self.linear_memory_offset) as *mut c_void, + base.add(self.linear_memory_offset).cast(), self.len, rustix::mm::ProtFlags::READ | rustix::mm::ProtFlags::WRITE, rustix::mm::MapFlags::PRIVATE | rustix::mm::MapFlags::FIXED, self.fd.as_file(), self.fd_offset, )?; - assert_eq!(ptr as usize, base + self.linear_memory_offset); + assert_eq!(ptr, base.add(self.linear_memory_offset).cast()); Ok(()) } else { match self.fd {} @@ -221,16 +222,16 @@ impl MemoryImage { } } - unsafe fn remap_as_zeros_at(&self, base: usize) -> Result<()> { + unsafe fn remap_as_zeros_at(&self, base: *mut u8) -> Result<()> { cfg_if::cfg_if! { if #[cfg(unix)] { let ptr = rustix::mm::mmap_anonymous( - (base + self.linear_memory_offset) as *mut c_void, + base.add(self.linear_memory_offset).cast(), self.len, rustix::mm::ProtFlags::READ | rustix::mm::ProtFlags::WRITE, rustix::mm::MapFlags::PRIVATE | rustix::mm::MapFlags::FIXED, )?; - assert_eq!(ptr as usize, base + self.linear_memory_offset); + assert_eq!(ptr.cast(), base.add(self.linear_memory_offset)); Ok(()) } else { match self.fd {} @@ -372,10 +373,7 @@ pub struct MemoryImageSlot { /// The base address in virtual memory of the actual heap memory. /// /// Bytes at this address are what is seen by the Wasm guest code. - /// - /// Note that this is stored as `usize` instead of `*mut u8` to not deal - /// with `Send`/`Sync. - base: usize, + base: SendSyncPtr, /// The maximum static memory size which `self.accessible` can grow to. static_size: usize, @@ -424,9 +422,8 @@ impl MemoryImageSlot { /// and all memory from `accessible` from `static_size` should be mapped as /// `PROT_NONE` backed by zero-bytes. pub(crate) fn create(base_addr: *mut c_void, accessible: usize, static_size: usize) -> Self { - let base = base_addr as usize; MemoryImageSlot { - base, + base: NonNull::new(base_addr.cast()).unwrap().into(), static_size, accessible, image: None, @@ -438,7 +435,7 @@ impl MemoryImageSlot { #[cfg(feature = "pooling-allocator")] pub(crate) fn dummy() -> MemoryImageSlot { MemoryImageSlot { - base: 0, + base: NonNull::new(sptr::invalid_mut(1)).unwrap().into(), static_size: 0, image: None, accessible: 0, @@ -547,7 +544,7 @@ impl MemoryImageSlot { ); if image.len > 0 { unsafe { - image.map_at(self.base)?; + image.map_at(self.base.as_ptr())?; } } } @@ -564,7 +561,7 @@ impl MemoryImageSlot { pub(crate) fn remove_image(&mut self) -> Result<()> { if let Some(image) = &self.image { unsafe { - image.remap_as_zeros_at(self.base)?; + image.remap_as_zeros_at(self.base.as_ptr())?; } self.image = None; } @@ -642,17 +639,13 @@ impl MemoryImageSlot { (keep_resident - image.linear_memory_offset).min(mem_after_image); // This is memset (1) - std::ptr::write_bytes(self.base as *mut u8, 0u8, image.linear_memory_offset); + std::ptr::write_bytes(self.base.as_ptr(), 0u8, image.linear_memory_offset); // This is madvise (2) self.madvise_reset(image.linear_memory_offset, image.len)?; // This is memset (3) - std::ptr::write_bytes( - (self.base + image_end) as *mut u8, - 0u8, - remaining_memset, - ); + std::ptr::write_bytes(self.base.as_ptr().add(image_end), 0u8, remaining_memset); // This is madvise (4) self.madvise_reset( @@ -680,7 +673,7 @@ impl MemoryImageSlot { // Note that the memset may be zero bytes here. // This is memset (1) - std::ptr::write_bytes(self.base as *mut u8, 0u8, keep_resident); + std::ptr::write_bytes(self.base.as_ptr(), 0u8, keep_resident); // This is madvise (2) self.madvise_reset(keep_resident, self.accessible - keep_resident)?; @@ -692,7 +685,7 @@ impl MemoryImageSlot { // the rest. None => { let size_to_memset = keep_resident.min(self.accessible); - std::ptr::write_bytes(self.base as *mut u8, 0u8, size_to_memset); + std::ptr::write_bytes(self.base.as_ptr(), 0u8, size_to_memset); self.madvise_reset(size_to_memset, self.accessible - size_to_memset)?; } } @@ -709,7 +702,7 @@ impl MemoryImageSlot { cfg_if::cfg_if! { if #[cfg(target_os = "linux")] { rustix::mm::madvise( - (self.base + base) as *mut c_void, + self.base.as_ptr().add(base).cast(), len, rustix::mm::Advice::LinuxDontNeed, )?; @@ -723,16 +716,16 @@ impl MemoryImageSlot { fn set_protection(&self, range: Range, readwrite: bool) -> Result<()> { assert!(range.start <= range.end); assert!(range.end <= self.static_size); - let start = self.base.checked_add(range.start).unwrap(); if range.len() == 0 { return Ok(()); } unsafe { + let start = self.base.as_ptr().add(range.start); cfg_if::cfg_if! { if #[cfg(miri)] { if readwrite { - std::ptr::write_bytes(start as *mut u8, 0u8, range.len()); + std::ptr::write_bytes(start, 0u8, range.len()); } } else if #[cfg(unix)] { let flags = if readwrite { @@ -740,14 +733,14 @@ impl MemoryImageSlot { } else { rustix::mm::MprotectFlags::empty() }; - rustix::mm::mprotect(start as *mut _, range.len(), flags)?; + rustix::mm::mprotect(start.cast(), range.len(), flags)?; } else { use windows_sys::Win32::System::Memory::*; let failure = if readwrite { - VirtualAlloc(start as _, range.len(), MEM_COMMIT, PAGE_READWRITE).is_null() + VirtualAlloc(start.cast(), range.len(), MEM_COMMIT, PAGE_READWRITE).is_null() } else { - VirtualFree(start as _, range.len(), MEM_DECOMMIT) == 0 + VirtualFree(start.cast(), range.len(), MEM_DECOMMIT) == 0 }; if failure { return Err(std::io::Error::last_os_error().into()); @@ -780,18 +773,18 @@ impl MemoryImageSlot { unsafe { cfg_if::cfg_if! { if #[cfg(miri)] { - std::ptr::write_bytes(self.base as *mut u8, 0, self.static_size); + std::ptr::write_bytes(self.base.as_ptr(), 0, self.static_size); } else if #[cfg(unix)] { let ptr = rustix::mm::mmap_anonymous( - self.base as *mut c_void, + self.base.as_ptr().cast(), self.static_size, rustix::mm::ProtFlags::empty(), rustix::mm::MapFlags::PRIVATE | rustix::mm::MapFlags::FIXED, )?; - assert_eq!(ptr as usize, self.base); + assert_eq!(ptr, self.base.as_ptr().cast()); } else { use windows_sys::Win32::System::Memory::*; - if VirtualFree(self.base as _, self.static_size, MEM_DECOMMIT) == 0 { + if VirtualFree(self.base.as_ptr().cast(), self.static_size, MEM_DECOMMIT) == 0 { return Err(std::io::Error::last_os_error().into()); } } From 106fb15adec110e8246af475bd78b82f01b401cc Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Wed, 3 May 2023 09:49:28 -0700 Subject: [PATCH 05/11] Remove pointer-to-integer casts in "raw" representations This commit changes the "raw" representation of `Func` and `ExternRef` to a `*mut c_void` instead of the previous `usize`. This is done to satisfy MIRI's requirements with strict provenance, properly marking the intermediate value as a pointer rather than round-tripping through integers. --- crates/c-api/include/wasmtime/func.h | 4 +- crates/c-api/include/wasmtime/val.h | 8 +- crates/c-api/src/func.rs | 7 +- crates/c-api/src/val.rs | 6 +- crates/runtime/src/instance.rs | 2 +- crates/runtime/src/vmcontext.rs | 41 +++++----- crates/wasmtime/src/externals.rs | 4 +- crates/wasmtime/src/func.rs | 10 ++- crates/wasmtime/src/func/typed.rs | 4 +- crates/wasmtime/src/ref.rs | 9 ++- crates/wasmtime/src/values.rs | 4 +- tests/all/func.rs | 113 +++++++++++++++++++++++++++ 12 files changed, 168 insertions(+), 44 deletions(-) diff --git a/crates/c-api/include/wasmtime/func.h b/crates/c-api/include/wasmtime/func.h index 0c86506f55c7..c5b927fd7923 100644 --- a/crates/c-api/include/wasmtime/func.h +++ b/crates/c-api/include/wasmtime/func.h @@ -296,14 +296,14 @@ WASM_API_EXTERN wasmtime_context_t* wasmtime_caller_context(wasmtime_caller_t* c */ WASM_API_EXTERN void wasmtime_func_from_raw( wasmtime_context_t* context, - size_t raw, + void *raw, wasmtime_func_t *ret); /** * \brief Converts a `func` which belongs to `context` into a `usize` * parameter that is suitable for insertion into a #wasmtime_val_raw_t. */ -WASM_API_EXTERN size_t wasmtime_func_to_raw( +WASM_API_EXTERN void *wasmtime_func_to_raw( wasmtime_context_t* context, const wasmtime_func_t *func); diff --git a/crates/c-api/include/wasmtime/val.h b/crates/c-api/include/wasmtime/val.h index f16c7bd48e40..bf3f748b1a7d 100644 --- a/crates/c-api/include/wasmtime/val.h +++ b/crates/c-api/include/wasmtime/val.h @@ -70,7 +70,7 @@ WASM_API_EXTERN void wasmtime_externref_delete(wasmtime_externref_t *ref); * Note that the returned #wasmtime_externref_t is an owned value that must be * deleted via #wasmtime_externref_delete by the caller if it is non-null. */ -WASM_API_EXTERN wasmtime_externref_t *wasmtime_externref_from_raw(wasmtime_context_t *context, size_t raw); +WASM_API_EXTERN wasmtime_externref_t *wasmtime_externref_from_raw(wasmtime_context_t *context, void *raw); /** * \brief Converts a #wasmtime_externref_t to a raw value suitable for storing @@ -82,7 +82,7 @@ WASM_API_EXTERN wasmtime_externref_t *wasmtime_externref_from_raw(wasmtime_conte * context of the store. Do not perform a GC between calling this function and * passing it to WebAssembly. */ -WASM_API_EXTERN size_t wasmtime_externref_to_raw( +WASM_API_EXTERN void *wasmtime_externref_to_raw( wasmtime_context_t *context, const wasmtime_externref_t *ref); @@ -180,7 +180,7 @@ typedef union wasmtime_val_raw { /// passed to `wasmtime_func_from_raw` to determine the `wasmtime_func_t`. /// /// Note that this field is always stored in a little-endian format. - size_t funcref; + void *funcref; /// Field for when this val is a WebAssembly `externref` value. /// /// If this is set to 0 then it's a null externref, otherwise this must be @@ -188,7 +188,7 @@ typedef union wasmtime_val_raw { /// `wasmtime_externref_t`. /// /// Note that this field is always stored in a little-endian format. - size_t externref; + void *externref; } wasmtime_val_raw_t; /** diff --git a/crates/c-api/src/func.rs b/crates/c-api/src/func.rs index eb01f8bbea51..2e4713f6236d 100644 --- a/crates/c-api/src/func.rs +++ b/crates/c-api/src/func.rs @@ -420,13 +420,16 @@ pub unsafe extern "C" fn wasmtime_caller_export_get( #[no_mangle] pub unsafe extern "C" fn wasmtime_func_from_raw( store: CStoreContextMut<'_>, - raw: usize, + raw: *mut c_void, func: &mut Func, ) { *func = Func::from_raw(store, raw).unwrap(); } #[no_mangle] -pub unsafe extern "C" fn wasmtime_func_to_raw(store: CStoreContextMut<'_>, func: &Func) -> usize { +pub unsafe extern "C" fn wasmtime_func_to_raw( + store: CStoreContextMut<'_>, + func: &Func, +) -> *mut c_void { func.to_raw(store) } diff --git a/crates/c-api/src/val.rs b/crates/c-api/src/val.rs index a98ece8bfeb8..904a2093cc87 100644 --- a/crates/c-api/src/val.rs +++ b/crates/c-api/src/val.rs @@ -296,17 +296,17 @@ pub extern "C" fn wasmtime_externref_delete(_val: Option) {} pub unsafe extern "C" fn wasmtime_externref_to_raw( cx: CStoreContextMut<'_>, val: Option>, -) -> usize { +) -> *mut c_void { match val { Some(ptr) => ptr.to_raw(cx), - None => 0, + None => ptr::null_mut(), } } #[no_mangle] pub unsafe extern "C" fn wasmtime_externref_from_raw( _cx: CStoreContextMut<'_>, - val: usize, + val: *mut c_void, ) -> Option { ExternRef::from_raw(val) } diff --git a/crates/runtime/src/instance.rs b/crates/runtime/src/instance.rs index 1bab636714be..ac1fa3d9e4c7 100644 --- a/crates/runtime/src/instance.rs +++ b/crates/runtime/src/instance.rs @@ -1153,7 +1153,7 @@ impl Instance { } } GlobalInit::RefFunc(f) => { - *(*to).as_func_ref_mut() = self.get_func_ref(f).unwrap() as *const VMFuncRef; + *(*to).as_func_ref_mut() = self.get_func_ref(f).unwrap(); } GlobalInit::RefNullConst => match global.wasm_ty { // `VMGlobalDefinition::new()` already zeroed out the bits diff --git a/crates/runtime/src/vmcontext.rs b/crates/runtime/src/vmcontext.rs index ae2c1f9154f6..652c6c7fc7ae 100644 --- a/crates/runtime/src/vmcontext.rs +++ b/crates/runtime/src/vmcontext.rs @@ -4,7 +4,9 @@ mod vm_host_func_context; use crate::externref::VMExternRef; +use sptr::Strict; use std::cell::UnsafeCell; +use std::ffi::c_void; use std::marker; use std::ptr::NonNull; use std::sync::atomic::{AtomicUsize, Ordering}; @@ -568,18 +570,14 @@ impl VMGlobalDefinition { /// Return a reference to the value as a `VMFuncRef`. #[allow(clippy::cast_ptr_alignment)] - pub unsafe fn as_func_ref(&self) -> *const VMFuncRef { - *(self.storage.as_ref().as_ptr().cast::<*const VMFuncRef>()) + pub unsafe fn as_func_ref(&self) -> *mut VMFuncRef { + *(self.storage.as_ref().as_ptr().cast::<*mut VMFuncRef>()) } /// Return a mutable reference to the value as a `VMFuncRef`. #[allow(clippy::cast_ptr_alignment)] - pub unsafe fn as_func_ref_mut(&mut self) -> &mut *const VMFuncRef { - &mut *(self - .storage - .as_mut() - .as_mut_ptr() - .cast::<*const VMFuncRef>()) + pub unsafe fn as_func_ref_mut(&mut self) -> &mut *mut VMFuncRef { + &mut *(self.storage.as_mut().as_mut_ptr().cast::<*mut VMFuncRef>()) } } @@ -1036,7 +1034,7 @@ pub union ValRaw { /// carefully calling the correct functions throughout the runtime. /// /// This value is always stored in a little-endian format. - funcref: usize, + funcref: *mut c_void, /// A WebAssembly `externref` value. /// @@ -1046,9 +1044,14 @@ pub union ValRaw { /// carefully calling the correct functions throughout the runtime. /// /// This value is always stored in a little-endian format. - externref: usize, + externref: *mut c_void, } +// This type is just a bag-of-bits so it's up to the caller to figure out how +// to safely deal with threading concerns and safely access interior bits. +unsafe impl Send for ValRaw {} +unsafe impl Sync for ValRaw {} + impl ValRaw { /// Creates a WebAssembly `i32` value #[inline] @@ -1104,15 +1107,17 @@ impl ValRaw { /// Creates a WebAssembly `funcref` value #[inline] - pub fn funcref(i: usize) -> ValRaw { - ValRaw { funcref: i.to_le() } + pub fn funcref(i: *mut c_void) -> ValRaw { + ValRaw { + funcref: Strict::map_addr(i, |i| i.to_le()), + } } /// Creates a WebAssembly `externref` value #[inline] - pub fn externref(i: usize) -> ValRaw { + pub fn externref(i: *mut c_void) -> ValRaw { ValRaw { - externref: i.to_le(), + externref: Strict::map_addr(i, |i| i.to_le()), } } @@ -1160,14 +1165,14 @@ impl ValRaw { /// Gets the WebAssembly `funcref` value #[inline] - pub fn get_funcref(&self) -> usize { - unsafe { usize::from_le(self.funcref) } + pub fn get_funcref(&self) -> *mut c_void { + unsafe { Strict::map_addr(self.funcref, |i| usize::from_le(i)) } } /// Gets the WebAssembly `externref` value #[inline] - pub fn get_externref(&self) -> usize { - unsafe { usize::from_le(self.externref) } + pub fn get_externref(&self) -> *mut c_void { + unsafe { Strict::map_addr(self.externref, |i| usize::from_le(i)) } } } diff --git a/crates/wasmtime/src/externals.rs b/crates/wasmtime/src/externals.rs index a9e1ebc8a94c..0e13c5669d6f 100644 --- a/crates/wasmtime/src/externals.rs +++ b/crates/wasmtime/src/externals.rs @@ -280,7 +280,7 @@ impl Global { .map(|inner| ExternRef { inner }), ), ValType::FuncRef => { - Val::FuncRef(Func::from_raw(store, definition.as_func_ref() as usize)) + Val::FuncRef(Func::from_raw(store, definition.as_func_ref().cast())) } ValType::V128 => Val::V128(*definition.as_u128()), } @@ -319,7 +319,7 @@ impl Global { Val::F32(f) => *definition.as_u32_mut() = f, Val::F64(f) => *definition.as_u64_mut() = f, Val::FuncRef(f) => { - *definition.as_func_ref_mut() = f.map_or(ptr::null(), |f| { + *definition.as_func_ref_mut() = f.map_or(ptr::null_mut(), |f| { f.caller_checked_func_ref(store).as_ptr().cast() }); } diff --git a/crates/wasmtime/src/func.rs b/crates/wasmtime/src/func.rs index 5f73a9acdb35..8f77ab1e3555 100644 --- a/crates/wasmtime/src/func.rs +++ b/crates/wasmtime/src/func.rs @@ -4,6 +4,7 @@ use crate::{ StoreContextMut, Val, ValRaw, ValType, }; use anyhow::{bail, Context as _, Error, Result}; +use std::ffi::c_void; use std::future::Future; use std::mem; use std::panic::{self, AssertUnwindSafe}; @@ -930,8 +931,8 @@ impl Func { /// This function is not safe because `raw` is not validated at all. The /// caller must guarantee that `raw` is owned by the `store` provided and is /// valid within the `store`. - pub unsafe fn from_raw(mut store: impl AsContextMut, raw: usize) -> Option { - Func::from_caller_checked_func_ref(store.as_context_mut().0, raw as *mut _) + pub unsafe fn from_raw(mut store: impl AsContextMut, raw: *mut c_void) -> Option { + Func::from_caller_checked_func_ref(store.as_context_mut().0, raw.cast()) } /// Extracts the raw value of this `Func`, which is owned by `store`. @@ -944,9 +945,10 @@ impl Func { /// The returned value is only valid for as long as the store is alive and /// this function is properly rooted within it. Additionally this function /// should not be liberally used since it's a very low-level knob. - pub unsafe fn to_raw(&self, mut store: impl AsContextMut) -> usize { + pub unsafe fn to_raw(&self, mut store: impl AsContextMut) -> *mut c_void { self.caller_checked_func_ref(store.as_context_mut().0) - .as_ptr() as usize + .as_ptr() + .cast() } /// Invokes this function with the `params` given, returning the results diff --git a/crates/wasmtime/src/func/typed.rs b/crates/wasmtime/src/func/typed.rs index c4570a633d8f..8229e14585b3 100644 --- a/crates/wasmtime/src/func/typed.rs +++ b/crates/wasmtime/src/func/typed.rs @@ -347,7 +347,7 @@ unsafe impl WasmTy for Option { #[inline] unsafe fn abi_into_raw(abi: *mut u8, raw: *mut ValRaw) { - *raw = ValRaw::externref(abi as usize); + *raw = ValRaw::externref(abi.cast()); } #[inline] @@ -433,7 +433,7 @@ unsafe impl WasmTy for Option { #[inline] unsafe fn abi_into_raw(abi: Self::Abi, raw: *mut ValRaw) { - *raw = ValRaw::funcref(abi as usize); + *raw = ValRaw::funcref(abi.cast()); } #[inline] diff --git a/crates/wasmtime/src/ref.rs b/crates/wasmtime/src/ref.rs index 7845a4f7ea7d..e04cbea2a2cf 100644 --- a/crates/wasmtime/src/ref.rs +++ b/crates/wasmtime/src/ref.rs @@ -2,6 +2,7 @@ use crate::AsContextMut; use std::any::Any; +use std::ffi::c_void; use wasmtime_runtime::VMExternRef; /// Represents an opaque reference to any data within WebAssembly. @@ -70,8 +71,8 @@ impl ExternRef { /// [`Store`]: crate::Store /// [`TypedFunc`]: crate::TypedFunc /// [`ValRaw`]: crate::ValRaw - pub unsafe fn from_raw(raw: usize) -> Option { - let raw = raw as *mut u8; + pub unsafe fn from_raw(raw: *mut c_void) -> Option { + let raw = raw.cast::(); if raw.is_null() { None } else { @@ -91,13 +92,13 @@ impl ExternRef { /// into the store. /// /// [`ValRaw`]: crate::ValRaw - pub unsafe fn to_raw(&self, mut store: impl AsContextMut) -> usize { + pub unsafe fn to_raw(&self, mut store: impl AsContextMut) -> *mut c_void { let externref_ptr = self.inner.as_raw(); store .as_context_mut() .0 .insert_vmexternref_without_gc(self.inner.clone()); - externref_ptr as usize + externref_ptr.cast() } } diff --git a/crates/wasmtime/src/values.rs b/crates/wasmtime/src/values.rs index bba872f35769..15954a0622ea 100644 --- a/crates/wasmtime/src/values.rs +++ b/crates/wasmtime/src/values.rs @@ -111,14 +111,14 @@ impl Val { Val::ExternRef(e) => { let externref = match e { Some(e) => e.to_raw(store), - None => 0, + None => ptr::null_mut(), }; ValRaw::externref(externref) } Val::FuncRef(f) => { let funcref = match f { Some(f) => f.to_raw(store), - None => 0, + None => ptr::null_mut(), }; ValRaw::funcref(funcref) } diff --git a/tests/all/func.rs b/tests/all/func.rs index 7c8021478519..4141213ad736 100644 --- a/tests/all/func.rs +++ b/tests/all/func.rs @@ -1307,3 +1307,116 @@ fn typed_funcs_count_params_correctly_in_error_messages() -> anyhow::Result<()> Ok(()) } + +#[test] +#[cfg_attr(miri, ignore)] +fn calls_with_funcref_and_externref() -> anyhow::Result<()> { + let mut store = Store::<()>::default(); + let module = Module::new( + store.engine(), + r#" + (module + (import "" "witness" (func $witness (param funcref externref))) + (func (export "f") (param funcref externref) (result externref funcref) + local.get 0 + local.get 1 + call $witness + local.get 1 + local.get 0 + ) + ) + + "#, + )?; + let mut linker = Linker::new(store.engine()); + linker.func_wrap( + "", + "witness", + |mut caller: Caller<'_, ()>, func: Option, externref: Option| { + if func.is_some() { + assert_my_funcref(&mut caller, func.as_ref())?; + } + if externref.is_some() { + assert_my_externref(externref.as_ref()); + } + Ok(()) + }, + )?; + let instance = linker.instantiate(&mut store, &module)?; + + let typed = instance + .get_typed_func::<(Option, Option), (Option, Option)>( + &mut store, "f", + )?; + let untyped = typed.func(); + + let my_funcref = Func::wrap(&mut store, || 100u32); + let my_externref = ExternRef::new(99u32); + let mut results = [Val::I32(0), Val::I32(0)]; + + fn assert_my_funcref(mut store: impl AsContextMut, func: Option<&Func>) -> Result<()> { + let mut store = store.as_context_mut(); + let func = func.unwrap(); + assert_eq!(func.typed::<(), u32>(&store)?.call(&mut store, ())?, 100); + Ok(()) + } + fn assert_my_externref(externref: Option<&ExternRef>) { + assert_eq!(externref.unwrap().data().downcast_ref(), Some(&99u32)); + } + + // funcref=null, externref=null + let (a, b) = typed.call(&mut store, (None, None))?; + assert!(a.is_none()); + assert!(b.is_none()); + untyped.call( + &mut store, + &[Val::FuncRef(None), Val::ExternRef(None)], + &mut results, + )?; + assert!(results[0].unwrap_externref().is_none()); + assert!(results[1].unwrap_funcref().is_none()); + + // funcref=Some, externref=null + let (a, b) = typed.call(&mut store, (Some(my_funcref), None))?; + assert!(a.is_none()); + assert_my_funcref(&mut store, b.as_ref())?; + untyped.call( + &mut store, + &[Val::FuncRef(Some(my_funcref)), Val::ExternRef(None)], + &mut results, + )?; + assert!(results[0].unwrap_externref().is_none()); + assert_my_funcref(&mut store, results[1].unwrap_funcref())?; + + // funcref=null, externref=Some + let (a, b) = typed.call(&mut store, (None, Some(my_externref.clone())))?; + assert_my_externref(a.as_ref()); + assert!(b.is_none()); + untyped.call( + &mut store, + &[ + Val::FuncRef(None), + Val::ExternRef(Some(my_externref.clone())), + ], + &mut results, + )?; + assert_my_externref(results[0].unwrap_externref().as_ref()); + assert!(results[1].unwrap_funcref().is_none()); + + // funcref=Some, externref=Some + let (a, b) = typed.call(&mut store, (Some(my_funcref), Some(my_externref.clone())))?; + assert_my_externref(a.as_ref()); + assert_my_funcref(&mut store, b.as_ref())?; + untyped.call( + &mut store, + &[ + Val::FuncRef(Some(my_funcref)), + Val::ExternRef(Some(my_externref.clone())), + ], + &mut results, + )?; + assert_my_externref(results[0].unwrap_externref().as_ref()); + assert_my_funcref(&mut store, results[1].unwrap_funcref())?; + + Ok(()) +} From 7b543dc9a0909caf411b254ccb2a71c3a96c9acf Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Wed, 3 May 2023 11:08:19 -0700 Subject: [PATCH 06/11] Minor remaining cleanups --- crates/runtime/src/instance.rs | 8 ++++++-- crates/runtime/src/libcalls.rs | 2 +- crates/runtime/src/table.rs | 6 +++--- 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/crates/runtime/src/instance.rs b/crates/runtime/src/instance.rs index ac1fa3d9e4c7..e5779bdb2120 100644 --- a/crates/runtime/src/instance.rs +++ b/crates/runtime/src/instance.rs @@ -941,7 +941,10 @@ impl Instance { }) } - /// TODO + /// Gets the raw runtime table data structure owned by this instance + /// given the provided `idx`. + /// + /// The `range` specified is eagerly initialized for funcref tables. pub fn get_defined_table_with_lazy_init( &mut self, idx: DefinedTableIndex, @@ -1211,7 +1214,8 @@ pub struct InstanceHandle { } impl InstanceHandle { - /// TODO + /// Creates an "empty" instance handle which internally has a null pointer + /// to an instance. pub fn null() -> InstanceHandle { InstanceHandle { instance: None } } diff --git a/crates/runtime/src/libcalls.rs b/crates/runtime/src/libcalls.rs index 81d14d83358a..f1044d2ea64a 100644 --- a/crates/runtime/src/libcalls.rs +++ b/crates/runtime/src/libcalls.rs @@ -385,7 +385,7 @@ unsafe fn table_get_lazy_init_func_ref( .get(index) .expect("table access already bounds-checked"); - elem.into_ref_asserting_initialized() as *mut u8 + elem.into_ref_asserting_initialized() }) } diff --git a/crates/runtime/src/table.rs b/crates/runtime/src/table.rs index 03004aaafa1b..ec394174e8ec 100644 --- a/crates/runtime/src/table.rs +++ b/crates/runtime/src/table.rs @@ -108,10 +108,10 @@ impl TableElement { /// # Safety /// /// The same warnings as for `into_table_values()` apply. - pub(crate) unsafe fn into_ref_asserting_initialized(self) -> usize { + pub(crate) unsafe fn into_ref_asserting_initialized(self) -> *mut u8 { match self { - Self::FuncRef(e) => e as usize, - Self::ExternRef(e) => e.map_or(0, |e| e.into_raw() as usize), + Self::FuncRef(e) => e.cast(), + Self::ExternRef(e) => e.map_or(ptr::null_mut(), |e| e.into_raw()), Self::UninitFunc => panic!("Uninitialized table element value outside of table slot"), } } From df8c1fc870f3eaf16e8fe5378d9b07bd8d287974 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Wed, 3 May 2023 11:13:05 -0700 Subject: [PATCH 07/11] Switch to Stacked Borrows for MIRI on CI Additionally enable the strict-provenance features to force warnings emitted today to become errors. --- .github/workflows/main.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 4e7b94a77757..7a1f8428686a 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -625,7 +625,7 @@ jobs: -p wasmtime-runtime \ -p wasmtime-environ env: - MIRIFLAGS: -Zmiri-tree-borrows + MIRIFLAGS: -Zmiri-strict-provenance # common logic to cancel the entire run if this job fails - run: gh run cancel ${{ github.run_id }} From f753a7d738a15a3886ed95d385d64194ac987818 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Thu, 4 May 2023 08:09:41 -0700 Subject: [PATCH 08/11] Fix a typo --- crates/runtime/src/instance.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/runtime/src/instance.rs b/crates/runtime/src/instance.rs index e5779bdb2120..6f72d877eaa6 100644 --- a/crates/runtime/src/instance.rs +++ b/crates/runtime/src/instance.rs @@ -97,7 +97,7 @@ pub struct Instance { /// A pointer to the `vmctx` field at the end of the `Instance`. /// /// If you're looking at this a reasonable question would be "why do we need - /// a pointer to ourselves?" because after all the pointer's valid is + /// a pointer to ourselves?" because after all the pointer's value is /// trivially derivable from any `&Instance` pointer. The rationale for this /// field's existence is subtle, but it's required for correctness. The /// short version is "this makes miri happy". From 91bc932b4d1d909038977d5061f736fe46e290e3 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Thu, 4 May 2023 08:10:13 -0700 Subject: [PATCH 09/11] Replace a negative offset with `sub` --- crates/runtime/src/instance.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/runtime/src/instance.rs b/crates/runtime/src/instance.rs index 6f72d877eaa6..ea3e2f3f07bf 100644 --- a/crates/runtime/src/instance.rs +++ b/crates/runtime/src/instance.rs @@ -212,7 +212,7 @@ impl Instance { pub unsafe fn from_vmctx(vmctx: *mut VMContext, f: impl FnOnce(&mut Instance) -> R) -> R { let ptr = vmctx .cast::() - .offset(-(mem::size_of::() as isize)) + .sub(mem::size_of::()) .cast::(); f(&mut *ptr) } From 5933d9d8dc9124099dc45412be85b048ff6a083a Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Thu, 4 May 2023 08:14:30 -0700 Subject: [PATCH 10/11] Comment the sentinel value --- crates/runtime/src/cow.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/crates/runtime/src/cow.rs b/crates/runtime/src/cow.rs index f3c9b387b51f..d817d18e68eb 100644 --- a/crates/runtime/src/cow.rs +++ b/crates/runtime/src/cow.rs @@ -435,6 +435,9 @@ impl MemoryImageSlot { #[cfg(feature = "pooling-allocator")] pub(crate) fn dummy() -> MemoryImageSlot { MemoryImageSlot { + // This pointer isn't ever actually used so its value doesn't + // matter, but we need to satisfy `NonNull` so pass a "1" value to + // handle that. Otherwise this shouldn't be used anywhere else. base: NonNull::new(sptr::invalid_mut(1)).unwrap().into(), static_size: 0, image: None, From d281e39d93d2be720ef633ecbb9d581276dba984 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Thu, 4 May 2023 12:57:58 -0700 Subject: [PATCH 11/11] Use NonNull::dangling --- crates/runtime/src/cow.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/crates/runtime/src/cow.rs b/crates/runtime/src/cow.rs index d817d18e68eb..8d6ebb8c39b8 100644 --- a/crates/runtime/src/cow.rs +++ b/crates/runtime/src/cow.rs @@ -436,9 +436,10 @@ impl MemoryImageSlot { pub(crate) fn dummy() -> MemoryImageSlot { MemoryImageSlot { // This pointer isn't ever actually used so its value doesn't - // matter, but we need to satisfy `NonNull` so pass a "1" value to - // handle that. Otherwise this shouldn't be used anywhere else. - base: NonNull::new(sptr::invalid_mut(1)).unwrap().into(), + // matter but we need to satisfy `NonNull` requirement so create a + // `dangling` pointer as a sentinel that should cause problems if + // it's actually used. + base: NonNull::dangling().into(), static_size: 0, image: None, accessible: 0,