Change some VMContext pointers to () pointers (#4190)

* Change some `VMContext` pointers to `()` pointers

This commit is motivated by my work on the component model
implementation for imported functions. Currently all context pointers in
wasm are `*mut VMContext` but with the component model my plan is to
make some pointers instead along the lines of `*mut VMComponentContext`.
In doing this though one worry I have is breaking what has otherwise
been a core invariant of Wasmtime for quite some time, subtly
introducing bugs by accident.

To help assuage my worry I've opted here to erase knowledge of
`*mut VMContext` where possible. Instead where applicable a context
pointer is simply known as `*mut ()` and the embedder doesn't actually
know anything about this context beyond the value of the pointer. This
will help prevent Wasmtime from accidentally ever trying to interpret
this context pointer as an actual `VMContext` when it might instead be a
`VMComponentContext`.

Overall this was a pretty smooth transition. The main change here is
that the `VMTrampoline` (now sporting more docs) has its first argument
changed to `*mut ()`. The second argument, the caller context, is still
configured as `*mut VMContext` though because all functions are always
called from wasm still. Eventually for component-to-component calls I
think we'll probably "fake" the second argument as the same as the first
argument, losing track of the original caller, as an intentional way of
isolating components from each other.

Along the way there are a few host locations which do actually assume
that the first argument is indeed a `VMContext`. These are valid
assumptions that are upheld from a correct implementation, but I opted
to add a "magic" field to `VMContext` to assert this in debug mode. This
new "magic" field is inintialized during normal vmcontext initialization
and it's checked whenever a `VMContext` is reinterpreted as an
`Instance` (but only in debug mode). My hope here is to catch any future
accidental mistakes, if ever.

* Use a VMOpaqueContext wrapper

* Fix typos

crates/environ/src/vmoffsets.rs

@@ -4,6 +4,7 @@
 // Currently the `VMContext` allocation by field looks like this:
 //
 // struct VMContext {
+//      magic: u32,
 //      runtime_limits: *const VMRuntimeLimits,
 //      externref_activations_table: *mut VMExternRefActivationsTable,
 //      store: *mut dyn Store,
@@ -74,6 +75,7 @@ pub struct VMOffsets<P> {
     pub num_escaped_funcs: u32,
 
     // precalculated offsets of various member fields
+    magic: u32,
     runtime_limits: u32,
     epoch_ptr: u32,
     externref_activations_table: u32,
@@ -222,6 +224,7 @@ impl<P: PtrSize> VMOffsets<P> {
             externref_activations_table: "jit host externref state",
             epoch_ptr: "jit current epoch state",
             runtime_limits: "jit runtime limits state",
+            magic: "magic value",
         }
     }
 }
@@ -239,6 +242,7 @@ impl<P: PtrSize> From<VMOffsetsFields<P>> for VMOffsets<P> {
             num_defined_memories: fields.num_defined_memories,
             num_defined_globals: fields.num_defined_globals,
             num_escaped_funcs: fields.num_escaped_funcs,
+            magic: 0,
             runtime_limits: 0,
             epoch_ptr: 0,
             externref_activations_table: 0,
@@ -278,7 +282,7 @@ impl<P: PtrSize> From<VMOffsetsFields<P>> for VMOffsets<P> {
                 next_field_offset = cadd(next_field_offset, u32::from($size));
                 fields!($($rest)*);
             };
-            (align($align:literal), $($rest:tt)*) => {
+            (align($align:expr), $($rest:tt)*) => {
                 next_field_offset = align(next_field_offset, $align);
                 fields!($($rest)*);
             };
@@ -286,6 +290,8 @@ impl<P: PtrSize> From<VMOffsetsFields<P>> for VMOffsets<P> {
         }
 
         fields! {
+            size(magic) = 4u32,
+            align(u32::from(ret.ptr.size())),
             size(runtime_limits) = ret.ptr.size(),
             size(epoch_ptr) = ret.ptr.size(),
             size(externref_activations_table) = ret.ptr.size(),
@@ -315,6 +321,11 @@ impl<P: PtrSize> From<VMOffsetsFields<P>> for VMOffsets<P> {
 
         ret.size = next_field_offset;
 
+        // This is required by the implementation of `VMContext::instance` and
+        // `VMContext::instance_mut`. If this value changes then those locations
+        // need to be updated.
+        assert_eq!(ret.magic, 0);
+
         return ret;
     }
 }
@@ -535,6 +546,12 @@ impl<P: PtrSize> VMOffsets<P> {
 
 /// Offsets for `VMContext`.
 impl<P: PtrSize> VMOffsets<P> {
+    /// Return the offset to the `magic` value in this `VMContext`.
+    #[inline]
+    pub fn vmctx_magic(&self) -> u32 {
+        self.magic
+    }
+
     /// Return the offset to the `VMRuntimeLimits` structure
     #[inline]
     pub fn vmctx_runtime_limits(&self) -> u32 {

crates/runtime/src/instance.rs

@@ -9,8 +9,8 @@ use crate::table::{Table, TableElement, TableElementType};
 use crate::traphandlers::Trap;
 use crate::vmcontext::{
     VMBuiltinFunctionsArray, VMCallerCheckedAnyfunc, VMContext, VMFunctionImport,
-    VMGlobalDefinition, VMGlobalImport, VMMemoryDefinition, VMMemoryImport, VMRuntimeLimits,
+    VMGlobalDefinition, VMGlobalImport, VMMemoryDefinition, VMMemoryImport, VMOpaqueContext,
-    VMTableDefinition, VMTableImport,
+    VMRuntimeLimits, VMTableDefinition, VMTableImport, VMCONTEXT_MAGIC,
 };
 use crate::{
     ExportFunction, ExportGlobal, ExportMemory, ExportTable, Imports, ModuleRuntimeInfo, Store,
@@ -488,7 +488,7 @@ impl Instance {
                 (self.runtime_info.image_base()
                     + self.runtime_info.function_info(def_index).start as usize)
                     as *mut _,
-                self.vmctx_ptr(),
+                VMOpaqueContext::from_vmcontext(self.vmctx_ptr()),
             )
         } else {
             let import = self.imported_function(index);
@@ -879,6 +879,8 @@ impl Instance {
     unsafe fn initialize_vmctx(&mut self, module: &Module, store: StorePtr, imports: Imports) {
         assert!(std::ptr::eq(module, self.module().as_ref()));
 
+        *self.vmctx_plus_offset(self.offsets.vmctx_magic()) = VMCONTEXT_MAGIC;
+
         if let Some(store) = store.as_raw() {
             *self.runtime_limits() = (*store).vmruntime_limits();
             *self.epoch_ptr() = (*store).epoch_ptr();

crates/runtime/src/lib.rs

@@ -65,8 +65,9 @@ pub use crate::traphandlers::{
 };
 pub use crate::vmcontext::{
     VMCallerCheckedAnyfunc, VMContext, VMFunctionBody, VMFunctionImport, VMGlobalDefinition,
-    VMGlobalImport, VMInvokeArgument, VMMemoryDefinition, VMMemoryImport, VMRuntimeLimits,
+    VMGlobalImport, VMInvokeArgument, VMMemoryDefinition, VMMemoryImport, VMOpaqueContext,
-    VMSharedSignatureIndex, VMTableDefinition, VMTableImport, VMTrampoline, ValRaw,
+    VMRuntimeLimits, VMSharedSignatureIndex, VMTableDefinition, VMTableImport, VMTrampoline,
+    ValRaw,
 };
 
 mod module_id;

crates/runtime/src/vmcontext.rs

@@ -9,6 +9,8 @@ use std::marker;
 use std::ptr::NonNull;
 use std::u32;
 
+pub const VMCONTEXT_MAGIC: u32 = u32::from_le_bytes(*b"core");
+
 /// An imported function.
 #[derive(Debug, Copy, Clone)]
 #[repr(C)]
@@ -16,8 +18,13 @@ pub struct VMFunctionImport {
     /// A pointer to the imported function body.
     pub body: NonNull<VMFunctionBody>,
 
-    /// A pointer to the `VMContext` that owns the function.
+    /// The VM state associated with this function.
-    pub vmctx: *mut VMContext,
+    ///
+    /// For core wasm instances this will be `*mut VMContext` but for the
+    /// upcoming implementation of the component model this will be something
+    /// else. The actual definition of what this pointer points to depends on
+    /// the definition of `func_ptr` and what compiled it.
+    pub vmctx: *mut VMOpaqueContext,
 }
 
 // Declare that this type is send/sync, it's the responsibility of users of
@@ -546,8 +553,13 @@ pub struct VMCallerCheckedAnyfunc {
     pub func_ptr: NonNull<VMFunctionBody>,
     /// Function signature id.
     pub type_index: VMSharedSignatureIndex,
-    /// Function `VMContext`.
+    /// The VM state associated with this function.
-    pub vmctx: *mut VMContext,
+    ///
+    /// For core wasm instances this will be `*mut VMContext` but for the
+    /// upcoming implementation of the component model this will be something
+    /// else. The actual definition of what this pointer points to depends on
+    /// the definition of `func_ptr` and what compiled it.
+    pub vmctx: *mut VMOpaqueContext,
     // If more elements are added here, remember to add offset_of tests below!
 }
 
@@ -746,6 +758,29 @@ pub struct VMContext {
 }
 
 impl VMContext {
+    /// Helper function to cast between context types using a debug assertion to
+    /// protect against some mistakes.
+    #[inline]
+    pub unsafe fn from_opaque(opaque: *mut VMOpaqueContext) -> *mut VMContext {
+        // Note that in general the offset of the "magic" field is stored in
+        // `VMOffsets::vmctx_magic`. Given though that this is a sanity check
+        // about converting this pointer to another type we ideally don't want
+        // to read the offset from potentially corrupt memory. Instead it would
+        // be better to catch errors here as soon as possible.
+        //
+        // To accomplish this the `VMContext` structure is laid out with the
+        // magic field at a statically known offset (here it's 0 for now). This
+        // static offset is asserted in `VMOffsets::from` and needs to be kept
+        // in sync with this line for this debug assertion to work.
+        //
+        // Also note that this magic is only ever invalid in the presence of
+        // bugs, meaning we don't actually read the magic and act differently
+        // at runtime depending what it is, so this is a debug assertion as
+        // opposed to a regular assertion.
+        debug_assert_eq!((*opaque).magic, VMCONTEXT_MAGIC);
+        opaque.cast()
+    }
+
     /// Return a mutable reference to the associated `Instance`.
     ///
     /// # Safety
@@ -968,10 +1003,69 @@ impl ValRaw {
     }
 }
 
-/// Trampoline function pointer type.
+/// Type definition of the trampoline used to enter WebAssembly from the host.
-pub type VMTrampoline = unsafe extern "C" fn(
+///
-    *mut VMContext,        // callee vmctx
+/// This function type is what's generated for the entry trampolines that are
-    *mut VMContext,        // caller vmctx
+/// compiled into a WebAssembly module's image. Note that trampolines are not
-    *const VMFunctionBody, // function we're actually calling
+/// always used by Wasmtime since the `TypedFunc` API allows bypassing the
-    *mut ValRaw,           // space for arguments and return values
+/// trampoline and directly calling the underlying wasm function (at the time of
-);
+/// this writing).
+///
+/// The trampoline's arguments here are:
+///
+/// * `*mut VMOpaqueContext` - this a contextual pointer defined within the
+///   context of the receiving function pointer. For now this is always `*mut
+///   VMContext` but with the component model it may be the case that this is a
+///   different type of pointer.
+///
+/// * `*mut VMContext` - this is the "caller" context, which at this time is
+///   always unconditionally core wasm (even in the component model). This
+///   contextual pointer cannot be `NULL` and provides information necessary to
+///   resolve the caller's context for the `Caller` API in Wasmtime.
+///
+/// * `*const VMFunctionBody` - this is the indirect function pointer which is
+///   the actual target function to invoke. This function uses the System-V ABI
+///   for its argumenst and a semi-custom ABI for the return values (one return
+///   value is returned directly, multiple return values have the first one
+///   returned directly and remaining ones returned indirectly through a
+///   stack pointer). This function pointer may be Cranelift-compiled code or it
+///   may also be a host-compiled trampoline (e.g. when a host function calls a
+///   host function through the `wasmtime::Func` wrapper). The definition of the
+///   first argument of this function depends on what this receiving function
+///   pointer desires.
+///
+/// * `*mut ValRaw` - this is storage space for both arguments and results of
+///   the function. The trampoline will read the arguments from this array to
+///   pass to the function pointer provided. The results are then written to the
+///   array afterwards (both reads and writes start at index 0). It's the
+///   caller's responsibility to make sure this array is appropriately sized.
+pub type VMTrampoline =
+    unsafe extern "C" fn(*mut VMOpaqueContext, *mut VMContext, *const VMFunctionBody, *mut ValRaw);
+
+/// An "opaque" version of `VMContext` which must be explicitly casted to a
+/// target context.
+///
+/// This context is used to represent that contexts specified in
+/// `VMCallerCheckedAnyfunc` can have any type and don't have an implicit
+/// structure. Neither wasmtime nor cranelift-generated code can rely on the
+/// structure of an opaque context in general and only the code which configured
+/// the context is able to rely on a particular structure. This is because the
+/// context pointer configured for `VMCallerCheckedAnyfunc` is guaranteed to be
+/// the first parameter passed.
+///
+/// Note that Wasmtime currently has a layout where all contexts that are casted
+/// to an opaque context start with a 32-bit "magic" which can be used in debug
+/// mode to debug-assert that the casts here are correct and have at least a
+/// little protection against incorrect casts.
+pub struct VMOpaqueContext {
+    magic: u32,
+    _marker: marker::PhantomPinned,
+}
+
+impl VMOpaqueContext {
+    /// Helper function to clearly indicate that cast desired
+    #[inline]
+    pub fn from_vmcontext(ptr: *mut VMContext) -> *mut VMOpaqueContext {
+        ptr.cast()
+    }
+}

crates/wasmtime/src/func.rs

@@ -13,8 +13,8 @@ use std::sync::Arc;
 use wasmtime_environ::FuncIndex;
 use wasmtime_runtime::{
     raise_user_trap, ExportFunction, InstanceAllocator, InstanceHandle, OnDemandInstanceAllocator,
-    VMCallerCheckedAnyfunc, VMContext, VMFunctionBody, VMFunctionImport, VMSharedSignatureIndex,
+    VMCallerCheckedAnyfunc, VMContext, VMFunctionBody, VMFunctionImport, VMOpaqueContext,
-    VMTrampoline,
+    VMSharedSignatureIndex, VMTrampoline,
 };
 
 /// A WebAssembly function which can be called.
@@ -1852,7 +1852,7 @@ macro_rules! impl_into_func {
                 /// by Cranelift, since Cranelift is generating raw function
                 /// calls directly to this function.
                 unsafe extern "C" fn wasm_to_host_shim<T, F, $($args,)* R>(
-                    vmctx: *mut VMContext,
+                    vmctx: *mut VMOpaqueContext,
                     caller_vmctx: *mut VMContext,
                     $( $args: $args::Abi, )*
                     retptr: R::Retptr,
@@ -1875,6 +1875,7 @@ macro_rules! impl_into_func {
                     // should be part of this block, and the long-jmp-ing
                     // happens after the block in handling `CallResult`.
                     let result = Caller::with(caller_vmctx, |mut caller| {
+                        let vmctx = VMContext::from_opaque(vmctx);
                         let state = (*vmctx).host_state();
                         // Double-check ourselves in debug mode, but we control
                         // the `Any` here so an unsafe downcast should also
@@ -1940,7 +1941,7 @@ macro_rules! impl_into_func {
                 /// calls the given function pointer, and then stores the result
                 /// back into the `args` array.
                 unsafe extern "C" fn host_trampoline<$($args,)* R>(
-                    callee_vmctx: *mut VMContext,
+                    callee_vmctx: *mut VMOpaqueContext,
                     caller_vmctx: *mut VMContext,
                     ptr: *const VMFunctionBody,
                     args: *mut ValRaw,
@@ -1952,7 +1953,7 @@ macro_rules! impl_into_func {
                     let ptr = mem::transmute::<
                         *const VMFunctionBody,
                         unsafe extern "C" fn(
-                            *mut VMContext,
+                            *mut VMOpaqueContext,
                             *mut VMContext,
                             $( $args::Abi, )*
                             R::Retptr,

crates/wasmtime/src/func/typed.rs

@@ -5,7 +5,9 @@ use anyhow::{bail, Result};
 use std::marker;
 use std::mem::{self, MaybeUninit};
 use std::ptr;
-use wasmtime_runtime::{VMCallerCheckedAnyfunc, VMContext, VMFunctionBody, VMSharedSignatureIndex};
+use wasmtime_runtime::{
+    VMCallerCheckedAnyfunc, VMContext, VMFunctionBody, VMOpaqueContext, VMSharedSignatureIndex,
+};
 
 /// A statically typed WebAssembly function.
 ///
@@ -464,7 +466,7 @@ pub unsafe trait WasmParams: Send {
     #[doc(hidden)]
     unsafe fn invoke<R: WasmResults>(
         func: *const VMFunctionBody,
-        vmctx1: *mut VMContext,
+        vmctx1: *mut VMOpaqueContext,
         vmctx2: *mut VMContext,
         abi: Self::Abi,
     ) -> R::ResultAbi;
@@ -494,7 +496,7 @@ where
 
     unsafe fn invoke<R: WasmResults>(
         func: *const VMFunctionBody,
-        vmctx1: *mut VMContext,
+        vmctx1: *mut VMOpaqueContext,
         vmctx2: *mut VMContext,
         abi: Self::Abi,
     ) -> R::ResultAbi {
@@ -554,14 +556,14 @@ macro_rules! impl_wasm_params {
 
             unsafe fn invoke<R: WasmResults>(
                 func: *const VMFunctionBody,
-                vmctx1: *mut VMContext,
+                vmctx1: *mut VMOpaqueContext,
                 vmctx2: *mut VMContext,
                 abi: Self::Abi,
             ) -> R::ResultAbi {
                 let fnptr = mem::transmute::<
                     *const VMFunctionBody,
                     unsafe extern "C" fn(
-                        *mut VMContext,
+                        *mut VMOpaqueContext,
                         *mut VMContext,
                         $($t::Abi,)*
                         <R::ResultAbi as HostAbi>::Retptr,

crates/wasmtime/src/instance.rs

@@ -11,7 +11,7 @@ use std::sync::Arc;
 use wasmtime_environ::{EntityType, FuncIndex, GlobalIndex, MemoryIndex, PrimaryMap, TableIndex};
 use wasmtime_runtime::{
     Imports, InstanceAllocationRequest, InstantiationError, StorePtr, VMContext, VMFunctionBody,
-    VMFunctionImport, VMGlobalImport, VMMemoryImport, VMTableImport,
+    VMFunctionImport, VMGlobalImport, VMMemoryImport, VMOpaqueContext, VMTableImport,
 };
 
 /// An instantiated WebAssembly module.
@@ -345,7 +345,7 @@ impl Instance {
             super::func::invoke_wasm_and_catch_traps(store, |_default_callee| {
                 mem::transmute::<
                     *const VMFunctionBody,
-                    unsafe extern "C" fn(*mut VMContext, *mut VMContext),
+                    unsafe extern "C" fn(*mut VMOpaqueContext, *mut VMContext),
                 >(f.anyfunc.as_ref().func_ptr.as_ptr())(
                     f.anyfunc.as_ref().vmctx, vmctx
                 )

crates/wasmtime/src/trampoline/func.rs

@@ -12,8 +12,8 @@ use wasmtime_environ::{
 use wasmtime_jit::{CodeMemory, ProfilingAgent};
 use wasmtime_runtime::{
     Imports, InstanceAllocationRequest, InstanceAllocator, InstanceHandle,
-    OnDemandInstanceAllocator, StorePtr, VMContext, VMFunctionBody, VMSharedSignatureIndex,
+    OnDemandInstanceAllocator, StorePtr, VMContext, VMFunctionBody, VMOpaqueContext,
-    VMTrampoline,
+    VMSharedSignatureIndex, VMTrampoline,
 };
 
 struct TrampolineState<F> {
@@ -23,7 +23,7 @@ struct TrampolineState<F> {
 }
 
 unsafe extern "C" fn stub_fn<F>(
-    vmctx: *mut VMContext,
+    vmctx: *mut VMOpaqueContext,
     caller_vmctx: *mut VMContext,
     values_vec: *mut ValRaw,
     values_vec_len: usize,
@@ -44,6 +44,7 @@ unsafe extern "C" fn stub_fn<F>(
     // have any. To prevent leaks we avoid having any local destructors by
     // avoiding local variables.
     let result = panic::catch_unwind(AssertUnwindSafe(|| {
+        let vmctx = VMContext::from_opaque(vmctx);
         // Double-check ourselves in debug mode, but we control
         // the `Any` here so an unsafe downcast should also
         // work.

tests/all/pooling_allocator.rs

@@ -630,11 +630,10 @@ fn instance_too_large() -> Result<()> {
 
     let engine = Engine::new(&config)?;
     let expected = "\
-instance allocation for this module requires 304 bytes which exceeds the \
+instance allocation for this module requires 320 bytes which exceeds the \
 configured maximum of 16 bytes; breakdown of allocation requirement:
 
- * 78.95% - 240 bytes - instance state management
+ * 80.00% - 256 bytes - instance state management
- * 5.26% - 16 bytes - jit store state
 ";
     match Module::new(&engine, "(module)") {
         Ok(_) => panic!("should have failed to compile"),
@@ -648,11 +647,11 @@ configured maximum of 16 bytes; breakdown of allocation requirement:
     lots_of_globals.push_str(")");
 
     let expected = "\
-instance allocation for this module requires 1904 bytes which exceeds the \
+instance allocation for this module requires 1920 bytes which exceeds the \
 configured maximum of 16 bytes; breakdown of allocation requirement:
 
- * 12.61% - 240 bytes - instance state management
+ * 13.33% - 256 bytes - instance state management
- * 84.03% - 1600 bytes - defined globals
+ * 83.33% - 1600 bytes - defined globals
 ";
     match Module::new(&engine, &lots_of_globals) {
         Ok(_) => panic!("should have failed to compile"),