Skip to content

Commit

Permalink
Merge #1590
Browse files Browse the repository at this point in the history
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 <mark@wasmer.io>
  • Loading branch information
bors[bot] and Mark McCaskey authored Sep 3, 2020
2 parents faf93d9 + b407144 commit 1470a0a
Show file tree
Hide file tree
Showing 3 changed files with 60 additions and 34 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -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
Expand Down
35 changes: 8 additions & 27 deletions lib/api/src/externals/global.rs
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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 {
Expand All @@ -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.
Expand All @@ -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(())
}
Expand Down
58 changes: 51 additions & 7 deletions lib/vm/src/global.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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(())
}
}

0 comments on commit 1470a0a

Please sign in to comment.