Repository: bytecodealliance/wasmtime
Pull request #8434 (2024-04-22 16:41:50)

Fix a panic calling host functions with refs in async mode
This commit fixes a panic when a host function defined with `Func::new` returned GC references and was called in async mode. The logic to auto-gc before the return values go to wasm asserted that a synchronous GC was possible but the context this function is called in could be either async or sync. The fix applied in this commit is to remove the auto-gc. This means that hosts will need to explicitly GC in these situations until auto-gc is re-added back to Wasmtime.

cc #8433 as this will make the behavior consistent, but we'll want to re-add the gc behavior.

<!--
Please make sure you include the following information:

- If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

- Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->


alexcrichton: I'll also note that I plan on backporting this to the 20.0.0 release to happen today too.

Patch:
diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 37086e8af..3ffec3232 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -464,6 +464,7 @@ jobs:
       working-directory: ./fuzz
     - run: cargo fuzz build --dev -s none
     - run: cargo fuzz build --dev -s none --fuzz-dir ./cranelift/isle/fuzz
+    - run: cargo fuzz build --dev -s none --fuzz-dir ./crates/environ/fuzz --features component-model
 
     # Verify the "min platform" example still works.
     #
diff --git a/RELEASES.md b/RELEASES.md
index 5b232971f..2ab587864 100644
--- a/RELEASES.md
+++ b/RELEASES.md
@@ -12,7 +12,7 @@ Unreleased.
 
 ## 20.0.0
 
-Unreleased.
+Released 2024-04-22
 
 ### Added
 
diff --git a/crates/environ/fuzz/Cargo.toml b/crates/environ/fuzz/Cargo.toml
index d4a68fee4..97ff78986 100644
--- a/crates/environ/fuzz/Cargo.toml
+++ b/crates/environ/fuzz/Cargo.toml
@@ -26,4 +26,8 @@ doc = false
 required-features = ["component-model"]
 
 [features]
-component-model = ["wasmtime-environ/component-model", "dep:component-fuzz-util"]
+component-model = [
+  "wasmtime-environ/component-model",
+  "wasmtime-environ/compile",
+  "dep:component-fuzz-util",
+]
diff --git a/crates/environ/fuzz/fuzz_targets/fact-valid-module.rs b/crates/environ/fuzz/fuzz_targets/fact-valid-module.rs
index d6b323285..cb7d85e02 100644
--- a/crates/environ/fuzz/fuzz_targets/fact-valid-module.rs
+++ b/crates/environ/fuzz/fuzz_targets/fact-valid-module.rs
@@ -9,35 +9,79 @@
 
 #![no_main]
 
-use arbitrary::Arbitrary;
-use component_fuzz_util::TestCase;
+use arbitrary::Unstructured;
+use component_fuzz_util::{TestCase, Type, MAX_TYPE_DEPTH};
 use libfuzzer_sys::fuzz_target;
+use wasmparser::types::ComponentAnyTypeId;
 use wasmparser::{Parser, Payload, Validator, WasmFeatures};
 use wasmtime_environ::component::*;
 use wasmtime_environ::fact::Module;
 
-#[derive(Arbitrary, Debug)]
-struct GenAdapterModule {
+const TYPE_COUNT: usize = 50;
+const MAX_ARITY: u32 = 5;
+const TEST_CASE_COUNT: usize = 20;
+
+#[derive(Debug)]
+struct GenAdapterModule<'a> {
     debug: bool,
-    adapters: Vec<GenAdapter>,
+    adapters: Vec<GenAdapter<'a>>,
 }
 
-#[derive(Arbitrary, Debug)]
-struct GenAdapter {
+#[derive(Debug)]
+struct GenAdapter<'a> {
     post_return: bool,
     lift_memory64: bool,
     lower_memory64: bool,
-    test: TestCase,
+    test: TestCase<'a>,
 }
 
-fuzz_target!(|module: GenAdapterModule| {
-    target(module);
+fuzz_target!(|data: &[u8]| {
+    let _ = target(data);
 });
 
-fn target(module: GenAdapterModule) {
+fn target(data: &[u8]) -> arbitrary::Result<()> {
     drop(env_logger::try_init());
 
-    let mut types = ComponentTypesBuilder::default();
+    let mut u = Unstructured::new(data);
+
+    // First generate a set of type to select from.
+    let mut type_fuel = 1000;
+    let mut types = Vec::new();
+    for _ in 0..u.int_in_range(1..=TYPE_COUNT)? {
+        // Only discount fuel if the generation was successful,
+        // otherwise we'll get more random data and try again.
+        types.push(Type::generate(&mut u, MAX_TYPE_DEPTH, &mut type_fuel)?);
+    }
+
+    // Next generate a set of static API test cases driven by the above
+    // types.
+    let mut ret = GenAdapterModule {
+        debug: u.arbitrary()?,
+        adapters: Vec::new(),
+    };
+    for _ in 0..u.int_in_range(1..=TEST_CASE_COUNT)? {
+        let mut params = Vec::new();
+        let mut results = Vec::new();
+        for _ in 0..u.int_in_range(0..=MAX_ARITY)? {
+            params.push(u.choose(&types)?);
+        }
+        for _ in 0..u.int_in_range(0..=MAX_ARITY)? {
+            results.push(u.choose(&types)?);
+        }
+
+        let test = TestCase {
+            params,
+            results,
+            encoding1: u.arbitrary()?,
+            encoding2: u.arbitrary()?,
+        };
+        ret.adapters.push(GenAdapter {
+            test,
+            post_return: u.arbitrary()?,
+            lift_memory64: u.arbitrary()?,
+            lower_memory64: u.arbitrary()?,
+        });
+    }
 
     // Manufactures a unique `CoreDef` so all function imports get unique
     // function imports.
@@ -72,9 +116,11 @@ fn target(module: GenAdapterModule) {
             item: ExportItem::Name(String::new()),
         }
     };
+    let mut validator = Validator::new();
+    let mut types = ComponentTypesBuilder::new(&validator);
 
     let mut adapters = Vec::new();
-    for adapter in module.adapters.iter() {
+    for adapter in ret.adapters.iter() {
         let wat_decls = adapter.test.declarations();
         let wat = format!(
             "(component
@@ -87,9 +133,7 @@ fn target(module: GenAdapterModule) {
         );
         let wasm = wat::parse_str(&wat).unwrap();
 
-        let mut validator = Validator::new();
-
-        types.push_type_scope();
+        let mut type_index = 0;
         for payload in Parser::new(0).parse_all(&wasm) {
             let payload = payload.unwrap();
             validator.payload(&payload).unwrap();
@@ -97,13 +141,17 @@ fn target(module: GenAdapterModule) {
                 Payload::ComponentTypeSection(s) => s,
                 _ => continue,
             };
-            for ty in section {
-                let ty = types.intern_component_type(&ty.unwrap()).unwrap();
-                types.push_component_typedef(ty);
-                let ty = match ty {
-                    TypeDef::ComponentFunc(ty) => ty,
+            for _ in section {
+                let validator_types = validator.types(0).unwrap();
+                let id = validator_types.component_any_type_at(type_index);
+                type_index += 1;
+                let id = match id {
+                    ComponentAnyTypeId::Func(id) => id,
                     _ => continue,
                 };
+                let ty = types
+                    .convert_component_func_type(validator_types, id)
+                    .unwrap();
                 adapters.push(Adapter {
                     lift_ty: ty,
                     lower_ty: ty,
@@ -137,10 +185,10 @@ fn target(module: GenAdapterModule) {
                 });
             }
         }
-        types.pop_type_scope();
+        validator.reset();
     }
 
-    let mut fact_module = Module::new(&types, module.debug);
+    let mut fact_module = Module::new(&types, ret.debug);
     for (i, adapter) in adapters.iter().enumerate() {
         fact_module.adapt(&format!("adapter{i}"), adapter);
     }
@@ -149,11 +197,11 @@ fn target(module: GenAdapterModule) {
         .validate_all(&wasm);
 
     let err = match result {
-        Ok(_) => return,
+        Ok(_) => return Ok(()),
         Err(e) => e,
     };
     eprintln!("invalid wasm module: {err:?}");
-    for adapter in module.adapters.iter() {
+    for adapter in ret.adapters.iter() {
         eprintln!("adapter: {adapter:?}");
     }
     std::fs::write("invalid.wasm", &wasm).unwrap();
diff --git a/crates/wasmtime/src/runtime/func.rs b/crates/wasmtime/src/runtime/func.rs
index 17dfece9b..c0d5de6b1 100644
--- a/crates/wasmtime/src/runtime/func.rs
+++ b/crates/wasmtime/src/runtime/func.rs
@@ -1346,24 +1346,6 @@ impl Func {
         let (params, results) = val_vec.split_at_mut(nparams);
         func(caller.sub_caller(), params, results)?;
 
-        #[cfg(feature = "gc")]
-        {
-            // See the comment in `Func::call_impl_check_args`.
-            let num_gc_refs = ty.as_wasm_func_type().non_i31_gc_ref_returns_count();
-            if let Some(num_gc_refs) = NonZeroUsize::new(num_gc_refs) {
-                if caller
-                    .as_context()
-                    .0
-                    .gc_store()?
-                    .gc_heap
-                    .need_gc_before_entering_wasm(num_gc_refs)
-                {
-                    assert!(!caller.as_context().0.async_support());
-                    caller.as_context_mut().gc();
-                }
-            }
-        }
-
         // Unlike our arguments we need to dynamically check that the return
         // values produced are correct. There could be a bug in `func` that
         // produces the wrong number, wrong types, or wrong stores of


Test patch:
diff --git a/tests/all/async_functions.rs b/tests/all/async_functions.rs
index 5e1fbf17d..2eaca31f6 100644
--- a/tests/all/async_functions.rs
+++ b/tests/all/async_functions.rs
@@ -983,3 +983,65 @@ async fn gc_preserves_externref_on_historical_async_stacks() -> Result<()> {
 
     Ok(())
 }
+
+#[tokio::test]
+async fn async_gc_with_func_new_and_func_wrap() -> Result<()> {
+    let _ = env_logger::try_init();
+
+    let mut config = Config::new();
+    config.async_support(true);
+    let engine = Engine::new(&config)?;
+
+    let module = Module::new(
+        &engine,
+        r#"
+            (module $m1
+                (import "" "a" (func $a (result externref)))
+                (import "" "b" (func $b (result externref)))
+
+                (table 2 funcref)
+                (elem (i32.const 0) func $a $b)
+
+                (func (export "a")
+                    (call $call (i32.const 0))
+                )
+                (func (export "b")
+                    (call $call (i32.const 1))
+                )
+
+                (func $call (param i32)
+                    (local $cnt i32)
+                    (loop $l
+                        (drop (call_indirect (result externref) (local.get 0)))
+                        (local.set $cnt (i32.add (local.get $cnt) (i32.const 1)))
+
+                        (if (i32.lt_u (local.get $cnt) (i32.const 1000))
+                         (then (br $l)))
+                    )
+                )
+            )
+        "#,
+    )?;
+
+    let mut linker = Linker::new(&engine);
+    linker.func_wrap("", "a", |mut cx: Caller<'_, _>| {
+        Ok(Some(ExternRef::new(&mut cx, 100)?))
+    })?;
+    let ty = FuncType::new(&engine, [], [ValType::EXTERNREF]);
+    linker.func_new("", "b", ty, |mut cx, _, results| {
+        results[0] = ExternRef::new(&mut cx, 100)?.into();
+        Ok(())
+    })?;
+
+    let mut store = Store::new(&engine, ());
+    let instance = linker.instantiate_async(&mut store, &module).await?;
+    let a = instance.get_typed_func::<(), ()>(&mut store, "a")?;
+    a.call_async(&mut store, ()).await?;
+
+    let mut store = Store::new(&engine, ());
+    let instance = linker.instantiate_async(&mut store, &module).await?;
+    let b = instance.get_typed_func::<(), ()>(&mut store, "b")?;
+    b.call_async(&mut store, ()).await?;
+
+    Ok(())
+}