Repository: bytecodealliance/wasm-tools (rust)

Issue:
components: additional validation needed in `wasmparser`
In addition to #619, the following needs to be validated for components in `wasmparser`:

* record field names should be unique.
* variant case names should be unique.
* enum names should be unique.
* flag names should be unique.
* variants must have at least one case.
* variant case refinements should validate that the index is less than the current case index.
* values must be "used" exactly once (right now we check for more than once, but don't require every value to be used).



Pull request description:
wasmparser: improve component validation.
This PR improves component validation by checking:

* Record field names, variant case names, enum tag names, and flag names should
  be unique per type definition.
* Variants must have at least one case.
* Variant case refinement index must be less than the current case index.
* Every value in the component must be used at exactly once.

Due to the last bullet point above, this commit also temporarily halts
generating value imports in `wasm-smith` because there currently is no way to
use them via instantiation, start function, or export.

Fixes #622.

Patch:
diff --git a/crates/wasm-smith/src/component.rs b/crates/wasm-smith/src/component.rs
index 712741a1..57058243 100644
--- a/crates/wasm-smith/src/component.rs
+++ b/crates/wasm-smith/src/component.rs
@@ -628,9 +628,11 @@ impl ComponentBuilder {
             });
         }
 
-        if for_type_def || !for_import || self.total_values < self.config.max_values() {
-            choices.push(|me, u| Ok(ComponentTypeRef::Value(me.arbitrary_component_val_type(u)?)));
-        }
+        // TODO: wasm-smith needs to ensure that every arbitrary value gets used exactly once.
+        //       until that time, don't import values
+        // if for_type_def || !for_import || self.total_values < self.config.max_values() {
+        //     choices.push(|me, u| Ok(ComponentTypeRef::Value(me.arbitrary_component_val_type(u)?)));
+        // }
 
         if !scope.func_types.is_empty()
             && (for_type_def
@@ -989,17 +991,19 @@ impl ComponentBuilder {
                 }
 
                 if me.current_type_scope().can_ref_type() && u.int_in_range::<u8>(0..=3)? == 0 {
-                    // Imports.
-                    let name = crate::unique_string(100, &mut imports, u)?;
-                    defs.push(ComponentTypeDef::Import(Import {
-                        name,
-                        ty: me.arbitrary_type_ref(u, true, true)?.unwrap(),
-                    }));
-                } else {
-                    // Type definitions, exports, and aliases.
-                    let def = me.arbitrary_instance_type_def(u, &mut exports, type_fuel)?;
-                    defs.push(def.into());
+                    if let Some(ty) = me.arbitrary_type_ref(u, true, true)? {
+                        // Imports.
+                        let name = crate::unique_string(100, &mut imports, u)?;
+                        defs.push(ComponentTypeDef::Import(Import { name, ty }));
+                        return Ok(true);
+                    }
+
+                    // Can't reference an arbitrary type, fallback to another definition.
                 }
+
+                // Type definitions, exports, and aliases.
+                let def = me.arbitrary_instance_type_def(u, &mut exports, type_fuel)?;
+                defs.push(def.into());
                 Ok(true)
             })
         })?;
@@ -1240,7 +1244,7 @@ impl ComponentBuilder {
     ) -> Result<VariantType> {
         let mut cases = vec![];
         let mut case_names = HashSet::new();
-        arbitrary_loop(u, 0, 100, |u| {
+        arbitrary_loop(u, 1, 100, |u| {
             *type_fuel = type_fuel.saturating_sub(1);
             if *type_fuel == 0 {
                 return Ok(false);
diff --git a/crates/wasmparser/src/validator.rs b/crates/wasmparser/src/validator.rs
index f965af3d..25aac748 100644
--- a/crates/wasmparser/src/validator.rs
+++ b/crates/wasmparser/src/validator.rs
@@ -1204,6 +1204,14 @@ impl Validator {
             State::Component => {
                 let mut component = self.components.pop().unwrap();
 
+                // Validate that all values were used for the component
+                if let Some(index) = component.values.iter().position(|(_, used)| !*used) {
+                    return Err(BinaryReaderError::new(
+                        format!("value index {index} was not used as part of an instantiation, start function, or export"),
+                        offset,
+                    ));
+                }
+
                 // If there's a parent component, pop the stack, add it to the parent,
                 // and continue to validate the component
                 if let Some(parent) = self.components.last_mut() {
diff --git a/crates/wasmparser/src/validator/component.rs b/crates/wasmparser/src/validator/component.rs
index 5027f29d..9f24f847 100644
--- a/crates/wasmparser/src/validator/component.rs
+++ b/crates/wasmparser/src/validator/component.rs
@@ -12,14 +12,14 @@ use super::{
 use crate::{
     limits::*,
     types::{
-        ComponentDefinedType, ComponentEntityType, InstanceTypeKind, TupleType, UnionType,
-        VariantType,
+        ComponentDefinedType, ComponentEntityType, InstanceTypeKind, LoweringInfo, TupleType,
+        UnionType, VariantType,
     },
     BinaryReaderError, CanonicalOption, ComponentExternalKind, ComponentOuterAliasKind,
     ComponentTypeRef, ExternalKind, FuncType, GlobalType, InstantiationArgKind, MemoryType,
     PrimitiveValType, Result, TableType, TypeBounds, ValType, WasmFeatures,
 };
-use indexmap::IndexMap;
+use indexmap::{IndexMap, IndexSet};
 use std::mem;
 
 pub(crate) struct ComponentState {
@@ -290,19 +290,27 @@ impl ComponentState {
 
         // Lifting a function is for an export, so match the expected canonical ABI
         // export signature
-        let (params, results, requires_memory) = ty.lower(types, false);
-        self.check_options(ty, Some(core_ty), requires_memory, &options, types, offset)?;
+        let info = ty.lower(types, false);
+        self.check_options(Some(core_ty), &info, &options, types, offset)?;
 
-        if core_ty.params.as_ref() != params.as_slice() {
+        if core_ty.params.as_ref() != info.params.as_slice() {
             return Err(BinaryReaderError::new(
-                format!("lowered parameter types `{:?}` do not match parameter types `{:?}` of core function {core_func_index}", params.as_slice(), core_ty.params),
+                format!(
+                    "lowered parameter types `{:?}` do not match parameter types `{:?}` of core function {core_func_index}",
+                    info.params.as_slice(),
+                    core_ty.params
+                ),
                 offset,
             ));
         }
 
-        if core_ty.returns.as_ref() != results.as_slice() {
+        if core_ty.returns.as_ref() != info.results.as_slice() {
             return Err(BinaryReaderError::new(
-                format!("lowered result types `{:?}` do not match result types `{:?}` of core function {core_func_index}", results.as_slice(), core_ty.returns),
+                format!(
+                    "lowered result types `{:?}` do not match result types `{:?}` of core function {core_func_index}",
+                    info.results.as_slice(),
+                    core_ty.returns
+                ),
                 offset,
             ));
         }
@@ -325,14 +333,11 @@ impl ComponentState {
 
         // Lowering a function is for an import, so use a function type that matches
         // the expected canonical ABI import signature.
-        let (params, results, requires_memory) = ty.lower(types, true);
+        let info = ty.lower(types, true);
 
-        self.check_options(ty, None, requires_memory, &options, types, offset)?;
+        self.check_options(None, &info, &options, types, offset)?;
 
-        let lowered_ty = Type::Func(FuncType {
-            params: params.as_slice().to_vec().into_boxed_slice(),
-            returns: results.as_slice().to_vec().into_boxed_slice(),
-        });
+        let lowered_ty = Type::Func(info.into_func_type());
 
         self.core_funcs.push(TypeId {
             type_size: lowered_ty.type_size(),
@@ -469,9 +474,8 @@ impl ComponentState {
 
     fn check_options(
         &self,
-        ty: &ComponentFuncType,
         core_ty: Option<&FuncType>,
-        requires_memory: bool,
+        info: &LoweringInfo,
         options: &[CanonicalOption],
         types: &TypeList,
         offset: usize,
@@ -581,15 +585,14 @@ impl ComponentState {
             }
         }
 
-        let requires_realloc = ty.requires_realloc(types);
-        if (requires_realloc || requires_memory) && memory.is_none() {
+        if info.requires_memory && memory.is_none() {
             return Err(BinaryReaderError::new(
                 "canonical option `memory` is required",
                 offset,
             ));
         }
 
-        if requires_realloc && realloc.is_none() {
+        if info.requires_realloc && realloc.is_none() {
             return Err(BinaryReaderError::new(
                 "canonical option `realloc` is required",
                 offset,
@@ -1620,19 +1623,24 @@ impl ComponentState {
         offset: usize,
     ) -> Result<ComponentDefinedType> {
         let mut type_size = 1;
-        let fields = fields
-            .iter()
-            .map(|(name, ty)| {
-                Self::check_name(name, "record field", offset)?;
-                let ty = self.create_component_val_type(*ty, types, offset)?;
-                type_size = combine_type_sizes(type_size, ty.type_size(), offset)?;
-                Ok((name.to_string(), ty))
-            })
-            .collect::<Result<_>>()?;
+        let mut field_map = IndexMap::with_capacity(fields.len());
+
+        for (name, ty) in fields {
+            Self::check_name(name, "record field", offset)?;
+            let ty = self.create_component_val_type(*ty, types, offset)?;
+            type_size = combine_type_sizes(type_size, ty.type_size(), offset)?;
+
+            if field_map.insert(name.to_string(), ty).is_some() {
+                return Err(BinaryReaderError::new(
+                    format!("duplicate field named `{}` in record type", name),
+                    offset,
+                ));
+            }
+        }
 
         Ok(ComponentDefinedType::Record(RecordType {
             type_size,
-            fields,
+            fields: field_map,
         }))
     }
 
@@ -1643,33 +1651,48 @@ impl ComponentState {
         offset: usize,
     ) -> Result<ComponentDefinedType> {
         let mut type_size = 1;
-        let cases = cases
-            .iter()
-            .map(|case| {
-                Self::check_name(case.name, "variant case", offset)?;
-                if let Some(refines) = case.refines {
-                    if refines >= cases.len() as u32 {
-                        return Err(BinaryReaderError::new(
-                            format!("variant case refines index {} is out of bounds", refines),
-                            offset,
-                        ));
-                    }
+        let mut case_map = IndexMap::with_capacity(cases.len());
+
+        if cases.is_empty() {
+            return Err(BinaryReaderError::new(
+                "variant type must have at least one case",
+                offset,
+            ));
+        }
+
+        for (i, case) in cases.iter().enumerate() {
+            Self::check_name(case.name, "variant case", offset)?;
+            if let Some(refines) = case.refines {
+                if refines >= i as u32 {
+                    return Err(BinaryReaderError::new(
+                        "variant case can only refine a previously defined case",
+                        offset,
+                    ));
                 }
-                let ty = self.create_component_val_type(case.ty, types, offset)?;
-                type_size = combine_type_sizes(type_size, ty.type_size(), offset)?;
-                Ok((
+            }
+            let ty = self.create_component_val_type(case.ty, types, offset)?;
+            type_size = combine_type_sizes(type_size, ty.type_size(), offset)?;
+
+            if case_map
+                .insert(
                     case.name.to_string(),
                     VariantCase {
                         ty,
                         refines: case.refines.map(|i| cases[i as usize].name.to_string()),
                     },
-                ))
-            })
-            .collect::<Result<_>>()?;
+                )
+                .is_some()
+            {
+                return Err(BinaryReaderError::new(
+                    format!("duplicate case named `{}` in variant type", case.name),
+                    offset,
+                ));
+            }
+        }
 
         Ok(ComponentDefinedType::Variant(VariantType {
             type_size,
-            cases,
+            cases: case_map,
         }))
     }
 
@@ -1693,15 +1716,19 @@ impl ComponentState {
     }
 
     fn create_flags_type(&self, names: &[&str], offset: usize) -> Result<ComponentDefinedType> {
-        Ok(ComponentDefinedType::Flags(
-            names
-                .iter()
-                .map(|name| {
-                    Self::check_name(name, "flag", offset)?;
-                    Ok(name.to_string())
-                })
-                .collect::<Result<_>>()?,
-        ))
+        let mut names_set = IndexSet::with_capacity(names.len());
+
+        for name in names {
+            Self::check_name(name, "flag", offset)?;
+            if !names_set.insert(name.to_string()) {
+                return Err(BinaryReaderError::new(
+                    format!("duplicate flag named `{}`", name),
+                    offset,
+                ));
+            }
+        }
+
+        Ok(ComponentDefinedType::Flags(names_set))
     }
 
     fn create_enum_type(&self, cases: &[&str], offset: usize) -> Result<ComponentDefinedType> {
@@ -1712,15 +1739,19 @@ impl ComponentState {
             ));
         }
 
-        Ok(ComponentDefinedType::Enum(
-            cases
-                .iter()
-                .map(|name| {
-                    Self::check_name(name, "enum tag", offset)?;
-                    Ok(name.to_string())
-                })
-                .collect::<Result<_>>()?,
-        ))
+        let mut tags = IndexSet::with_capacity(cases.len());
+
+        for tag in cases {
+            Self::check_name(tag, "enum tag", offset)?;
+            if !tags.insert(tag.to_string()) {
+                return Err(BinaryReaderError::new(
+                    format!("duplicate enum tag named `{}`", tag),
+                    offset,
+                ));
+            }
+        }
+
+        Ok(ComponentDefinedType::Enum(tags))
     }
 
     fn create_union_type(
diff --git a/crates/wasmparser/src/validator/types.rs b/crates/wasmparser/src/validator/types.rs
index 699815a0..cfc4f8f3 100644
--- a/crates/wasmparser/src/validator/types.rs
+++ b/crates/wasmparser/src/validator/types.rs
@@ -80,6 +80,34 @@ impl LoweredTypes {
     }
 }
 
+/// Represents information about a component function type lowering.
+pub(crate) struct LoweringInfo {
+    pub(crate) params: LoweredTypes,
+    pub(crate) results: LoweredTypes,
+    pub(crate) requires_memory: bool,
+    pub(crate) requires_realloc: bool,
+}
+
+impl LoweringInfo {
+    pub(crate) fn into_func_type(self) -> FuncType {
+        FuncType {
+            params: self.params.as_slice().to_vec().into_boxed_slice(),
+            returns: self.results.as_slice().to_vec().into_boxed_slice(),
+        }
+    }
+}
+
+impl Default for LoweringInfo {
+    fn default() -> Self {
+        Self {
+            params: LoweredTypes::new(MAX_FLAT_FUNC_PARAMS),
+            results: LoweredTypes::new(MAX_FLAT_FUNC_RESULTS),
+            requires_memory: false,
+            requires_realloc: false,
+        }
+    }
+}
+
 fn push_primitive_wasm_types(ty: &PrimitiveValType, lowered_types: &mut LoweredTypes) -> bool {
     match ty {
         PrimitiveValType::Unit => true,
@@ -625,11 +653,6 @@ pub struct ComponentFuncType {
 }
 
 impl ComponentFuncType {
-    pub(crate) fn requires_realloc(&self, types: &TypeList) -> bool {
-        self.result.requires_realloc(types)
-            || self.params.iter().any(|(_, ty)| ty.requires_realloc(types))
-    }
-
     pub(crate) fn is_subtype_of(&self, other: &Self, types: &TypeList) -> bool {
         // Subtyping rules:
         // https://github.com/WebAssembly/component-model/blob/17f94ed1270a98218e0e796ca1dad1feb7e5c507/design/mvp/Subtyping.md
@@ -666,41 +689,50 @@ impl ComponentFuncType {
 
     /// Lowers the component function type to core parameter and result types for the
     /// canonical ABI.
-    pub(crate) fn lower(
-        &self,
-        types: &TypeList,
-        import: bool,
-    ) -> (LoweredTypes, LoweredTypes, bool) {
-        let mut params = LoweredTypes::new(MAX_FLAT_FUNC_PARAMS);
-        let mut results = LoweredTypes::new(MAX_FLAT_FUNC_RESULTS);
-        let mut requires_memory = false;
+    pub(crate) fn lower(&self, types: &TypeList, import: bool) -> LoweringInfo {
+        let mut info = LoweringInfo::default();
 
         for (_, ty) in self.params.iter() {
-            if !ty.push_wasm_types(types, &mut params) {
+            // When `import` is false, it means we're lifting a core function,
+            // check if the parameters needs realloc
+            if !import {
+                info.requires_realloc = info.requires_realloc || ty.requires_realloc(types);
+            }
+
+            if !ty.push_wasm_types(types, &mut info.params) {
                 // Too many parameters to pass directly
                 // Function will have a single pointer parameter to pass the arguments
                 // via linear memory
-                params.clear();
-                assert!(params.push(ValType::I32));
-                requires_memory = true;
+                info.params.clear();
+                assert!(info.params.push(ValType::I32));
+                info.requires_memory = true;
                 break;
             }
         }
 
-        if !self.result.push_wasm_types(types, &mut results) {
+        // When `import` is true, it means we're lowering a component function,
+        // check if the result needs realloc
+        if import {
+            info.requires_realloc = info.requires_realloc || self.result.requires_realloc(types);
+        }
+
+        if !self.result.push_wasm_types(types, &mut info.results) {
             // Too many results to return directly, either a retptr parameter will be used (import)
             // or a single pointer will be returned (export)
-            results.clear();
+            info.results.clear();
             if import {
-                params.max = MAX_LOWERED_TYPES;
-                assert!(params.push(ValType::I32));
+                info.params.max = MAX_LOWERED_TYPES;
+                assert!(info.params.push(ValType::I32));
             } else {
-                assert!(results.push(ValType::I32));
+                assert!(info.results.push(ValType::I32));
             }
-            requires_memory = true;
+            info.requires_memory = true;
         }
 
-        (params, results, requires_memory)
+        // Memory is always required when realloc is required
+        info.requires_memory |= info.requires_realloc;
+
+        info
     }
 }
 


Test patch:
diff --git a/tests/local/component-model/a.wast b/tests/local/component-model/a.wast
index 0c2126fd..6246ec21 100644
--- a/tests/local/component-model/a.wast
+++ b/tests/local/component-model/a.wast
@@ -4,7 +4,7 @@
 (component
   (import "one" (func))
 
-  (import "two" (value string))
+  (import "two" (value $v string))
   (import "three" (instance
     (export "four" (instance
       (export "five" (core module
@@ -13,5 +13,7 @@
       ))
     ))
   ))
+
+  (export "one" (value $v))
   ;; ...
 )
diff --git a/tests/local/component-model/definedtypes.wast b/tests/local/component-model/definedtypes.wast
index 91fe642b..0ce5bb2d 100644
--- a/tests/local/component-model/definedtypes.wast
+++ b/tests/local/component-model/definedtypes.wast
@@ -18,11 +18,10 @@
   (type $A14b (record (field "x" unit)))
   (type $A14c (record (field "x" $A0)))
 
-  (type $A15a (variant))
-  (type $A15b (variant (case "x" unit)))
-  (type $A15c (variant (case "x" $A1)))
-  (type $A15e (variant (case $x "x" unit) (case $y "y" string (refines $x)) (case "z" string (refines $y))))
-  (type $A15f (variant (case "x" unit) (case "y" string (refines 0)) (case "z" string (refines 1))))
+  (type $A15a (variant (case "x" unit)))
+  (type $A15b (variant (case "x" $A1)))
+  (type $A15c (variant (case $x "x" unit) (case $y "y" string (refines $x)) (case "z" string (refines $y))))
+  (type $A15d (variant (case "x" unit) (case "y" string (refines 0)) (case "z" string (refines 1))))
 
   (type $A16a (list unit))
   (type $A16b (list $A3))
@@ -77,7 +76,15 @@
     (type $t string)
     (type $v (variant (case "x" $t (refines 1))))
   )
-  "variant case refines index 1 is out of bounds"
+  "variant case can only refine a previously defined case"
+)
+
+(assert_invalid
+  (component
+    (type $t string)
+    (type $v (variant (case "x" $t) (case "y" u64 (refines 2)) (case "z" string)))
+  )
+  "variant case can only refine a previously defined case"
 )
 
 (assert_invalid
@@ -131,18 +138,32 @@
   (component (type (tuple 0)))
   "index out of bounds")
 
+(assert_invalid
+  (component (type (record (field "x" string) (field "x" u8))))
+  "duplicate field named `x` in record type")
+(assert_invalid
+  (component (type (variant (case "x" s64) (case "x" s64))))
+  "duplicate case named `x` in variant type")
+(assert_invalid
+  (component (type (flags "x" "y" "x")))
+  "duplicate flag named `x`")
+(assert_invalid
+  (component (type (enum "x" "y" "x")))
+  "duplicate enum tag named `x`")
+
 (assert_invalid
   (component (type (record (field "" s32))))
   "name cannot be empty")
-
 (assert_invalid
   (component (type (variant (case "" s32))))
   "name cannot be empty")
-
 (assert_invalid
   (component (type (flags "")))
   "name cannot be empty")
-
 (assert_invalid
   (component (type (enum "")))
   "name cannot be empty")
+
+(assert_invalid
+  (component (type (variant)))
+  "variant type must have at least one case")
diff --git a/tests/local/component-model/func.wast b/tests/local/component-model/func.wast
index a000d24d..440efbdb 100644
--- a/tests/local/component-model/func.wast
+++ b/tests/local/component-model/func.wast
@@ -17,3 +17,51 @@
   )
   "canonical option `memory` is required"
 )
+
+(component
+  (import "" (func $log (param string)))
+  (core module $libc
+    (memory (export "memory") 1)
+  )
+  (core instance $libc (instantiate $libc))
+  (core func (canon lower (func $log) (memory (core memory $libc "memory"))))
+)
+
+(component
+  (core module $m
+    (memory (export "memory") 1)
+    (func (export "ret-list") (result i32) unreachable)
+  )
+  (core instance $i (instantiate $m))
+
+  (func (export "ret-list") (result (list u8))
+    (canon lift (core func $i "ret-list") (memory (core memory $i "memory")))
+  )
+)
+
+(assert_invalid
+  (component
+    (import "" (func $log (result string)))
+    (core module $libc
+      (memory (export "memory") 1)
+    )
+    (core instance $libc (instantiate $libc))
+    (core func (canon lower (func $log) (memory (core memory $libc "memory"))))
+  )
+  "canonical option `realloc` is required"
+)
+
+(assert_invalid
+  (component
+    (core module $m
+      (memory (export "memory") 1)
+      (func (export "param-list") (param i32 i32) unreachable)
+    )
+    (core instance $i (instantiate $m))
+
+    (func (export "param-list") (param (list u8))
+      (canon lift (core func $i "param-list") (memory (core memory $i "memory")))
+    )
+  )
+  "canonical option `realloc` is required"
+)
diff --git a/tests/local/component-model/import.wast b/tests/local/component-model/import.wast
index 6698a8dc..139ee540 100644
--- a/tests/local/component-model/import.wast
+++ b/tests/local/component-model/import.wast
@@ -96,6 +96,13 @@
   )
   "type index out of bounds")
 
+(assert_invalid
+  (component
+    (import "" (value string))
+  )
+  "value index 0 was not used as part of an instantiation, start function, or export")
+
 (component
   (import "" (value string))
+  (export "" (value 0))
 )