bytecodealliance / bytecodealliance/wasmtime

DRC collector leaks GC refs when struct/array initialization fails partway through

Open
#12,456 3 comments 0 reactions 0 assignees View on GitHub
bug wasm-proposal:gc
Dominant language
Rust
Stars
18.6k
Forks
1.8k
Avg merge
1d 19h
Merged PRs (30d)
121

Description

### Test Case

```
#[test]
fn struct_new_init_failure_no_leak() -> Result<()> {
let mut store = crate::gc_store()?;
let ty = StructType::new(
store.engine(),
[
FieldType::new(Mutability::Var, StorageType::ValType(ValType::EXTERNREF)),
FieldType::new(Mutability::Var, StorageType::ValType(ValType::EXTERNREF)),
],
)?;
let pre = StructRefPre::new(&mut store, ty);
let dropped = Arc::new(AtomicBool::new(false));
{
let mut scope = RootScope::new(&mut store);
let good = ExternRef::new(&mut scope, SetFlagOnDrop(dropped.clone()))?;
// Create an unrooted ref by letting its scope expire.
let bad = {
let mut tmp = RootScope::new(&mut scope);
ExternRef::new(&mut tmp, 0u32)?
};
assert!(StructRef::new(
&mut scope,
&pre,
&[Val::ExternRef(Some(good)), Val::ExternRef(Some(bad))],
)
.is_err());
}
let _ = store.gc(None);
assert!(dropped.load(SeqCst), "field 0 externref was leaked");
Ok(())
}

#[test]
fn array_new_fixed_init_failure_no_leak() -> Result<()> {
let mut store = crate::gc_store()?;
let ty = ArrayType::new(
store.engine(),
FieldType::new(Mutability::Var, StorageType::ValType(ValType::EXTERNREF)),
);
let pre = ArrayRefPre::new(&mut store, ty);
let dropped = Arc::new(AtomicBool::new(false));
{
let mut scope = RootScope::new(&mut store);
let good = ExternRef::new(&mut scope, SetFlagOnDrop(dropped.clone()))?;
// Create an unrooted ref by letting its scope expire.
let bad = {
let mut tmp = RootScope::new(&mut scope);
ExternRef::new(&mut tmp, 0u32)?
};
assert!(ArrayRef::new_fixed(
&mut scope,
&pre,
&[Val::ExternRef(Some(good)), Val::ExternRef(Some(bad))],
)
.is_err());
}
let _ = store.gc(None);
assert!(dropped.load(SeqCst), "element 0 externref was leaked");
Ok(())
}
```

### Steps to Reproduce

Append the above two test cases to `tests/all/gc.rs`.

Run `cargo test --test all -- gc::struct_new_init_failure_no_leak gc::array_new_fixed_init_failure_no_leak`

### Expected Results

The `dropped` should have been collected and the tests should pass.

### Actual Results

The `dropped` not collected and the tests fail.

### Versions and Environment

Wasmtime version or commit: ab1ab705dd93e7c944c13d4d4b2683e049f4ff73

### Extra Info

https://github.com/bytecodealliance/wasmtime/blob/43832481378bc067eadeaa0fb8434c6b71bc22e1/crates/wasmtime/src/runtime/vm/gc/enabled/drc.rs#L860-L862

https://github.com/bytecodealliance/wasmtime/blob/43832481378bc067eadeaa0fb8434c6b71bc22e1/crates/wasmtime/src/runtime/vm/gc/enabled/drc.rs#L884-L886

Here the `dealloc_uninit_struct_or_exn` and `dealloc_uninit_array` free the object's memory but do not decrement the reference counts of already-initialized GC reference fields.

When `StructRef::new` or `ArrayRef::new_fixed` fails partway through field/element initialization, this causes those referenced objects to leak permanently.

The two test cases trigger this path by creating struct/array ref with one good field/element and one bad field/element.

I think the fix might look like this:

```
let gc_ref: VMGcRef = arrayref.into();
let mut children = Vec::new();
self.trace_gc_ref(&gc_ref, &mut children);
for child in &children {
self.dec_ref_and_maybe_dealloc(host_data_table, child);
}
self.dealloc(gc_ref);
```

but it's not a trivial one-liner:

1. To call `trace_gc_ref`, allocation functions (`alloc_uninit_struct_or_exn` and `alloc_uninit_array`) might need to zero-fill the fields since uninitialized GC ref fields may contain stale data from previous allocations.
2. To call `dec_ref_and_maybe_dealloc`, `&mut ExternRefHostDataTable` is needed to clean up externref host data but `dealloc_uninit_struct_or_exn` / `dealloc_uninit_array` don't pass this table through now.

As always, I'm happy to help prepare a fix, but since the fix is not trivial and it's GC code, maybe someone who knows this code better can help.

Thanks for looking into this!

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.