Fix a possible use-after-free with `Global` (#956)

* Fix a possible use-after-free with `Global`

This commit fixes an issue with the implementation of the
`wasmtime::Global` type where if it previously outlived the original
`Instance` it came from then you could run into a use-after-free. Now
the `Global` type holds onto its underlying `InstanceHandle` to ensure
it retains ownership of the underlying backing store of the global's
memory.

* rustfmt

crates/api/src/externals.rs

@@ -4,7 +4,6 @@ use crate::Mutability;
 use crate::{ExternType, GlobalType, MemoryType, TableType, ValType};
 use crate::{Func, Store};
 use anyhow::{anyhow, bail, Result};
-use std::rc::Rc;
 use std::slice;
 use wasmtime_environ::wasm;
 use wasmtime_runtime::InstanceHandle;
@@ -104,7 +103,7 @@ impl Extern {
                 Extern::Memory(Memory::from_wasmtime_memory(export, store, instance_handle))
             }
             wasmtime_runtime::Export::Global { .. } => {
-                Extern::Global(Global::from_wasmtime_global(export, store))
+                Extern::Global(Global::from_wasmtime_global(export, store, instance_handle))
             }
             wasmtime_runtime::Export::Table { .. } => {
                 Extern::Table(Table::from_wasmtime_table(export, store, instance_handle))
@@ -154,15 +153,10 @@ impl From<Table> for Extern {
 /// instances are equivalent in their functionality.
 #[derive(Clone)]
 pub struct Global {
-    inner: Rc<GlobalInner>,
-}
-
-struct GlobalInner {
     _store: Store,
     ty: GlobalType,
     wasmtime_export: wasmtime_runtime::Export,
-    #[allow(dead_code)]
-    wasmtime_state: Option<crate::trampoline::GlobalState>,
+    wasmtime_handle: InstanceHandle,
 }
 
 impl Global {
@@ -181,24 +175,22 @@ impl Global {
         if val.ty() != *ty.content() {
             bail!("value provided does not match the type of this global");
         }
-        let (wasmtime_export, wasmtime_state) = generate_global_export(store, &ty, val)?;
+        let (wasmtime_handle, wasmtime_export) = generate_global_export(store, &ty, val)?;
         Ok(Global {
-            inner: Rc::new(GlobalInner {
             _store: store.clone(),
             ty,
             wasmtime_export,
-                wasmtime_state: Some(wasmtime_state),
-            }),
+            wasmtime_handle,
         })
     }
 
     /// Returns the underlying type of this `global`.
     pub fn ty(&self) -> &GlobalType {
-        &self.inner.ty
+        &self.ty
     }
 
     fn wasmtime_global_definition(&self) -> *mut wasmtime_runtime::VMGlobalDefinition {
-        match self.inner.wasmtime_export {
+        match self.wasmtime_export {
             wasmtime_runtime::Export::Global { definition, .. } => definition,
             _ => panic!("global definition not found"),
         }
@@ -249,10 +241,14 @@ impl Global {
     }
 
     pub(crate) fn wasmtime_export(&self) -> &wasmtime_runtime::Export {
-        &self.inner.wasmtime_export
+        &self.wasmtime_export
     }
 
-    pub(crate) fn from_wasmtime_global(export: wasmtime_runtime::Export, store: &Store) -> Global {
+    pub(crate) fn from_wasmtime_global(
+        export: wasmtime_runtime::Export,
+        store: &Store,
+        wasmtime_handle: InstanceHandle,
+    ) -> Global {
         let global = if let wasmtime_runtime::Export::Global { ref global, .. } = export {
             global
         } else {
@@ -263,12 +259,10 @@ impl Global {
         let ty = GlobalType::from_wasmtime_global(&global)
             .expect("core wasm global type should be supported");
         Global {
-            inner: Rc::new(GlobalInner {
             _store: store.clone(),
             ty: ty,
             wasmtime_export: export,
-                wasmtime_state: None,
-            }),
+            wasmtime_handle,
         }
     }
 }

crates/api/src/trampoline/global.rs

@@ -4,30 +4,9 @@ use crate::{GlobalType, Mutability, Val};
 use anyhow::{bail, Result};
 use wasmtime_environ::entity::PrimaryMap;
 use wasmtime_environ::{wasm, Module};
-use wasmtime_runtime::{InstanceHandle, VMGlobalDefinition};
-
-#[allow(dead_code)]
-pub struct GlobalState {
-    definition: Box<VMGlobalDefinition>,
-    handle: InstanceHandle,
-}
-
-pub fn create_global(
-    store: &Store,
-    gt: &GlobalType,
-    val: Val,
-) -> Result<(wasmtime_runtime::Export, GlobalState)> {
-    let mut definition = Box::new(VMGlobalDefinition::new());
-    unsafe {
-        match val {
-            Val::I32(i) => *definition.as_i32_mut() = i,
-            Val::I64(i) => *definition.as_i64_mut() = i,
-            Val::F32(f) => *definition.as_u32_mut() = f,
-            Val::F64(f) => *definition.as_u64_mut() = f,
-            _ => unimplemented!("create_global for {:?}", gt),
-        }
-    }
+use wasmtime_runtime::InstanceHandle;
 
+pub fn create_global(store: &Store, gt: &GlobalType, val: Val) -> Result<InstanceHandle> {
     let global = wasm::Global {
         ty: match gt.content().get_wasmtime_type() {
             Some(t) => t,
@@ -37,16 +16,20 @@ pub fn create_global(
             Mutability::Const => false,
             Mutability::Var => true,
         },
-        initializer: wasm::GlobalInit::Import, // TODO is it right?
-    };
-    let handle =
-        create_handle(Module::new(), store, PrimaryMap::new(), Box::new(())).expect("handle");
-    Ok((
-        wasmtime_runtime::Export::Global {
-            definition: definition.as_mut(),
-            vmctx: handle.vmctx_ptr(),
-            global,
+        initializer: match val {
+            Val::I32(i) => wasm::GlobalInit::I32Const(i),
+            Val::I64(i) => wasm::GlobalInit::I64Const(i),
+            Val::F32(f) => wasm::GlobalInit::F32Const(f),
+            Val::F64(f) => wasm::GlobalInit::F64Const(f),
+            _ => unimplemented!("create_global for {:?}", gt),
         },
-        GlobalState { definition, handle },
-    ))
+    };
+    let mut module = Module::new();
+    let global_id = module.globals.push(global);
+    module.exports.insert(
+        "global".to_string(),
+        wasmtime_environ::Export::Global(global_id),
+    );
+    let handle = create_handle(module, store, PrimaryMap::new(), Box::new(()))?;
+    Ok(handle)
 }

crates/api/src/trampoline/mod.rs

@@ -16,8 +16,6 @@ use std::any::Any;
 use std::rc::Rc;
 use wasmtime_runtime::VMFunctionBody;
 
-pub use self::global::GlobalState;
-
 pub fn generate_func_export(
     ft: &FuncType,
     func: &Rc<dyn Callable + 'static>,
@@ -46,8 +44,10 @@ pub fn generate_global_export(
     store: &Store,
     gt: &GlobalType,
     val: Val,
-) -> Result<(wasmtime_runtime::Export, GlobalState)> {
-    create_global(store, gt, val)
+) -> Result<(wasmtime_runtime::InstanceHandle, wasmtime_runtime::Export)> {
+    let instance = create_global(store, gt, val)?;
+    let export = instance.lookup("global").expect("global export");
+    Ok((instance, export))
 }
 
 pub fn generate_memory_export(

crates/api/tests/globals.rs

@@ -0,0 +1,93 @@
+use wasmtime::*;
+
+#[test]
+fn smoke() -> anyhow::Result<()> {
+    let store = Store::default();
+    let g = Global::new(
+        &store,
+        GlobalType::new(ValType::I32, Mutability::Const),
+        0.into(),
+    )?;
+    assert_eq!(g.get().i32(), Some(0));
+    assert!(g.set(0.into()).is_err());
+
+    let g = Global::new(
+        &store,
+        GlobalType::new(ValType::I32, Mutability::Const),
+        1i32.into(),
+    )?;
+    assert_eq!(g.get().i32(), Some(1));
+
+    let g = Global::new(
+        &store,
+        GlobalType::new(ValType::I64, Mutability::Const),
+        2i64.into(),
+    )?;
+    assert_eq!(g.get().i64(), Some(2));
+
+    let g = Global::new(
+        &store,
+        GlobalType::new(ValType::F32, Mutability::Const),
+        3.0f32.into(),
+    )?;
+    assert_eq!(g.get().f32(), Some(3.0));
+
+    let g = Global::new(
+        &store,
+        GlobalType::new(ValType::F64, Mutability::Const),
+        4.0f64.into(),
+    )?;
+    assert_eq!(g.get().f64(), Some(4.0));
+    Ok(())
+}
+
+#[test]
+fn mutability() -> anyhow::Result<()> {
+    let store = Store::default();
+    let g = Global::new(
+        &store,
+        GlobalType::new(ValType::I32, Mutability::Var),
+        0.into(),
+    )?;
+    assert_eq!(g.get().i32(), Some(0));
+    g.set(1.into())?;
+    assert_eq!(g.get().i32(), Some(1));
+    Ok(())
+}
+
+// Make sure that a global is still usable after its original instance is
+// dropped. This is a bit of a weird test and really only fails depending on the
+// implementation, but for now should hopefully be resilient enough to catch at
+// least some cases of heap corruption.
+#[test]
+fn use_after_drop() -> anyhow::Result<()> {
+    let store = Store::default();
+    let module = Module::new(
+        &store,
+        r#"
+            (module
+                (global (export "foo") (mut i32) (i32.const 100)))
+        "#,
+    )?;
+    let instance = Instance::new(&module, &[])?;
+    let g = instance.exports()[0].global().unwrap().clone();
+    assert_eq!(g.get().i32(), Some(100));
+    g.set(101.into())?;
+    drop(instance);
+    assert_eq!(g.get().i32(), Some(101));
+    Instance::new(&module, &[])?;
+    assert_eq!(g.get().i32(), Some(101));
+    drop(module);
+    assert_eq!(g.get().i32(), Some(101));
+    drop(store);
+    assert_eq!(g.get().i32(), Some(101));
+
+    // spray some heap values
+    let mut x = Vec::new();
+    for _ in 0..100 {
+        x.push("xy".to_string());
+    }
+    drop(x);
+    assert_eq!(g.get().i32(), Some(101));
+    Ok(())
+}