Deny unsafe_op_in_unsafe_fn in wasmtime::vm::instance (bytecodealliance#11267)

Turn the lint on and add some safety comments where appropriate.

cc bytecodealliance#11180

Author: alexcrichton

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

@@ -2,6 +2,11 @@
 //! wasm module (except its callstack and register state). An
 //! `InstanceHandle` is a reference-counting handle for an `Instance`.
 
+#![warn(
+    unsafe_op_in_unsafe_fn,
+    reason = "opt-in until the crate opts-in as a whole -- #11180"
+)]
+
 use crate::prelude::*;
 use crate::runtime::vm::const_expr::{ConstEvalContext, ConstExprEvaluator};
 use crate::runtime::vm::export::Export;
@@ -125,11 +130,18 @@ impl InstanceAndStore {
         f: impl for<'a> FnOnce(&'a mut Self) -> R,
     ) -> R {
         const _: () = assert!(mem::size_of::<InstanceAndStore>() == mem::size_of::<Instance>());
-        let mut ptr = vmctx
-            .byte_sub(mem::size_of::<Instance>())
-            .cast::<InstanceAndStore>();
+        // SAFETY: The validity of this `byte_sub` relies on `vmctx` being a
+        // valid allocation which is itself a contract of this function.
+        let mut ptr = unsafe {
+            vmctx
+                .byte_sub(mem::size_of::<Instance>())
+                .cast::<InstanceAndStore>()
+        };
 
-        f(ptr.as_mut())
+        // SAFETY: the ability to interpret `vmctx` as a safe pointer and
+        // continue on is a contract of this function itself, so the safety here
+        // is effectively up to callers.
+        unsafe { f(ptr.as_mut()) }
     }
 
     /// Unpacks this `InstanceAndStore` into its underlying `Instance` and `dyn
@@ -327,10 +339,18 @@ impl Instance {
     /// from the store that `self` belongs to.
     #[inline]
     unsafe fn sibling_vmctx<'a>(&'a self, vmctx: NonNull<VMContext>) -> &'a Instance {
-        let ptr = vmctx
-            .byte_sub(mem::size_of::<Instance>())
-            .cast::<Instance>();
-        ptr.as_ref()
+        // SAFETY: it's a contract of this function itself that `vmctx` is a
+        // valid pointer such that this pointer arithmetic is valid.
+        let ptr = unsafe {
+            vmctx
+                .byte_sub(mem::size_of::<Instance>())
+                .cast::<Instance>()
+        };
+        // SAFETY: it's a contract of this function itself that `vmctx` is a
+        // valid pointer to dereference. Additionally the lifetime of the return
+        // value is constrained to be the same as `self` to avoid granting a
+        // too-long lifetime.
+        unsafe { ptr.as_ref() }
     }
 
     /// Same as [`Self::sibling_vmctx`], but the mutable version.
@@ -344,10 +364,20 @@ impl Instance {
         self: Pin<&'a mut Self>,
         vmctx: NonNull<VMContext>,
     ) -> Pin<&'a mut Instance> {
-        let mut ptr = vmctx
-            .byte_sub(mem::size_of::<Instance>())
-            .cast::<Instance>();
-        Pin::new_unchecked(ptr.as_mut())
+        // SAFETY: it's a contract of this function itself that `vmctx` is a
+        // valid pointer such that this pointer arithmetic is valid.
+        let mut ptr = unsafe {
+            vmctx
+                .byte_sub(mem::size_of::<Instance>())
+                .cast::<Instance>()
+        };
+
+        // SAFETY: it's a contract of this function itself that `vmctx` is a
+        // valid pointer to dereference. Additionally the lifetime of the return
+        // value is constrained to be the same as `self` to avoid granting a
+        // too-long lifetime. Finally mutable references to an instance are
+        // always through `Pin`, so it's safe to create a pin-pointer here.
+        unsafe { Pin::new_unchecked(ptr.as_mut()) }
     }
 
     pub(crate) fn env_module(&self) -> &Arc<wasmtime_environ::Module> {
@@ -527,38 +557,42 @@ impl Instance {
     }
 
     pub(crate) unsafe fn set_store(mut self: Pin<&mut Self>, store: Option<NonNull<dyn VMStore>>) {
-        *self.as_mut().store_mut() = store.map(VMStoreRawPtr);
-        if let Some(mut store) = store {
-            let store = store.as_mut();
-            self.vm_store_context()
-                .write(Some(store.vm_store_context_ptr().into()));
-            #[cfg(target_has_atomic = "64")]
-            {
-                *self.as_mut().epoch_ptr() =
-                    Some(NonNull::from(store.engine().epoch_counter()).into());
-            }
+        // FIXME: should be more targeted ideally with the `unsafe` than just
+        // throwing this entire function in a large `unsafe` block.
+        unsafe {
+            *self.as_mut().store_mut() = store.map(VMStoreRawPtr);
+            if let Some(mut store) = store {
+                let store = store.as_mut();
+                self.vm_store_context()
+                    .write(Some(store.vm_store_context_ptr().into()));
+                #[cfg(target_has_atomic = "64")]
+                {
+                    *self.as_mut().epoch_ptr() =
+                        Some(NonNull::from(store.engine().epoch_counter()).into());
+                }
 
-            if self.env_module().needs_gc_heap {
-                self.as_mut().set_gc_heap(Some(store.gc_store().expect(
-                    "if we need a GC heap, then `Instance::new_raw` should have already \
+                if self.env_module().needs_gc_heap {
+                    self.as_mut().set_gc_heap(Some(store.gc_store().expect(
+                        "if we need a GC heap, then `Instance::new_raw` should have already \
                      allocated it for us",
-                )));
+                    )));
+                } else {
+                    self.as_mut().set_gc_heap(None);
+                }
             } else {
+                self.vm_store_context().write(None);
+                #[cfg(target_has_atomic = "64")]
+                {
+                    *self.as_mut().epoch_ptr() = None;
+                }
                 self.as_mut().set_gc_heap(None);
             }
-        } else {
-            self.vm_store_context().write(None);
-            #[cfg(target_has_atomic = "64")]
-            {
-                *self.as_mut().epoch_ptr() = None;
-            }
-            self.as_mut().set_gc_heap(None);
         }
     }
 
     unsafe fn set_gc_heap(self: Pin<&mut Self>, gc_store: Option<&GcStore>) {
         if let Some(gc_store) = gc_store {
-            *self.gc_heap_data() = Some(gc_store.gc_heap.vmctx_gc_heap_data().into());
+            *self.gc_heap_data() = Some(unsafe { gc_store.gc_heap.vmctx_gc_heap_data().into() });
         } else {
             *self.gc_heap_data() = None;
         }
@@ -1347,115 +1381,165 @@ impl Instance {
     ) {
         assert!(ptr::eq(module, self.env_module().as_ref()));
 
-        self.vmctx_plus_offset_raw::<u32>(offsets.ptr.vmctx_magic())
-            .write(VMCONTEXT_MAGIC);
-        self.as_mut().set_store(store.as_raw());
+        // SAFETY: the type of the magic field is indeed `u32` and this function
+        // is initializing its value.
+        unsafe {
+            self.vmctx_plus_offset_raw::<u32>(offsets.ptr.vmctx_magic())
+                .write(VMCONTEXT_MAGIC);
+        }
+
+        // SAFETY: it's up to the caller to provide a valid store pointer here.
+        unsafe {
+            self.as_mut().set_store(store.as_raw());
+        }
 
         // Initialize shared types
-        let types = NonNull::from(self.runtime_info.type_ids());
-        self.type_ids_array().write(types.cast().into());
+        //
+        // SAFETY: validity of the vmctx means it should be safe to write to it
+        // here.
+        unsafe {
+            let types = NonNull::from(self.runtime_info.type_ids());
+            self.type_ids_array().write(types.cast().into());
+        }
 
         // Initialize the built-in functions
-        static BUILTINS: VMBuiltinFunctionsArray = VMBuiltinFunctionsArray::INIT;
-        let ptr = BUILTINS.expose_provenance();
-        self.vmctx_plus_offset_raw(offsets.ptr.vmctx_builtin_functions())
-            .write(VmPtr::from(ptr));
+        //
+        // SAFETY: the type of the builtin functions field is indeed a pointer
+        // and the pointer being filled in here, plus the vmctx is valid to
+        // write to during initialization.
+        unsafe {
+            static BUILTINS: VMBuiltinFunctionsArray = VMBuiltinFunctionsArray::INIT;
+            let ptr = BUILTINS.expose_provenance();
+            self.vmctx_plus_offset_raw(offsets.ptr.vmctx_builtin_functions())
+                .write(VmPtr::from(ptr));
+        }
 
         // Initialize the imports
+        //
+        // SAFETY: the vmctx is safe to initialize during this function and
+        // validity of each item itself is a contract the caller must uphold.
         debug_assert_eq!(imports.functions.len(), module.num_imported_funcs);
-        ptr::copy_nonoverlapping(
-            imports.functions.as_ptr(),
-            self.vmctx_plus_offset_raw(offsets.vmctx_imported_functions_begin())
-                .as_ptr(),
-            imports.functions.len(),
-        );
-        debug_assert_eq!(imports.tables.len(), module.num_imported_tables);
-        ptr::copy_nonoverlapping(
-            imports.tables.as_ptr(),
-            self.vmctx_plus_offset_raw(offsets.vmctx_imported_tables_begin())
-                .as_ptr(),
-            imports.tables.len(),
-        );
-        debug_assert_eq!(imports.memories.len(), module.num_imported_memories);
-        ptr::copy_nonoverlapping(
-            imports.memories.as_ptr(),
-            self.vmctx_plus_offset_raw(offsets.vmctx_imported_memories_begin())
-                .as_ptr(),
-            imports.memories.len(),
-        );
-        debug_assert_eq!(imports.globals.len(), module.num_imported_globals);
-        ptr::copy_nonoverlapping(
-            imports.globals.as_ptr(),
-            self.vmctx_plus_offset_raw(offsets.vmctx_imported_globals_begin())
-                .as_ptr(),
-            imports.globals.len(),
-        );
-
-        debug_assert_eq!(imports.tags.len(), module.num_imported_tags);
-        ptr::copy_nonoverlapping(
-            imports.tags.as_ptr(),
-            self.vmctx_plus_offset_raw(offsets.vmctx_imported_tags_begin())
-                .as_ptr(),
-            imports.tags.len(),
-        );
+        unsafe {
+            ptr::copy_nonoverlapping(
+                imports.functions.as_ptr(),
+                self.vmctx_plus_offset_raw(offsets.vmctx_imported_functions_begin())
+                    .as_ptr(),
+                imports.functions.len(),
+            );
+            debug_assert_eq!(imports.tables.len(), module.num_imported_tables);
+            ptr::copy_nonoverlapping(
+                imports.tables.as_ptr(),
+                self.vmctx_plus_offset_raw(offsets.vmctx_imported_tables_begin())
+                    .as_ptr(),
+                imports.tables.len(),
+            );
+            debug_assert_eq!(imports.memories.len(), module.num_imported_memories);
+            ptr::copy_nonoverlapping(
+                imports.memories.as_ptr(),
+                self.vmctx_plus_offset_raw(offsets.vmctx_imported_memories_begin())
+                    .as_ptr(),
+                imports.memories.len(),
+            );
+            debug_assert_eq!(imports.globals.len(), module.num_imported_globals);
+            ptr::copy_nonoverlapping(
+                imports.globals.as_ptr(),
+                self.vmctx_plus_offset_raw(offsets.vmctx_imported_globals_begin())
+                    .as_ptr(),
+                imports.globals.len(),
+            );
+            debug_assert_eq!(imports.tags.len(), module.num_imported_tags);
+            ptr::copy_nonoverlapping(
+                imports.tags.as_ptr(),
+                self.vmctx_plus_offset_raw(offsets.vmctx_imported_tags_begin())
+                    .as_ptr(),
+                imports.tags.len(),
+            );
+        }
 
         // N.B.: there is no need to initialize the funcrefs array because we
         // eagerly construct each element in it whenever asked for a reference
         // to that element. In other words, there is no state needed to track
         // the lazy-init, so we don't need to initialize any state now.
 
         // Initialize the defined tables
-        let mut ptr = self.vmctx_plus_offset_raw(offsets.vmctx_tables_begin());
-        let tables = self.as_mut().tables_mut();
-        for i in 0..module.num_defined_tables() {
-            ptr.write(tables[DefinedTableIndex::new(i)].1.vmtable());
-            ptr = ptr.add(1);
+        //
+        // SAFETY: it's safe to initialize these tables during initialization
+        // here and the various types of pointers and such here should all be
+        // valid.
+        unsafe {
+            let mut ptr = self.vmctx_plus_offset_raw(offsets.vmctx_tables_begin());
+            let tables = self.as_mut().tables_mut();
+            for i in 0..module.num_defined_tables() {
+                ptr.write(tables[DefinedTableIndex::new(i)].1.vmtable());
+                ptr = ptr.add(1);
+            }
         }
 
         // Initialize the defined memories. This fills in both the
         // `defined_memories` table and the `owned_memories` table at the same
         // time. Entries in `defined_memories` hold a pointer to a definition
         // (all memories) whereas the `owned_memories` hold the actual
         // definitions of memories owned (not shared) in the module.
-        let mut ptr = self.vmctx_plus_offset_raw(offsets.vmctx_memories_begin());
-        let mut owned_ptr = self.vmctx_plus_offset_raw(offsets.vmctx_owned_memories_begin());
-        let memories = self.as_mut().memories_mut();
-        for i in 0..module.num_defined_memories() {
-            let defined_memory_index = DefinedMemoryIndex::new(i);
-            let memory_index = module.memory_index(defined_memory_index);
-            if module.memories[memory_index].shared {
-                let def_ptr = memories[defined_memory_index]
-                    .1
-                    .as_shared_memory()
-                    .unwrap()
-                    .vmmemory_ptr();
-                ptr.write(VmPtr::from(def_ptr));
-            } else {
-                owned_ptr.write(memories[defined_memory_index].1.vmmemory());
-                ptr.write(VmPtr::from(owned_ptr));
-                owned_ptr = owned_ptr.add(1);
+        //
+        // SAFETY: it's safe to initialize these memories during initialization
+        // here and the various types of pointers and such here should all be
+        // valid.
+        unsafe {
+            let mut ptr = self.vmctx_plus_offset_raw(offsets.vmctx_memories_begin());
+            let mut owned_ptr = self.vmctx_plus_offset_raw(offsets.vmctx_owned_memories_begin());
+            let memories = self.as_mut().memories_mut();
+            for i in 0..module.num_defined_memories() {
+                let defined_memory_index = DefinedMemoryIndex::new(i);
+                let memory_index = module.memory_index(defined_memory_index);
+                if module.memories[memory_index].shared {
+                    let def_ptr = memories[defined_memory_index]
+                        .1
+                        .as_shared_memory()
+                        .unwrap()
+                        .vmmemory_ptr();
+                    ptr.write(VmPtr::from(def_ptr));
+                } else {
+                    owned_ptr.write(memories[defined_memory_index].1.vmmemory());
+                    ptr.write(VmPtr::from(owned_ptr));
+                    owned_ptr = owned_ptr.add(1);
+                }
+                ptr = ptr.add(1);
             }
-            ptr = ptr.add(1);
         }
 
         // Zero-initialize the globals so that nothing is uninitialized memory
         // after this function returns. The globals are actually initialized
         // with their const expression initializers after the instance is fully
         // allocated.
-        for (index, _init) in module.global_initializers.iter() {
-            self.global_ptr(index).write(VMGlobalDefinition::new());
+        //
+        // SAFETY: it's safe to initialize globals during initialization
+        // here. Note that while the value being written is not valid for all
+        // types of globals it's initializing the memory to zero instead of
+        // being in an undefined state. So it's still unsafe to access globals
+        // after this, but if it's read then it'd hopefully crash faster than
+        // leaving this undefined.
+        unsafe {
+            for (index, _init) in module.global_initializers.iter() {
+                self.global_ptr(index).write(VMGlobalDefinition::new());
+            }
         }
 
         // Initialize the defined tags
-        let mut ptr = self.vmctx_plus_offset_raw(offsets.vmctx_tags_begin());
-        for i in 0..module.num_defined_tags() {
-            let defined_index = DefinedTagIndex::new(i);
-            let tag_index = module.tag_index(defined_index);
-            let tag = module.tags[tag_index];
-            ptr.write(VMTagDefinition::new(
-                tag.signature.unwrap_engine_type_index(),
-            ));
-            ptr = ptr.add(1);
+        //
+        // SAFETY: it's safe to initialize these tags during initialization
+        // here and the various types of pointers and such here should all be
+        // valid.
+        unsafe {
+            let mut ptr = self.vmctx_plus_offset_raw(offsets.vmctx_tags_begin());
+            for i in 0..module.num_defined_tags() {
+                let defined_index = DefinedTagIndex::new(i);
+                let tag_index = module.tag_index(defined_index);
+                let tag = module.tags[tag_index];
+                ptr.write(VMTagDefinition::new(
+                    tag.signature.unwrap_engine_type_index(),
+                ));
+                ptr = ptr.add(1);
+            }
         }
     }