Reel in unsafety around InstanceHandle (#856)

* Reel in unsafety around `InstanceHandle`

This commit is an attempt, or at least is targeted at being a start, at
reeling in the unsafety around the `InstanceHandle` type. Currently this
type represents a sort of moral `Rc<Instance>` but is a bit more
specialized since the underlying memory is allocated through mmap.

Additionally, though, `InstanceHandle` exposes a fundamental flaw in its
safety by safetly allowing mutable access so long as you have `&mut
InstanceHandle`. This type, however, is trivially created by simply
cloning a `InstanceHandle` to get an owned reference. This means that
`&mut InstanceHandle` does not actually provide any guarantees about
uniqueness, so there's no more safety than `&InstanceHandle` itself.

This commit removes all `&mut self` APIs from `InstanceHandle`,
additionally removing some where `&self` was `unsafe` and `&mut self`
was safe (since it was trivial to subvert this "safety"). In doing so
interior mutability patterns are now used much more extensively through
structures such as `Table` and `Memory`. Additionally a number of
methods were refactored to be a bit clearer and use helper functions
where possible.

This is a relatively large commit unfortunately, but it snowballed very
quickly into touching quite a few places. My hope though is that this
will prevent developers working on wasmtime internals as well as
developers still yet to migrate to the `wasmtime` crate from falling
into trivial unsafe traps by accidentally using `&mut` when they can't.
All existing users relying on `&mut` will need to migrate to some form
of interior mutability, such as using `RefCell` or `Cell`.

This commit also additionally marks `InstanceHandle::new` as an `unsafe`
function. The rationale for this is that the `&mut`-safety is only the
beginning for the safety of `InstanceHandle`. In general the wasmtime
internals are extremely unsafe and haven't been audited for appropriate
usage of `unsafe`. Until that's done it's hoped that we can warn users
with this `unsafe` constructor and otherwise push users to the
`wasmtime` crate which we know is safe.

* Fix windows build

* Wrap up mutable memory state in one structure

Rather than having separate fields

* Use `Cell::set`, not `Cell::replace`, where possible

* Add a helper function for offsets from VMContext

* Fix a typo from merging

* rustfmt

* Use try_from, not as

* Tweak style of some setters

crates/api/src/externals.rs

@@ -420,12 +420,9 @@ fn set_table_item(
     item_index: u32,
     item: wasmtime_runtime::VMCallerCheckedAnyfunc,
 ) -> Result<()> {
-    if let Some(item_ref) = handle.table_get_mut(table_index, item_index) {
-        *item_ref = item;
-        Ok(())
-    } else {
-        bail!("table element index out of bounds")
-    }
+    handle
+        .table_set(table_index, item_index, item)
+        .map_err(|()| anyhow!("table element index out of bounds"))
 }
 
 impl Table {

crates/api/src/instance.rs

@@ -25,18 +25,20 @@ fn instantiate(
     imports: &[Extern],
 ) -> Result<InstanceHandle, Error> {
     let mut resolver = SimpleResolver { imports };
-    let instance = compiled_module
-        .instantiate(&mut resolver)
-        .map_err(|e| -> Error {
-            if let Some(trap) = take_api_trap() {
-                trap.into()
-            } else if let InstantiationError::StartTrap(trap) = e {
-                Trap::from_jit(trap).into()
-            } else {
-                e.into()
-            }
-        })?;
-    Ok(instance)
+    unsafe {
+        let instance = compiled_module
+            .instantiate(&mut resolver)
+            .map_err(|e| -> Error {
+                if let Some(trap) = take_api_trap() {
+                    trap.into()
+                } else if let InstantiationError::StartTrap(trap) = e {
+                    Trap::from_jit(trap).into()
+                } else {
+                    e.into()
+                }
+            })?;
+        Ok(instance)
+    }
 }
 
 /// An instantiated WebAssembly module.
@@ -108,7 +110,7 @@ impl Instance {
     /// [issue]: https://github.com/bytecodealliance/wasmtime/issues/727
     pub fn new(module: &Module, imports: &[Extern]) -> Result<Instance, Error> {
         let store = module.store();
-        let mut instance_handle = instantiate(module.compiled_module(), imports)?;
+        let instance_handle = instantiate(module.compiled_module(), imports)?;
 
         let exports = {
             let mut exports = Vec::with_capacity(module.exports().len());
@@ -179,9 +181,8 @@ impl Instance {
     pub fn from_handle(store: &Store, instance_handle: InstanceHandle) -> Instance {
         let mut exports = Vec::new();
         let mut exports_types = Vec::new();
-        let mut mutable = instance_handle.clone();
-        for (name, _) in instance_handle.clone().exports() {
-            let export = mutable.lookup(name).expect("export");
+        for (name, _) in instance_handle.exports() {
+            let export = instance_handle.lookup(name).expect("export");
             if let wasmtime_runtime::Export::Function { signature, .. } = &export {
                 // HACK ensure all handles, instantiated outside Store, present in
                 // the store's SignatureRegistry, e.g. WASI instances that are
@@ -220,7 +221,6 @@ impl Instance {
 
     #[doc(hidden)]
     pub fn get_wasmtime_memory(&self) -> Option<wasmtime_runtime::Export> {
-        let mut instance_handle = self.instance_handle.clone();
-        instance_handle.lookup("memory")
+        self.instance_handle.lookup("memory")
     }
 }

crates/api/src/trampoline/create_handle.rs

@@ -36,14 +36,15 @@ pub(crate) fn create_handle(
         })
         .unwrap_or_else(PrimaryMap::new);
 
-    Ok(InstanceHandle::new(
-        Arc::new(module),
-        finished_functions.into_boxed_slice(),
-        imports,
-        &data_initializers,
-        signatures.into_boxed_slice(),
-        None,
-        state,
-    )
-    .expect("instance"))
+    unsafe {
+        Ok(InstanceHandle::new(
+            Arc::new(module),
+            finished_functions.into_boxed_slice(),
+            imports,
+            &data_initializers,
+            signatures.into_boxed_slice(),
+            None,
+            state,
+        )?)
+    }
 }

crates/api/src/trampoline/func.rs

@@ -70,7 +70,7 @@ unsafe extern "C" fn stub_fn(
     call_id: u32,
     values_vec: *mut i128,
 ) -> u32 {
-    let mut instance = InstanceHandle::from_vmctx(vmctx);
+    let instance = InstanceHandle::from_vmctx(vmctx);
 
     let (args, returns_len) = {
         let module = instance.module_ref();
@@ -89,7 +89,7 @@ unsafe extern "C" fn stub_fn(
     let mut returns = vec![Val::null(); returns_len];
     let func = &instance
         .host_state()
-        .downcast_mut::<TrampolineState>()
+        .downcast_ref::<TrampolineState>()
         .expect("state")
         .func;
 

crates/api/src/trampoline/global.rs

@@ -34,12 +34,12 @@ pub fn create_global(gt: &GlobalType, val: Val) -> Result<(wasmtime_runtime::Exp
         },
         initializer: wasm::GlobalInit::Import, // TODO is it right?
     };
-    let mut handle =
+    let handle =
         create_handle(Module::new(), None, PrimaryMap::new(), Box::new(())).expect("handle");
     Ok((
         wasmtime_runtime::Export::Global {
             definition: definition.as_mut(),
-            vmctx: handle.vmctx_mut_ptr(),
+            vmctx: handle.vmctx_ptr(),
             global,
         },
         GlobalState { definition, handle },

crates/api/src/trampoline/mod.rs

@@ -23,7 +23,7 @@ pub fn generate_func_export(
     func: &Rc<dyn Callable + 'static>,
     store: &Store,
 ) -> Result<(wasmtime_runtime::InstanceHandle, wasmtime_runtime::Export)> {
-    let mut instance = create_handle_with_function(ft, func, store)?;
+    let instance = create_handle_with_function(ft, func, store)?;
     let export = instance.lookup("trampoline").expect("trampoline export");
     Ok((instance, export))
 }
@@ -38,7 +38,7 @@ pub fn generate_global_export(
 pub fn generate_memory_export(
     m: &MemoryType,
 ) -> Result<(wasmtime_runtime::InstanceHandle, wasmtime_runtime::Export)> {
-    let mut instance = create_handle_with_memory(m)?;
+    let instance = create_handle_with_memory(m)?;
     let export = instance.lookup("memory").expect("memory export");
     Ok((instance, export))
 }
@@ -46,7 +46,7 @@ pub fn generate_memory_export(
 pub fn generate_table_export(
     t: &TableType,
 ) -> Result<(wasmtime_runtime::InstanceHandle, wasmtime_runtime::Export)> {
-    let mut instance = create_handle_with_table(t)?;
+    let instance = create_handle_with_table(t)?;
     let export = instance.lookup("table").expect("table export");
     Ok((instance, export))
 }

crates/api/src/values.rs

@@ -217,7 +217,7 @@ pub(crate) fn into_checked_anyfunc(
 }
 
 pub(crate) fn from_checked_anyfunc(
-    item: &wasmtime_runtime::VMCallerCheckedAnyfunc,
+    item: wasmtime_runtime::VMCallerCheckedAnyfunc,
     store: &Store,
 ) -> Val {
     if item.type_index == wasmtime_runtime::VMSharedSignatureIndex::default() {