Skip over unsupported instructions instead of panicking

Fixes rustwasm#2969

This changes `wasm-bindgen-wasm-interpreter` to ignore a function rather than panicking if it contains an unsupported instruction. This works around some runtime glue that gets added to our descriptor functions on the WASI target by more-or-less ignoring them.

I also put some work into making sure a helpful error message is given if the actual describe function contains unsupported instructions, by keeping track of the instructions that caused us to skip and including them in the error message if the descriptor is invalid.

crates/cli-support/Cargo.toml

@@ -28,3 +28,6 @@ wasm-bindgen-wasm-interpreter = { path = "../wasm-interpreter", version = '=0.2.
 wit-text = "0.8.0"
 wit-walrus = "0.6.0"
 wit-validator = "0.2.0"
+
+[dev-dependencies]
+wat = "1.0"

crates/cli-support/src/descriptor.rs

@@ -108,14 +108,17 @@ pub enum VectorKind {
 }
 
 impl Descriptor {
-    pub fn decode(mut data: &[u32]) -> Descriptor {
-        let descriptor = Descriptor::_decode(&mut data, false);
-        assert!(data.is_empty(), "remaining data {:?}", data);
-        descriptor
+    /// Decodes `data` into a descriptor, or returns `None` if it's invalid.
+    pub fn decode(mut data: &[u32]) -> Option<Descriptor> {
+        let descriptor = Descriptor::_decode(&mut data, false)?;
+        if !data.is_empty() {
+            return None;
+        }
+        Some(descriptor)
     }
 
-    fn _decode(data: &mut &[u32], clamped: bool) -> Descriptor {
-        match get(data) {
+    fn _decode(data: &mut &[u32], clamped: bool) -> Option<Descriptor> {
+        Some(match get(data)? {
             I8 => Descriptor::I8,
             I16 => Descriptor::I16,
             I32 => Descriptor::I32,
@@ -128,31 +131,31 @@ impl Descriptor {
             F32 => Descriptor::F32,
             F64 => Descriptor::F64,
             BOOLEAN => Descriptor::Boolean,
-            FUNCTION => Descriptor::Function(Box::new(Function::decode(data))),
-            CLOSURE => Descriptor::Closure(Box::new(Closure::decode(data))),
-            REF => Descriptor::Ref(Box::new(Descriptor::_decode(data, clamped))),
-            REFMUT => Descriptor::RefMut(Box::new(Descriptor::_decode(data, clamped))),
-            SLICE => Descriptor::Slice(Box::new(Descriptor::_decode(data, clamped))),
-            VECTOR => Descriptor::Vector(Box::new(Descriptor::_decode(data, clamped))),
-            OPTIONAL => Descriptor::Option(Box::new(Descriptor::_decode(data, clamped))),
-            RESULT => Descriptor::Result(Box::new(Descriptor::_decode(data, clamped))),
+            FUNCTION => Descriptor::Function(Box::new(Function::decode(data)?)),
+            CLOSURE => Descriptor::Closure(Box::new(Closure::decode(data)?)),
+            REF => Descriptor::Ref(Box::new(Descriptor::_decode(data, clamped)?)),
+            REFMUT => Descriptor::RefMut(Box::new(Descriptor::_decode(data, clamped)?)),
+            SLICE => Descriptor::Slice(Box::new(Descriptor::_decode(data, clamped)?)),
+            VECTOR => Descriptor::Vector(Box::new(Descriptor::_decode(data, clamped)?)),
+            OPTIONAL => Descriptor::Option(Box::new(Descriptor::_decode(data, clamped)?)),
+            RESULT => Descriptor::Result(Box::new(Descriptor::_decode(data, clamped)?)),
             CACHED_STRING => Descriptor::CachedString,
             STRING => Descriptor::String,
             EXTERNREF => Descriptor::Externref,
-            ENUM => Descriptor::Enum { hole: get(data) },
+            ENUM => Descriptor::Enum { hole: get(data)? },
             RUST_STRUCT => {
-                let name = get_string(data);
+                let name = get_string(data)?;
                 Descriptor::RustStruct(name)
             }
             NAMED_EXTERNREF => {
-                let name = get_string(data);
+                let name = get_string(data)?;
                 Descriptor::NamedExternref(name)
             }
             CHAR => Descriptor::Char,
             UNIT => Descriptor::Unit,
-            CLAMPED => Descriptor::_decode(data, true),
-            other => panic!("unknown descriptor: {}", other),
-        }
+            CLAMPED => Descriptor::_decode(data, true)?,
+            _ => return None,
+        })
     }
 
     pub fn unwrap_function(self) -> Function {
@@ -204,45 +207,48 @@ impl Descriptor {
     }
 }
 
-fn get(a: &mut &[u32]) -> u32 {
-    let ret = a[0];
+fn get(a: &mut &[u32]) -> Option<u32> {
+    let ret = *a.get(0)?;
     *a = &a[1..];
-    ret
+    Some(ret)
 }
 
-fn get_string(data: &mut &[u32]) -> String {
-    (0..get(data))
-        .map(|_| char::from_u32(get(data)).unwrap())
+fn get_string(data: &mut &[u32]) -> Option<String> {
+    (0..get(data)?)
+        .map(|_| char::from_u32(get(data)?))
         .collect()
 }
 
 impl Closure {
-    fn decode(data: &mut &[u32]) -> Closure {
-        let shim_idx = get(data);
-        let dtor_idx = get(data);
-        let mutable = get(data) == REFMUT;
-        assert_eq!(get(data), FUNCTION);
-        Closure {
+    fn decode(data: &mut &[u32]) -> Option<Closure> {
+        let shim_idx = get(data)?;
+        let dtor_idx = get(data)?;
+        let mutable = get(data)? == REFMUT;
+        if get(data)? != FUNCTION {
+            return None;
+        }
+
+        Some(Closure {
             shim_idx,
             dtor_idx,
             mutable,
-            function: Function::decode(data),
-        }
+            function: Function::decode(data)?,
+        })
     }
 }
 
 impl Function {
-    fn decode(data: &mut &[u32]) -> Function {
-        let shim_idx = get(data);
-        let arguments = (0..get(data))
+    fn decode(data: &mut &[u32]) -> Option<Function> {
+        let shim_idx = get(data)?;
+        let arguments = (0..get(data)?)
             .map(|_| Descriptor::_decode(data, false))
-            .collect::<Vec<_>>();
-        Function {
+            .collect::<Option<Vec<_>>>()?;
+        Some(Function {
             arguments,
             shim_idx,
-            ret: Descriptor::_decode(data, false),
-            inner_ret: Some(Descriptor::_decode(data, false)),
-        }
+            ret: Descriptor::_decode(data, false)?,
+            inner_ret: Some(Descriptor::_decode(data, false)?),
+        })
     }
 }
 

crates/cli-support/src/descriptors.rs

@@ -14,9 +14,10 @@ use crate::descriptor::{Closure, Descriptor};
 use anyhow::Error;
 use std::borrow::Cow;
 use std::collections::{HashMap, HashSet};
+use std::fmt::Write;
 use walrus::ImportId;
 use walrus::{CustomSection, FunctionId, Module, TypedCustomSectionId};
-use wasm_bindgen_wasm_interpreter::Interpreter;
+use wasm_bindgen_wasm_interpreter::{Interpreter, Skip};
 
 #[derive(Default, Debug)]
 pub struct WasmBindgenDescriptorsSection {
@@ -40,6 +41,31 @@ pub fn execute(module: &mut Module) -> Result<WasmBindgenDescriptorsSectionId, E
     Ok(module.customs.add(section))
 }
 
+/// Returns an error for an invalid descriptor.
+///
+/// `skipped` is the list of functions that were skipped while retrieving the
+/// descriptor from the wasm module, which are likely the cause of the error if
+/// present.
+fn descriptor_error(descriptor: &[u32], skipped: &[Skip]) -> anyhow::Error {
+    let mut msg = format!("invalid descriptor: {descriptor:?}");
+
+    if !skipped.is_empty() {
+        writeln!(
+            msg,
+            "\n\n\
+            Some functions containing unsupported instructions were skipped while\n\
+            intepreting the wasm module, which may have caused this:"
+        )
+        .unwrap();
+
+        for skip in skipped {
+            writeln!(msg, "    {skip}").unwrap();
+        }
+    }
+
+    anyhow::Error::msg(msg)
+}
+
 impl WasmBindgenDescriptorsSection {
     fn execute_exports(
         &mut self,
@@ -56,9 +82,10 @@ impl WasmBindgenDescriptorsSection {
                 walrus::ExportItem::Function(id) => id,
                 _ => panic!("{} export not a function", export.name),
             };
-            if let Some(d) = interpreter.interpret_descriptor(id, module) {
+            if let Some((d, skipped)) = interpreter.interpret_descriptor(id, module) {
                 let name = &export.name[prefix.len()..];
-                let descriptor = Descriptor::decode(d);
+                let descriptor =
+                    Descriptor::decode(d).ok_or_else(|| descriptor_error(d, &skipped))?;
                 self.descriptors.insert(name.to_string(), descriptor);
             }
             to_remove.push(export.id());
@@ -96,10 +123,14 @@ impl WasmBindgenDescriptorsSection {
             };
             dfs_in_order(&mut find, local, local.entry_block());
             if find.found {
-                let descriptor = interpreter
+                let (descriptor, skipped) = interpreter
                     .interpret_closure_descriptor(id, module, &mut element_removal_list)
                     .unwrap();
-                func_to_descriptor.insert(id, Descriptor::decode(descriptor));
+                func_to_descriptor.insert(
+                    id,
+                    Descriptor::decode(descriptor)
+                        .ok_or_else(|| descriptor_error(descriptor, &skipped))?,
+                );
             }
         }
 
@@ -179,3 +210,47 @@ impl CustomSection for WasmBindgenDescriptorsSection {
         panic!("shouldn't emit custom sections just yet");
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::WasmBindgenDescriptorsSectionId;
+
+    fn execute(wat: &str) -> anyhow::Result<WasmBindgenDescriptorsSectionId> {
+        let wasm = wat::parse_str(wat).unwrap();
+        let mut module = walrus::Module::from_buffer(&wasm).unwrap();
+        super::execute(&mut module)
+    }
+
+    #[test]
+    fn unsupported_instruction() {
+        let result = execute(
+            r#"
+            (module
+                (import "__wbindgen_placeholder__" "__wbindgen_describe"
+                (func $__wbindgen_describe (param i32)))
+
+                (func $bar
+                    ;; We don't support `block`, so this will cause an error.
+                    block
+                    end
+
+                    ;; Which means this descriptor won't go through.
+                    i32.const 0
+                    call $__wbindgen_describe
+                )
+
+                (func $foo
+                    ;; Call into a nested function `bar`, since an unsupported instruction in the
+                    ;; root function we're calling panics immediately.
+                    call $bar
+                )
+
+                (export "__wbindgen_describe_foo" (func $foo))
+            )
+            "#,
+        );
+        let err = result.unwrap_err();
+        assert!(err.to_string().contains("invalid descriptor: []"));
+        assert!(err.to_string().contains("Block"));
+    }
+}