diff --git a/CHANGELOG.md b/CHANGELOG.md index 2480b2996a2..9d24118bfb4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,7 @@ # Changelog ## **[Unreleased]** +- [#1590](https://github.com/wasmerio/wasmer/pull/1590) Fix soundness issue in API of vm::Global - [#1566](https://github.com/wasmerio/wasmer/pull/1566) Add support for opening special Unix files to the WASI FS ## TODO: 1.0.0-alpha1.0 diff --git a/lib/api/src/externals/global.rs b/lib/api/src/externals/global.rs index 7203700db76..c1907e3afac 100644 --- a/lib/api/src/externals/global.rs +++ b/lib/api/src/externals/global.rs @@ -1,7 +1,7 @@ use crate::exports::{ExportError, Exportable}; use crate::externals::Extern; use crate::store::{Store, StoreObject}; -use crate::types::{Val, ValType}; +use crate::types::Val; use crate::GlobalType; use crate::Mutability; use crate::RuntimeError; @@ -42,14 +42,9 @@ impl Global { ty: val.ty(), }); unsafe { - match val { - Val::I32(x) => *global.get_mut().as_i32_mut() = x, - Val::I64(x) => *global.get_mut().as_i64_mut() = x, - Val::F32(x) => *global.get_mut().as_f32_mut() = x, - Val::F64(x) => *global.get_mut().as_f64_mut() = x, - Val::V128(x) => *global.get_mut().as_u128_bits_mut() = x.to_ne_bytes(), - _ => return Err(RuntimeError::new(format!("create_global for {:?}", val))), - } + global + .set(val.clone()) + .map_err(|e| RuntimeError::new(format!("create global for {:?}: {}", val, e)))?; }; Ok(Global { @@ -70,16 +65,7 @@ impl Global { /// Retrieves the current value [`Val`] that the Global has. pub fn get(&self) -> Val { - unsafe { - let definition = self.global.get(); - match self.ty().ty { - ValType::I32 => Val::from(*definition.as_i32()), - ValType::I64 => Val::from(*definition.as_i64()), - ValType::F32 => Val::F32(*definition.as_f32()), - ValType::F64 => Val::F64(*definition.as_f64()), - _ => unimplemented!("Global::get for {:?}", self.ty().ty), - } - } + self.global.get() } /// Sets a custom value [`Val`] to the runtime Global. @@ -106,14 +92,9 @@ impl Global { return Err(RuntimeError::new("cross-`Store` values are not supported")); } unsafe { - let definition = self.global.get_mut(); - match val { - Val::I32(i) => *definition.as_i32_mut() = i, - Val::I64(i) => *definition.as_i64_mut() = i, - Val::F32(f) => *definition.as_f32_mut() = f, - Val::F64(f) => *definition.as_f64_mut() = f, - _ => unimplemented!("Global::set for {:?}", val.ty()), - } + self.global + .set(val) + .map_err(|e| RuntimeError::new(format!("Failed to set global: {}", e)))?; } Ok(()) } diff --git a/lib/vm/src/global.rs b/lib/vm/src/global.rs index 1f479072a7b..d1ff6937fd4 100644 --- a/lib/vm/src/global.rs +++ b/lib/vm/src/global.rs @@ -3,7 +3,7 @@ use std::cell::UnsafeCell; use std::ptr::NonNull; use std::sync::Mutex; use thiserror::Error; -use wasmer_types::{GlobalType, Type}; +use wasmer_types::{GlobalType, Mutability, Type, Value}; #[derive(Debug)] /// TODO: figure out a decent name for this thing @@ -63,13 +63,57 @@ impl Global { unsafe { NonNull::new_unchecked(ptr) } } - /// Get a reference to the definition - pub fn get(&self) -> &VMGlobalDefinition { - unsafe { &*self.vm_global_definition.get() } + /// Get a value from the global. + pub fn get(&self) -> Value { + let _global_guard = self.lock.lock().unwrap(); + unsafe { + let definition = &*self.vm_global_definition.get(); + match self.ty().ty { + Type::I32 => Value::I32(*definition.as_i32()), + Type::I64 => Value::I64(*definition.as_i64()), + Type::F32 => Value::F32(*definition.as_f32()), + Type::F64 => Value::F64(*definition.as_f64()), + Type::V128 => Value::V128(*definition.as_u128()), + _ => unimplemented!("Global::get for {:?}", self.ty), + } + } + } + + /// Set a value for the global. + /// + /// # Safety + /// The caller should check that the `val` comes from the same store as this global. + pub unsafe fn set(&self, val: Value) -> Result<(), GlobalError> { + let _global_guard = self.lock.lock().unwrap(); + if self.ty().mutability != Mutability::Var { + return Err(GlobalError::ImmutableGlobalCannotBeSet); + } + if val.ty() != self.ty().ty { + return Err(GlobalError::IncorrectType { + expected: self.ty.ty, + found: val.ty(), + }); + } + self.set_unchecked(val) } - /// Get a mutable reference to the definition - pub fn get_mut(&self) -> &mut VMGlobalDefinition { - unsafe { &mut *self.vm_global_definition.get() } + /// Set a value from the global (unchecked) + /// + /// # Safety + /// The caller should check that the `val` comes from the same store as this global. + /// The caller should also ensure that this global is synchronized. Otherwise, use + /// `set` instead. + pub unsafe fn set_unchecked(&self, val: Value) -> Result<(), GlobalError> { + // ideally we'd use atomics for the global value rather than needing to lock it + let definition = &mut *self.vm_global_definition.get(); + match val { + Value::I32(i) => *definition.as_i32_mut() = i, + Value::I64(i) => *definition.as_i64_mut() = i, + Value::F32(f) => *definition.as_f32_mut() = f, + Value::F64(f) => *definition.as_f64_mut() = f, + Value::V128(x) => *definition.as_u128_bits_mut() = x.to_ne_bytes(), + _ => unimplemented!("Global::set for {:?}", val.ty()), + } + Ok(()) } }