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

[zzjas]

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: ab1ab70

Extra Info

wasmtime/crates/wasmtime/src/runtime/vm/gc/enabled/drc.rs

Lines 860 to 862 in 4383248

	fn dealloc_uninit_struct_or_exn(&mut self, gcref: VMGcRef) {
	self.dealloc(gcref);
	}

wasmtime/crates/wasmtime/src/runtime/vm/gc/enabled/drc.rs

Lines 884 to 886 in 4383248

	fn dealloc_uninit_array(&mut self, arrayref: VMArrayRef) {
	self.dealloc(arrayref.into())
	}

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!

[alexcrichton]

thanks @zzjas for the bug report, this is very thorough and much appreciated!

cc @fitzgen you're likely interested in this

[fitzgen]

Yeah we should handle this. Looks like you diagnosed the bug correctly.

I don't have time at the moment to fix it myself, but if you want to whip up a patch @zzjas, I'm happy to review it!

[zzjas]

Thank you both!

if you want to whip up a patch @zzjas, I'm happy to review it!

I'll work on the fix a bit later this week.