From 4283fdc862ad2158607afb60703c61ed95846f24 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Wed, 19 Feb 2020 20:57:41 -0600 Subject: [PATCH] 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 | 44 ++++++-------- crates/api/src/trampoline/global.rs | 51 ++++++---------- crates/api/src/trampoline/mod.rs | 8 +-- crates/api/tests/globals.rs | 93 +++++++++++++++++++++++++++++ 4 files changed, 133 insertions(+), 63 deletions(-) create mode 100644 crates/api/tests/globals.rs diff --git a/crates/api/src/externals.rs b/crates/api/src/externals.rs index 70d796e047..12a0c5ed34 100644 --- a/crates/api/src/externals.rs +++ b/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 for Extern { /// instances are equivalent in their functionality. #[derive(Clone)] pub struct Global { - inner: Rc, -} - -struct GlobalInner { _store: Store, ty: GlobalType, wasmtime_export: wasmtime_runtime::Export, - #[allow(dead_code)] - wasmtime_state: Option, + 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), - }), + _store: store.clone(), + ty, + wasmtime_export, + 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, - }), + _store: store.clone(), + ty: ty, + wasmtime_export: export, + wasmtime_handle, } } } diff --git a/crates/api/src/trampoline/global.rs b/crates/api/src/trampoline/global.rs index c36226f923..8820749a2e 100644 --- a/crates/api/src/trampoline/global.rs +++ b/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, - 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 { 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) } diff --git a/crates/api/src/trampoline/mod.rs b/crates/api/src/trampoline/mod.rs index d65906ae12..287ae66b5e 100644 --- a/crates/api/src/trampoline/mod.rs +++ b/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, @@ -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( diff --git a/crates/api/tests/globals.rs b/crates/api/tests/globals.rs new file mode 100644 index 0000000000..8f5406d398 --- /dev/null +++ b/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(()) +}