Repository: bytecodealliance/wasmtime
Pull request #2424 (2020-11-16 19:39:43)

Fix a memory leak on returning incompatible values
This fixes an issue where if a store-incompatible value is returned from
a host-defined function then that value is leaked. Practically this
means that it's possible to accidentally leak `Func` values, but a
simple insertion of a `drop` does the trick!


github-actions[bot]: #### Subscribe to Label Action

cc @peterhuene

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

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

* peterhuene: wasmtime:api

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

[Learn more.](https://github.com/bytecodealliance/subscribe-to-label-action)
</details>

Patch:
diff --git a/crates/wasmtime/src/func.rs b/crates/wasmtime/src/func.rs
index 216ef6279..a4de5209e 100644
--- a/crates/wasmtime/src/func.rs
+++ b/crates/wasmtime/src/func.rs
@@ -1611,6 +1611,14 @@ macro_rules! impl_into_func {
                             )
                         }))
                     };
+
+                    // Note that we need to be careful when dealing with traps
+                    // here. Traps are implemented with longjmp/setjmp meaning
+                    // that it's not unwinding and consequently no Rust
+                    // destructors are run. We need to be careful to ensure that
+                    // nothing on the stack needs a destructor when we exit
+                    // abnormally from this `match`, e.g. on `Err`, on
+                    // cross-store-issues, or if `Ok(Err)` is raised.
                     match ret {
                         Err(panic) => wasmtime_runtime::resume_panic(panic),
                         Ok(ret) => {
@@ -1618,6 +1626,7 @@ macro_rules! impl_into_func {
                             // can't assume it returned a value that is
                             // compatible with this store.
                             if !ret.compatible_with_store(weak_store) {
+                                drop(ret);
                                 raise_cross_store_trap();
                             }
 


Test patch:
diff --git a/tests/all/funcref.rs b/tests/all/funcref.rs
index 940d42bcb..4abea9d79 100644
--- a/tests/all/funcref.rs
+++ b/tests/all/funcref.rs
@@ -1,4 +1,6 @@
 use super::ref_types_module;
+use std::cell::Cell;
+use std::rc::Rc;
 use wasmtime::*;
 
 #[test]
@@ -83,3 +85,28 @@ fn receive_null_funcref_from_wasm() -> anyhow::Result<()> {
 
     Ok(())
 }
+
+#[test]
+fn wrong_store() -> anyhow::Result<()> {
+    let dropped = Rc::new(Cell::new(false));
+    {
+        let store1 = Store::default();
+        let store2 = Store::default();
+
+        let set = SetOnDrop(dropped.clone());
+        let f1 = Func::wrap(&store1, move || drop(&set));
+        let f2 = Func::wrap(&store2, move || Some(f1.clone()));
+        assert!(f2.call(&[]).is_err());
+    }
+    assert!(dropped.get());
+
+    return Ok(());
+
+    struct SetOnDrop(Rc<Cell<bool>>);
+
+    impl Drop for SetOnDrop {
+        fn drop(&mut self) {
+            self.0.set(true);
+        }
+    }
+}