T0b1/wasmtime
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Revamp memory management of InstanceHandle (#1624)

Browse Source 
* Revamp memory management of `InstanceHandle`

This commit fixes a known but in Wasmtime where an instance could still
be used after it was freed. Unfortunately the fix here is a bit of a
hammer, but it's the best that we can do for now. The changes made in
this commit are:

* A `Store` now stores all `InstanceHandle` objects it ever creates.
  This keeps all instances alive unconditionally (along with all host
  functions and such) until the `Store` is itself dropped. Note that a
  `Store` is reference counted so basically everything has to be dropped
  to drop anything, there's no longer any partial deallocation of instances.

* The `InstanceHandle` type's own reference counting has been removed.
  This is largely redundant with what's already happening in `Store`, so
  there's no need to manage two reference counts.

* Each `InstanceHandle` no longer tracks its dependencies in terms of
  instance handles. This set was actually inaccurate due to dynamic
  updates to tables and such, so we needed to revamp it anyway.

* Initialization of an `InstanceHandle` is now deferred until after
  `InstanceHandle::new`. This allows storing the `InstanceHandle` before
  side-effectful initialization, such as copying element segments or
  running the start function, to ensure that regardless of the result of
  instantiation the underlying `InstanceHandle` is still available to
  persist in storage.

Overall this should fix a known possible way to safely segfault Wasmtime
today (yay!) and it should also fix some flaikness I've seen on CI.
Turns out one of the spec tests
(bulk-memory-operations/partial-init-table-segment.wast) exercises this
functionality and we were hitting sporating use-after-free, but only on
Windows.

* Shuffle some APIs around

* Comment weak cycle
This commit is contained in:
Alex Crichton
2020-04-29 12:47:49 -05:00
committed by GitHub
parent 738e2742da
commit 654e953fbf
21 changed files with 315 additions and 256 deletions
Download Patch File Download Diff File Expand all files Collapse all files

101
crates/runtime/src/instance.rs
View File

@@ -20,7 +20,7 @@ use more_asserts::assert_lt;
use std::alloc::{self, Layout};
use std::any::Any;
use std::cell::{Cell, RefCell};
use std::collections::{HashMap, HashSet};
use std::collections::HashMap;
use std::convert::TryFrom;
use std::rc::Rc;
use std::sync::Arc;
@@ -39,7 +39,7 @@ cfg_if::cfg_if! {
impl InstanceHandle {
/// Set a custom signal handler
pub fn set_signal_handler<H>(&mut self, handler: H)
pub fn set_signal_handler<H>(&self, handler: H)
where
H: 'static + Fn(libc::c_int, *const libc::siginfo_t, *const libc::c_void) -> bool,
{
@@ -51,7 +51,7 @@ cfg_if::cfg_if! {
impl InstanceHandle {
/// Set a custom signal handler
pub fn set_signal_handler<H>(&mut self, handler: H)
pub fn set_signal_handler<H>(&self, handler: H)
where
H: 'static + Fn(winapi::um::winnt::PEXCEPTION_POINTERS) -> bool,
{
@@ -66,14 +66,6 @@ cfg_if::cfg_if! {
/// This is repr(C) to ensure that the vmctx field is last.
#[repr(C)]
pub(crate) struct Instance {
/// The number of references to this `Instance`.
refcount: Cell<usize>,
/// `Instance`s from which this `Instance` imports. These won't
/// create reference cycles because wasm instances can't cyclically
/// import from each other.
dependencies: HashSet<InstanceHandle>,
/// The `Module` this `Instance` was instantiated from.
module: Arc<Module>,
@@ -878,13 +870,10 @@ impl InstanceHandle {
trampolines: HashMap<VMSharedSignatureIndex, VMTrampoline>,
imports: Imports,
mem_creator: Option<&dyn RuntimeMemoryCreator>,
data_initializers: &[DataInitializer<'_>],
vmshared_signatures: BoxedSlice<SignatureIndex, VMSharedSignatureIndex>,
dbg_jit_registration: Option<Rc<GdbJitImageRegistration>>,
is_bulk_memory: bool,
host_state: Box<dyn Any>,
interrupts: Arc<VMInterrupts>,
max_wasm_stack: usize,
) -> Result<Self, InstantiationError> {
let tables = create_tables(&module);
let memories = create_memories(&module, mem_creator.unwrap_or(&DefaultMemoryCreator {}))?;
@@ -909,8 +898,6 @@ impl InstanceHandle {
let handle = {
let instance = Instance {
refcount: Cell::new(1),
dependencies: imports.dependencies,
module,
offsets,
memories,
@@ -983,30 +970,44 @@ impl InstanceHandle {
);
*instance.interrupts() = &*instance.interrupts;
// Ensure that our signal handlers are ready for action.
// TODO: Move these calls out of `InstanceHandle`.
traphandlers::init();
// Perform infallible initialization in this constructor, while fallible
// initialization is deferred to the `initialize` method.
initialize_passive_elements(instance);
initialize_globals(instance);
Ok(handle)
}
/// Finishes the instantiation process started by `Instance::new`.
///
/// Only safe to call immediately after instantiation.
pub unsafe fn initialize(
&self,
is_bulk_memory: bool,
max_wasm_stack: usize,
data_initializers: &[DataInitializer<'_>],
) -> Result<(), InstantiationError> {
// Check initializer bounds before initializing anything. Only do this
// when bulk memory is disabled, since the bulk memory proposal changes
// instantiation such that the intermediate results of failed
// initializations are visible.
if !is_bulk_memory {
check_table_init_bounds(instance)?;
check_memory_init_bounds(instance, data_initializers)?;
check_table_init_bounds(self.instance())?;
check_memory_init_bounds(self.instance(), data_initializers)?;
}
// Apply the initializers.
initialize_tables(instance)?;
initialize_passive_elements(instance);
initialize_memories(instance, data_initializers)?;
initialize_globals(instance);
// Apply fallible initializers. Note that this can "leak" state even if
// it fails.
initialize_tables(self.instance())?;
initialize_memories(self.instance(), data_initializers)?;
// Ensure that our signal handlers are ready for action.
// TODO: Move these calls out of `InstanceHandle`.
traphandlers::init();
// The WebAssembly spec specifies that the start function is
// invoked automatically at instantiation time.
instance.invoke_start_function(max_wasm_stack)?;
Ok(handle)
// And finally, invoke the start function.
self.instance().invoke_start_function(max_wasm_stack)?;
Ok(())
}
/// Create a new `InstanceHandle` pointing at the instance
@@ -1017,7 +1018,6 @@ impl InstanceHandle {
/// be a `VMContext` allocated as part of an `Instance`.
pub unsafe fn from_vmctx(vmctx: *mut VMContext) -> Self {
let instance = (&mut *vmctx).instance();
instance.refcount.set(instance.refcount.get() + 1);
Self {
instance: instance as *const Instance as *mut Instance,
}
@@ -1130,30 +1130,29 @@ impl InstanceHandle {
pub(crate) fn instance(&self) -> &Instance {
unsafe { &*(self.instance as *const Instance) }
}
}
impl Clone for InstanceHandle {
fn clone(&self) -> Self {
let instance = self.instance();
instance.refcount.set(instance.refcount.get() + 1);
Self {
/// Returns a clone of this instance.
///
/// This is unsafe because the returned handle here is just a cheap clone
/// of the internals, there's no lifetime tracking around its validity.
/// You'll need to ensure that the returned handles all go out of scope at
/// the same time.
pub unsafe fn clone(&self) -> InstanceHandle {
InstanceHandle {
instance: self.instance,
}
}
}
impl Drop for InstanceHandle {
fn drop(&mut self) {
/// Deallocates memory associated with this instance.
///
/// Note that this is unsafe because there might be other handles to this
/// `InstanceHandle` elsewhere, and there's nothing preventing usage of
/// this handle after this function is called.
pub unsafe fn dealloc(&self) {
let instance = self.instance();
let count = instance.refcount.get();
instance.refcount.set(count - 1);
if count == 1 {
let layout = instance.alloc_layout();
unsafe {
ptr::drop_in_place(self.instance);
alloc::dealloc(self.instance.cast(), layout);
}
}
let layout = instance.alloc_layout();
ptr::drop_in_place(self.instance);
alloc::dealloc(self.instance.cast(), layout);
}
}