Merge #1590

1590: Fix soundness issue in vm::Global r=MarkMcCaskey a=MarkMcCaskey

The `unsafe impl` for `Send` and `Sync` relies on the global being
locked when it's being accessed; the get and get_mut functions did
not (and could not) do this, so we replace them with `get` and `set`
methods which operate on `Value`s directly.

Additionally the `get_mut` function took a `&self` and returned a
`&mut` to the underlying data which allows for aliased mutable
references to the same data which is another soundness issue.

# Review

- [x] Add a short description of the the change to the CHANGELOG.md file


Co-authored-by: Mark McCaskey <[email protected]>

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

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_unchecked(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.
@@ -90,30 +76,13 @@ impl Global {
     /// * The global is not mutable
     /// * The type of the `Val` doesn't matches the Global type.
     pub fn set(&self, val: Val) -> Result<(), RuntimeError> {
-        if self.ty().mutability != Mutability::Var {
-            return Err(RuntimeError::new(
-                "immutable global cannot be set".to_string(),
-            ));
-        }
-        if val.ty() != self.ty().ty {
-            return Err(RuntimeError::new(format!(
-                "global of type {:?} cannot be set to {:?}",
-                self.ty().ty,
-                val.ty()
-            )));
-        }
         if !val.comes_from_same_store(&self.store) {
             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!("{}", e)))?;
         }
         Ok(())
     }

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<T>(&self) -> Value<T> {
+        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<T>(&self, val: Value<T>) -> 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<T>(&self, val: Value<T>) -> 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(())
     }
 }