Make table/memory allocation functions safe (#11320)

These were previously marked as `unsafe` trait methods with a
requirement that the memory/table shape must be validated ahead of time.
Neither the ondemand nor pooling allocator actually has an unsafe
contract to uphold with respect to this and both may assert/reject
non-validated shapes but memory unsafety won't happen as a result.
Consequently these functions are made safe.

Instance allocation functions are adjusted to reflect how the
correctness of `imports` is required for the functions to be safe.

Author: alexcrichton

crates/wasmtime/src/runtime/store.rs

@@ -1461,12 +1461,10 @@ impl StoreOpaque {
             let mem_ty = engine.tunables().gc_heap_memory_type();
             let tunables = engine.tunables();
 
-            // SAFETY: We validated the GC heap's memory type during engine creation.
-            let (mem_alloc_index, mem) = unsafe {
+            let (mem_alloc_index, mem) =
                 engine
                     .allocator()
-                    .allocate_memory(&mut request, &mem_ty, tunables, None)?
-            };
+                    .allocate_memory(&mut request, &mem_ty, tunables, None)?;
 
             // Then, allocate the actual GC heap, passing in that memory
             // storage.
@@ -2062,10 +2060,8 @@ at https://bytecodealliance.org/security.
     ///
     /// # Safety
     ///
-    /// The request's associated module, memories, tables, and vmctx must have
-    /// already have been validated by `validate_module` for the allocator
-    /// configured. This is typically done during module construction for
-    /// example.
+    /// The `imports` provided must be correctly sized/typed for the module
+    /// being allocated.
     pub(crate) unsafe fn allocate_instance(
         &mut self,
         kind: AllocateInstanceKind<'_>,
@@ -2078,16 +2074,20 @@ at https://bytecodealliance.org/security.
             AllocateInstanceKind::Module(_) => self.engine().allocator(),
             AllocateInstanceKind::Dummy { allocator } => allocator,
         };
-        let handle = allocator.allocate_module(InstanceAllocationRequest {
-            id,
-            runtime_info,
-            imports,
-            store: StorePtr::new(self.traitobj()),
-            #[cfg(feature = "wmemcheck")]
-            wmemcheck: self.engine().config().wmemcheck,
-            pkey: self.get_pkey(),
-            tunables: self.engine().tunables(),
-        })?;
+        // SAFETY: this function's own contract is the same as
+        // `allocate_module`, namely the imports provided are valid.
+        let handle = unsafe {
+            allocator.allocate_module(InstanceAllocationRequest {
+                id,
+                runtime_info,
+                imports,
+                store: StorePtr::new(self.traitobj()),
+                #[cfg(feature = "wmemcheck")]
+                wmemcheck: self.engine().config().wmemcheck,
+                pkey: self.get_pkey(),
+                tunables: self.engine().tunables(),
+            })?
+        };
 
         let actual = match kind {
             AllocateInstanceKind::Module(module_id) => {

crates/wasmtime/src/runtime/trampoline/memory.rs

@@ -165,7 +165,7 @@ unsafe impl InstanceAllocatorImpl for SingleMemoryInstance<'_> {
         self.ondemand.decrement_core_instance_count();
     }
 
-    unsafe fn allocate_memory(
+    fn allocate_memory(
         &self,
         request: &mut InstanceAllocationRequest,
         ty: &wasmtime_environ::Memory,
@@ -201,7 +201,7 @@ unsafe impl InstanceAllocatorImpl for SingleMemoryInstance<'_> {
             .deallocate_memory(memory_index, allocation_index, memory)
     }
 
-    unsafe fn allocate_table(
+    fn allocate_table(
         &self,
         req: &mut InstanceAllocationRequest,
         ty: &wasmtime_environ::Table,

crates/wasmtime/src/runtime/vm/instance.rs

@@ -270,7 +270,13 @@ impl Instance {
     ///
     /// It is assumed the memory was properly aligned and the
     /// allocation was `alloc_size` in bytes.
-    fn new(
+    ///
+    /// # Safety
+    ///
+    /// The `req.imports` field must be appropriately sized/typed for the module
+    /// being allocated according to `req.runtime_info`. Additionally `memories`
+    /// and `tables` must have been allocated for `req.store`.
+    unsafe fn new(
         req: InstanceAllocationRequest,
         memories: PrimaryMap<DefinedMemoryIndex, (MemoryAllocationIndex, Memory)>,
         tables: PrimaryMap<DefinedTableIndex, (TableAllocationIndex, Table)>,

crates/wasmtime/src/runtime/vm/instance/allocator.rs

@@ -253,13 +253,7 @@ pub unsafe trait InstanceAllocatorImpl {
     fn decrement_core_instance_count(&self);
 
     /// Allocate a memory for an instance.
-    ///
-    /// # Unsafety
-    ///
-    /// The memory and its associated module must have already been validated by
-    /// `Self::validate_memory` (or transtively via
-    /// `Self::validate_{module,component}`) and passed that validation.
-    unsafe fn allocate_memory(
+    fn allocate_memory(
         &self,
         request: &mut InstanceAllocationRequest,
         ty: &wasmtime_environ::Memory,
@@ -282,12 +276,7 @@ pub unsafe trait InstanceAllocatorImpl {
     );
 
     /// Allocate a table for an instance.
-    ///
-    /// # Unsafety
-    ///
-    /// The table and its associated module must have already been validated by
-    /// `Self::validate_module` and passed that validation.
-    unsafe fn allocate_table(
+    fn allocate_table(
         &self,
         req: &mut InstanceAllocationRequest,
         table: &wasmtime_environ::Table,
@@ -409,10 +398,10 @@ pub trait InstanceAllocator: InstanceAllocatorImpl {
     /// Note that the returned instance must still have `.initialize(..)` called
     /// on it to complete the instantiation process.
     ///
-    /// # Unsafety
+    /// # Safety
     ///
-    /// The request's associated module, memories, tables, and vmctx must have
-    /// already have been validated by `Self::validate_module`.
+    /// The `request` provided must be valid, e.g. the imports within are
+    /// correctly sized/typed for the instance being created.
     unsafe fn allocate_module(
         &self,
         mut request: InstanceAllocationRequest,
@@ -432,15 +421,14 @@ pub trait InstanceAllocator: InstanceAllocatorImpl {
         let mut tables = PrimaryMap::with_capacity(num_defined_tables);
 
         match (|| {
-            // SAFETY: validation of tables/memories is a contract of this
-            // function.
-            unsafe {
-                self.allocate_memories(&mut request, &mut memories)?;
-                self.allocate_tables(&mut request, &mut tables)?;
-            }
+            self.allocate_memories(&mut request, &mut memories)?;
+            self.allocate_tables(&mut request, &mut tables)?;
             Ok(())
         })() {
-            Ok(_) => Ok(Instance::new(request, memories, tables, &module.memories)),
+            // SAFETY: memories/tables were just allocated from the store within
+            // `request` and this function's own contract requires that the
+            // imports are valid.
+            Ok(_) => unsafe { Ok(Instance::new(request, memories, tables, &module.memories)) },
             Err(e) => {
                 // SAFETY: these were previously allocated by this allocator
                 unsafe {
@@ -475,12 +463,7 @@ pub trait InstanceAllocator: InstanceAllocatorImpl {
 
     /// Allocate the memories for the given instance allocation request, pushing
     /// them into `memories`.
-    ///
-    /// # Unsafety
-    ///
-    /// The request's associated module and memories must have previously been
-    /// validated by `Self::validate_module`.
-    unsafe fn allocate_memories(
+    fn allocate_memories(
         &self,
         request: &mut InstanceAllocationRequest,
         memories: &mut PrimaryMap<DefinedMemoryIndex, (MemoryAllocationIndex, Memory)>,
@@ -496,10 +479,7 @@ pub trait InstanceAllocator: InstanceAllocatorImpl {
                 .defined_memory_index(memory_index)
                 .expect("should be a defined memory since we skipped imported ones");
 
-            // SAFETY: validation of the memory from this allocator is itself a
-            // contract of this function.
-            let memory =
-                unsafe { self.allocate_memory(request, ty, request.tunables, Some(memory_index))? };
+            let memory = self.allocate_memory(request, ty, request.tunables, Some(memory_index))?;
             memories.push(memory);
         }
 
@@ -533,12 +513,7 @@ pub trait InstanceAllocator: InstanceAllocatorImpl {
 
     /// Allocate tables for the given instance allocation request, pushing them
     /// into `tables`.
-    ///
-    /// # Unsafety
-    ///
-    /// The request's associated module and tables must have previously been
-    /// validated by `Self::validate_module`.
-    unsafe fn allocate_tables(
+    fn allocate_tables(
         &self,
         request: &mut InstanceAllocationRequest,
         tables: &mut PrimaryMap<DefinedTableIndex, (TableAllocationIndex, Table)>,
@@ -554,10 +529,7 @@ pub trait InstanceAllocator: InstanceAllocatorImpl {
                 .defined_table_index(index)
                 .expect("should be a defined table since we skipped imported ones");
 
-            // SAFETY: the contract here is that the table has been validated by
-            // this allocator which is a contract of this function itself.
-            let table =
-                unsafe { self.allocate_table(request, table, request.tunables, def_index)? };
+            let table = self.allocate_table(request, table, request.tunables, def_index)?;
             tables.push(table);
         }
 

crates/wasmtime/src/runtime/vm/instance/allocator/on_demand.rs

@@ -110,7 +110,7 @@ unsafe impl InstanceAllocatorImpl for OnDemandInstanceAllocator {
 
     fn decrement_core_instance_count(&self) {}
 
-    unsafe fn allocate_memory(
+    fn allocate_memory(
         &self,
         request: &mut InstanceAllocationRequest,
         ty: &wasmtime_environ::Memory,
@@ -154,7 +154,7 @@ unsafe impl InstanceAllocatorImpl for OnDemandInstanceAllocator {
         // Normal destructors do all the necessary clean up.
     }
 
-    unsafe fn allocate_table(
+    fn allocate_table(
         &self,
         request: &mut InstanceAllocationRequest,
         ty: &wasmtime_environ::Table,

crates/wasmtime/src/runtime/vm/instance/allocator/pooling.rs

@@ -622,7 +622,7 @@ unsafe impl InstanceAllocatorImpl for PoolingInstanceAllocator {
         self.live_core_instances.fetch_sub(1, Ordering::AcqRel);
     }
 
-    unsafe fn allocate_memory(
+    fn allocate_memory(
         &self,
         request: &mut InstanceAllocationRequest,
         ty: &wasmtime_environ::Memory,
@@ -663,7 +663,7 @@ unsafe impl InstanceAllocatorImpl for PoolingInstanceAllocator {
         self.merge_or_flush(queue);
     }
 
-    unsafe fn allocate_table(
+    fn allocate_table(
         &self,
         request: &mut InstanceAllocationRequest,
         ty: &wasmtime_environ::Table,