Fix cyclic ref in WasmerEnv #2327

MarkMcCaskey · 2021-05-19T20:28:23Z

This is an alternative to #2312 to fix #2304

The core idea is to have a WeakInstanceRef too. The main concern is that Memory (from a WasmerEnv) should behave like it has a strong instance ref if it's cloned. Currently we always convert weak VMMemorys into strong ones and if we can't, we panic.

1 other edge case not handled yet: we need to make sure that when the InstanceRef is weak that everything still works as expected (for example when the Instance it points to is gone). Or maybe we don't. WasmerEnvs are always passed by & and Cloneing forces them to upgrade into Strong or panic. So there's no real case where this can happen except for when you're calling into a host function from Wasm while the Instance is freed. Doing that means that you're already executing uninitialized memory as code so it shouldn't be possible / if it is, the concern is not withWasmerEnv.

Review

Add a short description of the change to the CHANGELOG.md file

…cyclic-ref

syrusakbary · 2021-05-26T18:06:12Z

lib/vm/src/export.rs

+            instance_ref: self
+                .instance_ref
+                .as_ref()
+                .map(|v| WeakOrStrongInstanceRef::Strong(v.get_strong().unwrap())),


Cloning it automatically make ti strong?

It's a bit weird this behavior. Perhaps we want to be more explicit having a custom method for this saying: into_strong that makes it strong.

Yeah, was unsure to what extent we want to encapsulate that logic, but this is the secret sauce that makes this solution invisible to the user and behave correctly in every situation. Weak externs automatically become strong when they're shared (via cloning)

syrusakbary · 2021-05-26T18:08:26Z

lib/api/tests/export.rs

+    f.call()?;
+
+    Ok(())
+}


We are testing the strong/weak behavior directly (is_strong_instance_ref) which is great. (Edit: we should move that functions into the test file directly, they don't seem to be used outside of this file)

But can we have an extra test that verifies the behavior is fixed without checking for strong/weak fields? (that means, without assuming the design the API)

It's not exactly easy to test because it's a memory leak test, it is possible to run some logic in a loop and if it crashes due to OOM then it triggers. My concern about that is that changes to the system (like how much virtual / real memory is available) may change when/if that test passes / fails.

lib/vm/src/instance/ref.rs

syrusakbary · 2021-05-26T18:12:44Z

lib/api/src/externals/global.rs

+
+    /// Check if the global holds a strong `InstanceRef`.
+    /// None means there's no `InstanceRef`, strong or weak.
+    // TODO: maybe feature gate this, we only need it for tests...


This is only used in tests. As such I would expect to only be visible there (can we delete it from here and move the logic somewhere else?)

Unfortunately it can't be, these accessors access private info. It has to be a method on the type

One solution to this would be to define a trait with these methods on it and export it in like wasmer::test and mark that module as #[hidden]so it doesn't show up in docs. Then these methods will only be available if you import wasmer::test::SomeTrait

lib/api/src/externals/table.rs

lib/api/src/externals/memory.rs

lib/api/src/externals/function.rs

syrusakbary · 2021-05-26T18:14:22Z

lib/api/src/externals/mod.rs

@@ -72,6 +72,15 @@ impl<'a> Exportable<'a> for Extern {
        // Since this is already an extern, we can just return it.
        Ok(_extern)
    }
+
+    fn into_weak_instance_ref(&mut self) {


I'm not super fan of this function name. I think we can just have into_weak (the API user should not care about how is implemented?)

Yeah, the reason for the extra precision is due to overlap in ownership. It's not really a weak version of the thing it's being called on. That thing can still keep things alive. The only thing being made weak here is an internal InstanceRef. So I do think to that extent, it does matter, it's generally not something users should have to interact with unless they're doing low level things.

Looking at this function again, we probably need to make it unsafe because you can probably get use after free with it. I'll investigate this with a test and maybe update it

lib/api/src/native.rs

syrusakbary · 2021-05-26T18:15:55Z

lib/vm/src/instance/ref.rs

+    /// Check if the reference contained is strong.
+    pub fn is_strong(&self) -> bool {
+        matches!(self, WeakOrStrongInstanceRef::Strong(_))
+    }


This is only used in tests. I believe we can move the logic out of this trait (perhaps into the tests?)

Yeah, it could be a utility function in tests / in the methods of all the types that use it, it seems like a useful method to have on a type like this though

syrusakbary · 2021-05-26T18:19:21Z

lib/vm/src/instance/ref.rs

+impl MemoryUsage for WeakInstanceRef {
+    fn size_of_val(&self, tracker: &mut dyn MemoryUsageTracker) -> usize {
+        mem::size_of_val(self)
+            + if let Some(ir) = self.upgrade() {


Getting the size of a value makes it strong? That seems wrong.

Yeah, weak references can't be accessed. The only way to look into this type and see how much memory might be being used is to try to make it into a strong reference. If it can become a strong reference then that memory exists and we can count it, if it's not then we can't.

And to clarify it doesn't mutate the internal value, it just needs to upgrade the reference in order to count correctly. This logic should be implemented inside of loupe itself on the Weak type, not here.

lib/vm/src/instance/ref.rs

Expose inner VM externals as an unsafe method and move logic into the tests

syrusakbary · 2021-05-28T21:49:32Z

lib/api/src/externals/function.rs

+    /// # Safety
+    /// This function is unsafe to call outside of tests for the wasmer crate
+    /// because there is no stability guarantee for the returned type and we may
+    /// make breaking changes to it at any time or remove this method.
+    #[doc(hidden)]


Good call on the unsafe to discourage usage

syrusakbary · 2021-05-28T21:49:42Z

lib/api/src/externals/function.rs

+impl Clone for Function {
+    fn clone(&self) -> Self {
+        let mut exported = self.exported.clone();
+        exported.vm_function.upgrade_instance_ref().unwrap();
+
+        Self {
+            store: self.store.clone(),
+            exported,
+        }
+    }
 }


syrusakbary · 2021-05-28T21:51:07Z

bors r+

2327: Fix cyclic ref in WasmerEnv r=syrusakbary a=MarkMcCaskey This is an alternative to #2312 to fix #2304 The core idea is to have a `WeakInstanceRef` too. The main concern is that `Memory` (from a `WasmerEnv`) should behave like it has a strong instance ref if it's cloned. Currently we always convert weak `VMMemory`s into strong ones and if we can't, we panic. 1 other edge case not handled yet: we need to make sure that when the `InstanceRef` is weak that everything still works as expected (for example when the `Instance` it points to is gone). Or maybe we don't. `WasmerEnv`s are always passed by `&` and `Clone`ing forces them to upgrade into `Strong` or panic. So there's no real case where this can happen except for when you're calling into a host function from Wasm while the Instance is freed. Doing that means that you're already executing uninitialized memory as code so it shouldn't be possible / if it is, the concern is not with`WasmerEnv`. # Review - [ ] Add a short description of the change to the CHANGELOG.md file Co-authored-by: Mark McCaskey <[email protected]> Co-authored-by: Syrus Akbary <[email protected]> Co-authored-by: Mark McCaskey <[email protected]>

bors · 2021-05-28T22:06:48Z

Build failed:

Test on linux-x64

MarkMcCaskey · 2021-05-28T23:19:25Z

bors r+

bors · 2021-05-29T00:23:40Z

Build succeeded:

kaimast · 2021-05-29T00:34:09Z

Thanks for fixing this!

Fix cyclic ref in WasmerEnv

c3d344e

MarkMcCaskey changed the base branch from simplify-instance-ref to master May 20, 2021 15:41

syrusakbary and others added 3 commits May 20, 2021 12:31

Merge branch 'master' into fix-cyclic-ref

a336996

Add tests, implement for all externs

dea2abb

Merge branch 'fix-cyclic-ref' of github.com:wasmerio/wasmer into fix-…

d3c58eb

…cyclic-ref

thedavidmeister mentioned this pull request May 21, 2021

2021 05 17 wasmer holochain/holochain#773

Merged

MarkMcCaskey mentioned this pull request May 25, 2021

Fix cyclic ref direct instance #2341

Closed

1 task

Clean up PR for review

fe8d077

MarkMcCaskey marked this pull request as ready for review May 25, 2021 19:38

MarkMcCaskey requested review from Hywan and syrusakbary as code owners May 25, 2021 19:38

MarkMcCaskey mentioned this pull request May 26, 2021

Attempt to solve cyclic ref in WasmerEnv #2312

Closed

1 task