bytecodealliance / bytecodealliance/wasmtime
DRC collector leaks GC refs when struct/array initialization fails partway through
- 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
Assessment
This issue has not been assessed yet.