wasmtime / PR #11605 Rec struct · git-wasmtime

Subscribe to Label Action

cc @fitzgen

<details>
This issue or pull request has been labeled: "fuzzing"

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

fitzgen: fuzzing

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

Learn more.
</details>

Wasmtime GitHub notifications bot (Sep 04 2025 at 14:30):

alexcrichton requested fitzgen for a review on PR #11605.

Wasmtime GitHub notifications bot (Sep 04 2025 at 22:50):

fitzgen submitted PR review:

Good start, although I think we need some tweaks before this can land.

One thing I don't see is any mutators for our types and rec groups (e.g. add/remove types, switch a type from one rec group to another, etc). Is the plan that we will add mutators in follow up PRs? That is fine, but I just want to make sure it isn't overlooked.

Thanks!

Wasmtime GitHub notifications bot (Sep 04 2025 at 22:50):

fitzgen created PR review comment:

None of these methods seem to be used?

Wasmtime GitHub notifications bot (Sep 04 2025 at 22:50):

fitzgen created PR review comment:

We specifically do not want to evenly distribute types between rec groups. We want to explore the weird edge cases where all types are in one rec group and all other rec groups are empty, etc. Also, we already have a mapping from type to rec group in the form of the SubType::rec_group field and we should use that mapping, not ignore it.

Instead, I would expect something like this:

// Gather all rec groups and types by rec group.
let mut rec_groups = self.types
                         .rec_groups
                         .iter()
                         .map(|id| (id, vec![]))
                         .collect::<BTreeMap<RecGroupId, Vec<TypeId>>();
for (id, ty) in self.types.type_defs.iter() {
    rec_groups.entry(ty.rec_group).or_default().push(id);
}

// Encode those rec groups and their types into the types section.
for type_ids in rec_groups.values() {
    types.ty().rec(
        type_ids
            .iter()
            .map(|ty_id| todo!("create a `wasm_encoder::SubType` for the type with id `ty_id`"))
            .collect::<Vec<_>>(),
    );
}

Wasmtime GitHub notifications bot (Sep 04 2025 at 22:50):

fitzgen created PR review comment:

Instead of num_{rec_groups,types} can these be max_{rec_groups,types}? We already have an (implicit) count of rec groups and types via the size of the Types::{type_defs,rec_groups} fields. Then we would define a Types::fixup method that takes a reference to the limits and removes any entries beyond the given limit, and finally we would call Types::fixup from TableOps::fixup:
impl Types {
    // ...

    fn fixup(&mut self, limits: &TableOpsLimits) {
        while self.rec_groups.len() > limits.max_rec_groups {
            self.rec_groups.pop_last();
        }
        while self.type_defs.len() > limits.max_types {
            self.type_defs.pop_last();
        }
    }
}
This ensures that we always have a valid number of types and rec groups by the time we get to emitting a Wasm binary, which is similar to how we treat everything else (assume/assert it is valid in to_wasm_binary, enforce validity in fixup).

(Aside: eventually we will probably want custom serialization/deserialization with #[serde(serialize_with = "my_serialization_func")] for all of our container types so that we never try to allocate a huge Vec/BTreeSet/BTreeMap during deserialization of untrusted fuzzer data, leading to OOMs.)

Wasmtime GitHub notifications bot (Sep 04 2025 at 22:53):

khagankhan commented on PR #11605:

Thanks for the comments @fitzgen! Yes mutators are coming next. I just want to go step by step so addressing them is easy

Wasmtime GitHub notifications bot (Sep 15 2025 at 21:54):

khagankhan updated PR #11605.

Wasmtime GitHub notifications bot (Sep 15 2025 at 21:56):

khagankhan commented on PR #11605:

@fitzgen It is ready for the second round of reviews. There is a failing test of limit > 0 which is related to the #11587 that I am looking at.

Wasmtime GitHub notifications bot (Sep 16 2025 at 17:03):

fitzgen created PR review comment:

Style nitpick: newline between imports and declarations

Wasmtime GitHub notifications bot (Sep 16 2025 at 17:03):

fitzgen submitted PR review:

Looks good! Two tiny nitpicks below, but should be good to merge when they are addressed and the test issue you mentioned is resolved. Thanks!

Wasmtime GitHub notifications bot (Sep 16 2025 at 17:03):

fitzgen created PR review comment:

Nitpick: lets exhaustively match on the struct definition here so that as we expand that type we are reminded to update this code by the compiler:
                CompositeType::Struct(StructType {}) => wasm_encoder::SubType {

Wasmtime GitHub notifications bot (Sep 17 2025 at 17:42):

khagankhan updated PR #11605.

Wasmtime GitHub notifications bot (Sep 17 2025 at 18:02):

khagankhan updated PR #11605.

Wasmtime GitHub notifications bot (Sep 17 2025 at 18:06):

khagankhan commented on PR #11605:

@fitzgen ready for the review. It passes the tests now. The potential merge should fix #11587

Wasmtime GitHub notifications bot (Sep 17 2025 at 18:35):

khagankhan commented on PR #11605:

cc @eeide

Wasmtime GitHub notifications bot (Sep 17 2025 at 19:29):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Sep 17 2025 at 19:53):

fitzgen merged PR #11605.

Last updated: Dec 06 2025 at 07:03 UTC