Repository: bytecodealliance/wasm-tools (rust)

Issue:
components: Indirect returns don't validate that a canonical `memory` option is present
Currently this validates:

```wasm
(component
    (module $m
        (memory (export "memory") 1)
        (func (export "foo") (result i32) unreachable)
    )
    (instance $i (instantiate (module $m)))

    (func (export "tuple")
        (canon.lift
            (func (result (tuple s8 u8)))
            (func $i "foo")
        )
    )
)
```

but it shouldn't because an `(into $i)` directive is missing to indicate which memory the result is being loaded from.

Pull request description:
wasmparser: validate `memory` option is present.
This PR updates the component validation in `wasmparser` to check if the
`memory` option is present when the lowered signature of a canonical function
would need indirect memory access (e.g. the parameters or results exceeds the
canonical ABI maximums and are passed indirectly through memory).

Fixes #619.

Patch:
diff --git a/crates/wasmparser/src/validator/component.rs b/crates/wasmparser/src/validator/component.rs
index 68c855dc..868df0b3 100644
--- a/crates/wasmparser/src/validator/component.rs
+++ b/crates/wasmparser/src/validator/component.rs
@@ -290,7 +290,8 @@ impl ComponentState {
 
         // Lifting a function is for an export, so match the expected canonical ABI
         // export signature
-        let (params, results) = ty.lower(types, false);
+        let (params, results, requires_memory) = ty.lower(types, false);
+        self.check_options(ty, Some(core_ty), requires_memory, &options, types, offset)?;
 
         if core_ty.params.as_ref() != params.as_slice() {
             return Err(BinaryReaderError::new(
@@ -306,7 +307,6 @@ impl ComponentState {
             ));
         }
 
-        self.check_options(ty, Some(core_ty), &options, types, offset)?;
         self.funcs.push(self.types[type_index as usize]);
 
         Ok(())
@@ -323,11 +323,11 @@ impl ComponentState {
             .as_component_func_type()
             .unwrap();
 
-        self.check_options(ty, None, &options, types, offset)?;
-
         // Lowering a function is for an import, so use a function type that matches
         // the expected canonical ABI import signature.
-        let (params, results) = ty.lower(types, true);
+        let (params, results, requires_memory) = ty.lower(types, true);
+
+        self.check_options(ty, None, requires_memory, &options, types, offset)?;
 
         let lowered_ty = Type::Func(FuncType {
             params: params.as_slice().to_vec().into_boxed_slice(),
@@ -471,6 +471,7 @@ impl ComponentState {
         &self,
         ty: &ComponentFuncType,
         core_ty: Option<&FuncType>,
+        requires_memory: bool,
         options: &[CanonicalOption],
         types: &TypeList,
         offset: usize,
@@ -580,20 +581,19 @@ impl ComponentState {
             }
         }
 
-        if ty.requires_realloc(types) {
-            if memory.is_none() {
-                return Err(BinaryReaderError::new(
-                    "canonical option `memory` is required",
-                    offset,
-                ));
-            }
+        let requires_realloc = ty.requires_realloc(types);
+        if (requires_realloc || requires_memory) && memory.is_none() {
+            return Err(BinaryReaderError::new(
+                "canonical option `memory` is required",
+                offset,
+            ));
+        }
 
-            if realloc.is_none() {
-                return Err(BinaryReaderError::new(
-                    "canonical option `realloc` is required",
-                    offset,
-                ));
-            }
+        if requires_realloc && realloc.is_none() {
+            return Err(BinaryReaderError::new(
+                "canonical option `realloc` is required",
+                offset,
+            ));
         }
 
         Ok(())
diff --git a/crates/wasmparser/src/validator/types.rs b/crates/wasmparser/src/validator/types.rs
index 14107a3d..699815a0 100644
--- a/crates/wasmparser/src/validator/types.rs
+++ b/crates/wasmparser/src/validator/types.rs
@@ -666,9 +666,14 @@ 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) {
+    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;
 
         for (_, ty) in self.params.iter() {
             if !ty.push_wasm_types(types, &mut params) {
@@ -677,6 +682,7 @@ impl ComponentFuncType {
                 // via linear memory
                 params.clear();
                 assert!(params.push(ValType::I32));
+                requires_memory = true;
                 break;
             }
         }
@@ -691,9 +697,10 @@ impl ComponentFuncType {
             } else {
                 assert!(results.push(ValType::I32));
             }
+            requires_memory = true;
         }
 
-        (params, results)
+        (params, results, requires_memory)
     }
 }
 


Test patch:
diff --git a/tests/local/component-model/func.wast b/tests/local/component-model/func.wast
index 5745940e..a000d24d 100644
--- a/tests/local/component-model/func.wast
+++ b/tests/local/component-model/func.wast
@@ -2,3 +2,18 @@
   (import "" (func (param "foo" string)))
   (import "a" (func (param "foo" string) (param s32) (param "bar" u32)))
 )
+
+(assert_invalid
+  (component
+      (core module $m
+          (memory (export "memory") 1)
+          (func (export "foo") (result i32) unreachable)
+      )
+      (core instance $i (instantiate $m))
+
+      (func (export "tuple") (result (tuple s8 u8))
+          (canon lift (core func $i "foo"))
+      )
+  )
+  "canonical option `memory` is required"
+)