Skip to content
Permalink
Browse files Browse the repository at this point in the history
Merge pull request from GHSA-88xq-w8cq-xfg7
This commit fixes the drop of an uninitialized `VMExternRef` for an `externref`
global in an instance that failed to be allocated by the pooling instance
allocator.

The following engine configuration (via `Config`) is required to be impacted by
this bug:

* support for the reference types proposal must be enabled (this is the
  default for `Config`).
* a pooling allocation strategy must be configured via
  `Config::allocation_strategy`, which is not the default allocation strategy.

A module must be instantiated with the following characteristics:

* The module defines at least one table or memory.
* The module defines at least one `externref` global.

During instantiation, one of the following must occur to cause the
instantiation to fail:

* a call to `mprotect` or `VirtualAlloc` fails (e.g. out-of-memory conditions).
* a resource limiter was configured in the associated Store (via `Store::limiter`
  or `Store::limiter_async`) and the limiter returns false from the initial call
  to grow_memory or grow_table.

This results in a partially-initialized instance being dropped and that attempts
to drop the uninitialized `VMExternRef` representing the defined `externref` global.

The fix is to track whether or not the `VMContext` of the instance has been fully
initialized and skip the dropping of globals if not.
  • Loading branch information
peterhuene committed Feb 16, 2022
1 parent 39b88e4 commit 886ecc5
Show file tree
Hide file tree
Showing 3 changed files with 74 additions and 1 deletion.
12 changes: 11 additions & 1 deletion crates/runtime/src/instance.rs
Expand Up @@ -92,6 +92,10 @@ pub(crate) struct Instance {
/// allocation, but some host-defined objects will store their state here.
host_state: Box<dyn Any + Send + Sync>,

/// Flag to track when the vmctx has been initialized.
/// The pooling allocator may drop an instance before `vmctx` is initialized.
vmctx_initialized: bool,

/// Additional context used by compiled wasm code. This field is last, and
/// represents a dynamically-sized array that extends beyond the nominal
/// end of the struct (similar to a flexible array member).
Expand Down Expand Up @@ -119,6 +123,7 @@ impl Instance {
dropped_data: EntitySet::with_capacity(module.passive_data_map.len()),
host_state,
wasm_data,
vmctx_initialized: false,
vmctx: VMContext {
_marker: std::marker::PhantomPinned,
},
Expand Down Expand Up @@ -733,13 +738,18 @@ impl Instance {
}

fn drop_globals(&mut self) {
// Dropping globals requires that the vmctx be fully initialized
if !self.vmctx_initialized {
return;
}

for (idx, global) in self.module.globals.iter() {
let idx = match self.module.defined_global_index(idx) {
Some(idx) => idx,
None => continue,
};
match global.wasm_ty {
// For now only externref gloabls need to get destroyed
// For now only externref globals need to get destroyed
WasmType::ExternRef => {}
_ => continue,
}
Expand Down
5 changes: 5 additions & 0 deletions crates/runtime/src/instance/allocator.rs
Expand Up @@ -475,6 +475,8 @@ fn initialize_instance(
}

unsafe fn initialize_vmcontext(instance: &mut Instance, req: InstanceAllocationRequest) {
assert!(!instance.vmctx_initialized);

if let Some(store) = req.store.as_raw() {
*instance.interrupts() = (*store).vminterrupts();
*instance.epoch_ptr() = (*store).epoch_ptr();
Expand Down Expand Up @@ -570,6 +572,9 @@ unsafe fn initialize_vmcontext(instance: &mut Instance, req: InstanceAllocationR

// Initialize the defined globals
initialize_vmcontext_globals(instance);

// Mark the vmctx as initialized
instance.vmctx_initialized = true;
}

unsafe fn initialize_vmcontext_globals(instance: &Instance) {
Expand Down
58 changes: 58 additions & 0 deletions tests/all/pooling_allocator.rs
Expand Up @@ -511,3 +511,61 @@ fn preserve_data_segments() -> Result<()> {

Ok(())
}

#[test]
fn drop_externref_global_during_module_init() -> Result<()> {
struct Limiter;

impl ResourceLimiter for Limiter {
fn memory_growing(&mut self, _: usize, _: usize, _: Option<usize>) -> bool {
false
}

fn table_growing(&mut self, _: u32, _: u32, _: Option<u32>) -> bool {
false
}
}

let mut config = Config::new();
config.wasm_reference_types(true);
config.allocation_strategy(InstanceAllocationStrategy::Pooling {
strategy: PoolingAllocationStrategy::NextAvailable,
module_limits: Default::default(),
instance_limits: InstanceLimits { count: 1 },
});

let engine = Engine::new(&config)?;

let module = Module::new(
&engine,
r#"
(module
(global i32 (i32.const 1))
(global i32 (i32.const 2))
(global i32 (i32.const 3))
(global i32 (i32.const 4))
(global i32 (i32.const 5))
)
"#,
)?;

let mut store = Store::new(&engine, Limiter);
drop(Instance::new(&mut store, &module, &[])?);
drop(store);

let module = Module::new(
&engine,
r#"
(module
(memory 1)
(global (mut externref) (ref.null extern))
)
"#,
)?;

let mut store = Store::new(&engine, Limiter);
store.limiter(|s| s);
assert!(Instance::new(&mut store, &module, &[]).is_err());

Ok(())
}

0 comments on commit 886ecc5

Please sign in to comment.