Fix leaks of already-initialized fields when struct/array init fails

[zzjas]

When StructRef::new or ArrayRef::new_fixed fails midway, dealloc_uninit frees the object without dec-refing GC refs already written to earlier fields, leaking them. Fix by zero-filling the object body in alloc_raw so trace_gc_ref skips uninitialized slots, then tracing and dec-refing children in dealloc_uninit before freeing.

Fixes #12456

[zzjas]

cc: @fitzgen

[github-actions]

Subscribe to Label Action

cc @fitzgen

Details

This issue or pull request has been labeled: "wasmtime:api", "wasmtime:ref-types"

Thus the following users have been cc'd because of the following labels:

• fitzgen: wasmtime:ref-types

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

[alexcrichton]

Thanks for this!

Thinking about this a bit more though, it feels like it's a lot of infrastructure here for not a ton of benefit. For example this pessimizes all allocations to be zero-filled instead of truly uninitialized. This isn't even used by compiled guest wasm and it's also only used on the host, a secondary vector for implementing GC things.

Given all that, I'm wondering if we should step back a bit and reconsider the basic strategy here. For example I would naively expect that one way to do this would be to enhance the type-check we have for initializing structs/arrays/etc on the host such that the initialization step never fails. We already do a type-check there and it looks like the externref bits aren't fully checked there. One other way to tackle this problem would be to (1) enhance the type-check to avoid failing during initialization and (2) use .unwrap() to assert that per-field initialization succeeds.

cc @fitzgen on this as well, you likely have thoughts too! I also think there's a case to be made for "let's fix the bug first and then make it fast later", in which case I think this PR is perfectly suitable. The only downside is that it puts things in a bit of a weird state where the "uninit" terminology is now more of a "maybe uninit maybe not" which is a bit confusing. Not enough to block landing this though, but that's what led me to reconsider the overall approach.

[fitzgen]

I like the idea of checking that references are properly rooted at type-checking time, and unwraping during field initialization.

My other suggestion would have been to keep track of how many fields have been initialized and then drop the initialized fields in an Undo, but I like checking ahead of time better.